ci: hand built depends to source jobs via artifacts#7403
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
✅ Review complete (commit 7b8682d) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 333bbc6f1e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # scopes and cache saves fail with "cache write denied: token has no writable | ||
| # scopes" on pull_request_target and other restricted-permission events. This | ||
| # in turn causes the src build's `fail-on-cache-miss: true` restore to fail. | ||
| actions: write |
There was a problem hiding this comment.
On pull_request_target runs from fork PRs, this grants a new base-repository write scope to jobs that execute PR-controlled code: the called workflows checkout ${{ github.event.pull_request.head.sha }} and run it (for example .github/workflows/cache-depends-sources.yml:25, .github/workflows/build-depends.yml:110, and .github/workflows/build-src.yml:49). GitHub documents that pull_request_target gets read/write repository permission even for public forks and that actions: write can work with Actions APIs such as canceling runs, so a malicious PR can now execute with this new permission; keep actions: write on trusted push/scheduled cache-warming jobs instead of setting it globally. See https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax#permissions
Useful? React with 👍 / 👎.
WalkthroughThe workflows now pass built depends outputs from Estimated code review effort: 4 (Complex) | ~45 minutes Related issues: None specified Related PRs: None specified Suggested labels: ci, workflows Suggested reviewers: None specified Sequence Diagram(s)sequenceDiagram
participant build.yml
participant build-depends.yml
participant build-src.yml
participant Cache
participant ArtifactStorage
build.yml->>build-depends.yml: run depends job
build-depends.yml->>Cache: save depends/built cache when allowed
build-depends.yml->>ArtifactStorage: upload depends-built artifact
build.yml->>build-src.yml: pass depends-artifact input
build-src.yml->>Cache: restore depends/built cache
alt cache miss
build-src.yml->>ArtifactStorage: download depends-built artifact
end
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/workflows/build.yml:
- Line 13: The workflow currently grants actions:write at the top level, which
gives unnecessary write access to jobs that only build or restore caches. Update
the permissions in the build workflow so actions:write is applied only to the
cache-saving caller jobs, specifically build-depends and build-src, and remove
the workflow-level write grant.
- Around line 8-12: The workflow cache warm-up currently relies on actions:
write for pull_request_target, but low-trust runs cannot save caches and the src
build’s fail-on-cache-miss path can still fail. Update the cache strategy in the
build workflow and related reusable workflows so cache population happens only
on trusted triggers like push, schedule, or workflow_dispatch, while the
pull_request_target path remains restore-only. Keep the fix centered around the
actions/cache/save steps and the fail-on-cache-miss behavior.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 06cfa5f5-76c4-4ef2-92b7-05baf0852a50
📒 Files selected for processing (1)
.github/workflows/build.yml
| # scopes and cache saves fail with "cache write denied: token has no writable | ||
| # scopes" on pull_request_target and other restricted-permission events. This | ||
| # in turn causes the src build's `fail-on-cache-miss: true` restore to fail. | ||
| actions: write |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the workflow with line numbers.
sed -n '1,220p' .github/workflows/build.yml | cat -nRepository: dashpay/dash
Length of output: 10490
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the reusable workflows and cache usage.
for f in .github/workflows/cache-depends-sources.yml .github/workflows/build-container.yml .github/workflows/build-depends.yml .github/workflows/build-src.yml .github/workflows/lint.yml; do
echo "===== $f ====="
if [ -f "$f" ]; then
sed -n '1,220p' "$f" | cat -n
else
echo "MISSING"
fi
echo
done
echo "===== cache/action usage ====="
rg -n "actions/cache|cache/save|cache/restore|permissions:" .github/workflows -g '*.yml' -g '*.yaml'Repository: dashpay/dash
Length of output: 25911
🌐 Web query:
GitHub Actions reusable workflow job permissions job-level permissions reusable workflow caller docs
💡 Result:
In GitHub Actions, the relationship between job permissions in a caller workflow and a reusable workflow is governed by a strict hierarchy where the caller's permissions act as a maximum ceiling [1][2]. Key Principles: 1. Inheritance and Constraints: A reusable workflow inherits its permissions from the caller job [2][3]. Crucially, the reusable workflow can only downgrade these permissions; it cannot elevate them [1][2]. If a reusable workflow attempts to request permissions that were not granted by the caller job, the workflow will fail, often resulting in a startup_failure [1][4]. 2. Caller Configuration: To ensure a reusable workflow has sufficient access, you must define the necessary permissions within the job that calls it [1]. If you do not specify a permissions block in the calling job, it will default to the standard GITHUB_TOKEN permissions [2]. If those defaults are insufficient for the reusable workflow's requirements, the execution will fail [1][4]. 3. Downgrading: While you cannot exceed the caller's permissions, the reusable workflow can define its own job-level permissions to further restrict the GITHUB_TOKEN access for its specific tasks [5][2]. For example, if the caller provides write access, the reusable workflow can choose to use only read access for its internal jobs [2]. 4. Best Practice: To avoid validation errors, ensure the calling job's permissions block is a superset of the permissions required by all jobs within the called reusable workflow [4]. Example of proper configuration: jobs: call-reusable: uses:./.github/workflows/my-reusable.yml permissions: contents: read pull-requests: write # This grants the necessary ceiling for the reusable workflow with: input-var: value
Citations:
- 1: https://learnedgeek.com/Blog/Post/debugging-startup-failure-reusable-workflows
- 2: https://docs.github.com/en/enterprise-server@3.13/actions/reference/reusable-workflows-reference
- 3: https://rtfm.co.ua/en/github-actions-working-with-reusable-workflows/
- 4:
call-workflowgenerated caller jobs omit requiredpermissions:for reusable workflows github/gh-aw#21071 - 5: https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax
Scope actions:write to the cache-saving jobs .github/workflows/build.yml:13
The workflow-level write grant reaches jobs that only build or restore caches. Move actions:write onto the build-depends and build-src caller jobs that actually save caches.
🧰 Tools
🪛 zizmor (1.26.1)
[error] 13-13: overly broad permissions (excessive-permissions): actions: write is overly broad at the workflow level
(excessive-permissions)
🤖 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/build.yml at line 13, The workflow currently grants
actions:write at the top level, which gives unnecessary write access to jobs
that only build or restore caches. Update the permissions in the build workflow
so actions:write is applied only to the cache-saving caller jobs, specifically
build-depends and build-src, and remove the workflow-level write grant.
Source: Linters/SAST tools
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Minimal CI fix bumping the top-level workflow permission from actions: read to actions: write so actions/cache/save can write to cache-service-v2 under pull_request_target, resolving fail-on-cache-miss failures. The change is correct and well-documented inline. Two non-blocking suggestions remain: the PR description's claim that fork PRs are automatically read-only under pull_request_target is inaccurate, and the elevated permission is granted workflow-wide rather than scoped to the jobs that actually save caches.
Source: reviewers: opus (general), claude-sonnet-5 (general), gpt-5.5[high] (general, failed), opus (dash-core-commit-history), claude-sonnet-5 (dash-core-commit-history), gpt-5.5[high] (dash-core-commit-history, failed); verifier: opus; specialists: dash-core-commit-history
🟡 2 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `.github/workflows/build.yml`:
- [SUGGESTION] .github/workflows/build.yml:5-15: PR description's fork-PR safety rationale is inaccurate for pull_request_target
The PR description argues this change "does not grant fork PRs any new privilege (GitHub already forces PRT tokens from forks to read-only regardless of the workflow declaration)". That is how the plain `pull_request` event behaves, not `pull_request_target`. Under `pull_request_target`, GitHub runs the base repo's workflow file with the declared permissions, and it does not auto-downgrade the token to read-only for fork-originated events — that is precisely the trigger the "pwn request" guidance warns about. The reusable jobs invoked here (`build-depends.yml`, `build-src.yml`) check out `github.event.pull_request.head.sha`, so once this permissions bump lands, any approved fork PR run will build attacker-influenced source with an `actions: write` token. The functional fix is still correct (the cache-service-v2 write requirement is real), but before relying on that safety claim, confirm that first-time-contributor approval gates are in place for `pull_request_target` runs from forks, and update the PR description so future maintainers don't inherit the wrong mental model.
- [SUGGESTION] .github/workflows/build.yml:7-15: actions: write is granted workflow-wide instead of scoped to cache-writing jobs
The `permissions:` block at the top of build.yml applies to every job in the workflow — `check-skip`, `container`, `container-slim`, `lint`, and all `*-test` jobs — none of which call `actions/cache/save`. Only the reusable `build-depends.yml` / `build-src.yml` invocations actually need `actions: write`. A job-level `permissions:` block on the depends/build entries (which acts as a ceiling for the called reusable workflow) would follow least-privilege and reduce how many jobs run with the elevated token. That matters here because those same build jobs also check out untrusted PR head code under `pull_request_target`. Not a blocker for the immediate cache-save fix, but worth scoping down as a follow-up.
Note: GitHub does not allow me to submit a normal inline/approval review on my own PR, so this is posted as a body-only COMMENT while preserving the verified findings.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Prior findings targeting the reverted actions: write grant are both FIXED — the workflow permissions block is back to actions: read and the cache-write concern no longer applies. New findings on the current head are all non-blocking: a converged commit-history concern about the self-reverting 333bbc6→fdbb36119d pair landing permanently in develop (dashpay/dash does not squash on merge), a converged doc nit that retention-days: 3 disagrees with the commit message's '1 day' claim, and a stale-PR-description observation.
Source: reviewers: opus (general), claude-sonnet-5 (general), gpt-5.5[high] (general, failed), opus (dash-core-commit-history), claude-sonnet-5 (dash-core-commit-history), gpt-5.5[high] (dash-core-commit-history, failed); verifier: opus; specialists: dash-core-commit-history
2 suggestion(s) | 1 nitpick(s)
Prior finding reconciliation
- FIXED
PR description's fork-PR safety rationale is inaccurate for pull_request_target— FIXED. Commit fdbb361 reverted theactions: writegrant back toactions: read(build.yml:14) and replaced the disputed 'fork PRT tokens are read-only regardless' comment with an accurate rationale referencing the June 2026 GitHub cache hardening. No elevated token is granted at all in the current head, so the security-rationale accuracy concern no longer applies to shipped code. - FIXED
actions: write is granted workflow-wide instead of scoped to cache-writing jobs— FIXED. Theactions: writegrant has been removed entirely — build.yml:7-16 now declaresactions: read,contents: read,packages: write. There is no elevated scope left to narrow; the artifact-handoff design in f036156 avoids ever needing writable cache scopes onpull_request_targetruns.
[SUGGESTION] Squash the self-reverting 333bbc6 → fdbb361 pair before merge
.github/workflows/build.yml:7-16
Commit 333bbc6 flips the workflow permissions: block from actions: read to actions: write; commit fdbb361 (three commits later in the same stack) reverts that exact line back to actions: read. Diffing 333bbc6..fdbb361 confirms the two commits cancel each other out on .github/workflows/build.yml with only a comment-text swap remaining. Because dashpay/dash merges PR stacks without squashing, both no-op-in-aggregate commits will land in develop's permanent history. Two hygiene hazards result: (a) git bisect landing on 333bbc6 will be looking at a security-relevant permission bump the author already knows is wrong (grants writable scope on a pull_request_target workflow that executes PR-controlled code); (b) git blame on the permissions: block will forever show actions: read → actions: write → actions: read churn that had no lasting effect. Recommend rebasing to drop 333bbc6 and fdbb361, keeping only f036156 (which is a clean, single-purpose commit implementing the artifact-handoff fix), and folding the useful rationale from fdbb361's message (why actions: write cannot fix this under June 2026 cache hardening) into the retained commit body.
[NITPICK] retention-days: 3 disagrees with the commit message's "1 day" description
.github/workflows/build-depends.yml:165
The f036156 commit message describes the artifact as "short-lived (1 day, stored-compression-only)". compression-level: 0 matches the "stored-compression-only" half, but retention-days: 3 keeps the artifact three times longer than documented. Not harmful (still well below the 90-day default), but the code and the commit rationale disagree, which will confuse future maintainers auditing the PR. Either align the message on 3 or drop retention-days to 1.
Suggested replacement:
retention-days: 1
[SUGGESTION] PR title and description still describe the abandoned actions: write approach
.github/workflows/build.yml:1
The PR is titled ci: grant actions: write to unblock cache/save on pull_request_target and its description walks through why that grant is the right fix. The current head reverts that change (fdbb361) and solves the problem via same-run artifact handoff (f036156) instead. Commit messages on the stack correctly explain the new approach, but the PR title/body do not — reviewers reading only the description will be misled. Update the PR title and body to describe the artifact-handoff design (and briefly note why actions: write was abandoned as a fork-PR-safety concession) so the record stays coherent.
Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
- [SUGGESTION] .github/workflows/build.yml:7-16: Squash the self-reverting 333bbc6f1e → fdbb36119d pair before merge
Commit 333bbc6f1e flips the workflow `permissions:` block from `actions: read` to `actions: write`; commit fdbb36119d (three commits later in the same stack) reverts that exact line back to `actions: read`. Diffing 333bbc6f1e..fdbb36119d confirms the two commits cancel each other out on `.github/workflows/build.yml` with only a comment-text swap remaining. Because dashpay/dash merges PR stacks without squashing, both no-op-in-aggregate commits will land in develop's permanent history. Two hygiene hazards result: (a) `git bisect` landing on 333bbc6f1e will be looking at a security-relevant permission bump the author already knows is wrong (grants writable scope on a pull_request_target workflow that executes PR-controlled code); (b) `git blame` on the `permissions:` block will forever show `actions: read` → `actions: write` → `actions: read` churn that had no lasting effect. Recommend rebasing to drop 333bbc6f1e and fdbb36119d, keeping only f0361564b3 (which is a clean, single-purpose commit implementing the artifact-handoff fix), and folding the useful rationale from fdbb36119d's message (why `actions: write` cannot fix this under June 2026 cache hardening) into the retained commit body.
- [NITPICK] .github/workflows/build-depends.yml:165: retention-days: 3 disagrees with the commit message's "1 day" description
The f0361564b3 commit message describes the artifact as "short-lived (1 day, stored-compression-only)". `compression-level: 0` matches the "stored-compression-only" half, but `retention-days: 3` keeps the artifact three times longer than documented. Not harmful (still well below the 90-day default), but the code and the commit rationale disagree, which will confuse future maintainers auditing the PR. Either align the message on 3 or drop retention-days to 1.
- [SUGGESTION] .github/workflows/build.yml:1: PR title and description still describe the abandoned `actions: write` approach
The PR is titled `ci: grant actions: write to unblock cache/save on pull_request_target` and its description walks through why that grant is the right fix. The current head reverts that change (fdbb36119d) and solves the problem via same-run artifact handoff (f0361564b3) instead. Commit messages on the stack correctly explain the new approach, but the PR title/body do not — reviewers reading only the description will be misled. Update the PR title and body to describe the artifact-handoff design (and briefly note why `actions: write` was abandoned as a fork-PR-safety concession) so the record stays coherent.
thepastaclaw
changed the title
Since GitHub's June 2026 cache hardening, untrusted triggers such as pull_request_target receive read-only cache tokens: restores still work, but `actions/cache/save` is denied no matter what `permissions:` the workflow requests. When a PR changed the depends cache key, the depends job built successfully but could not save, and every downstream src job then died at its `fail-on-cache-miss: true` restore. Replace the denied save with a same-run handoff: - build-depends.yml only saves the persistent cache on trusted events (push; later PR runs restore it read-only). On pull_request_target it uploads the freshly built depends/built/<host> tree as a short-lived (1 day, stored-compression-only) artifact instead and exposes the artifact name as a new `built-artifact` output, empty on cache hit. - build-src.yml accepts the artifact name as optional input `depends-artifact`. The depends restore no longer hard-fails: on a cache miss it downloads the artifact, and only errors out if neither the cache entry nor an artifact exists. - build.yml threads the new output into every build-src.yml call. This keeps the workflow least-privileged: PR runs that check out and execute PR-controlled code hold no writable cache or repo scopes, while trusted push runs keep warming the shared caches. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
force-pushed
the
fix/ci-depends-cache-restore
branch
from
f036156 to
7b8682d
Compare
|
Handled the current review feedback in
@coderabbitai review |
|
✅ Action performedReview finished.
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Incremental verification on head 7b8682d: all three prior findings are FIXED — the stack is now a single clean commit (self-reverting actions:write grant/revert pair squashed away), build-depends.yml:165 uses retention-days: 1 matching the commit-message's '1 day' description, and the commit message accurately describes the artifact-handoff design. No new findings surfaced in the latest delta or on cumulative re-read of the three workflow files. Both opus-* and sonnet5-* reviewers converge on a clean approval.
Source: reviewers: opus (general), claude-sonnet-5 (general), gpt-5.5[high] (general, failed), opus (dash-core-commit-history), claude-sonnet-5 (dash-core-commit-history), gpt-5.5[high] (dash-core-commit-history, failed); verifier: opus; specialists: dash-core-commit-history
Note: GitHub does not allow me to submit an approval review on my own PR, so this clean result is posted as a body-only COMMENT for the exact head SHA.
|
CI triage update: the current red source-build jobs are expected for this PR because I re-verified the PR-head workflow logic: So the remaining choice is operational rather than another workflow patch: either maintainers accept/override the structurally-red PR checks after review, or we stage-test the same workflow change on a trusted branch/event before merging. |
2 participants
Summary
GitHub changed Actions cache behavior on June 26, 2026: untrusted triggers now receive read-only Actions cache tokens. See GitHub's changelog entry, "Read-only Actions cache for untrusted triggers".
For
pull_request_targetruns, a depends job can still build dependencies successfully, butactions/cache/saveonly warns when the token cannot reserve/write the cache. The later source-build jobs then fail if their depends cache restore misses.This PR keeps
build.ymlleast-privileged (actions: read) and adds a same-run handoff for freshly built depends trees:pull_request_targetruns skip the denied cache save path;Why
The old workflow assumed that if a depends job built
depends/built/<host>, the result could be saved to the Actions cache for downstream source jobs. That is no longer true for low-trust PR-triggered runs: restore may be allowed, while save is denied.The previous attempted fix granted
actions: writeat the top level. That was reverted because the workflow checks out and executes PR-controlled code, and writable Actions scope is the wrong security model forpull_request_target.Implementation
.github/workflows/build-depends.ymlbuilt-artifactoutput;pull_request_target;depends/built/<host>asdepends-built-<target>when a fresh build was needed..github/workflows/build-src.ymldepends-artifactinput;.github/workflows/build.ymlbuilt-artifactoutput to the matching source jobs;actions: read.Current CI caveat
The PR's own
pull_request_targetrun cannot fully validate these reusable workflow changes yet, because GitHub runs reusable workflows from the base branch in this context. The current failing run shows source jobs using:That means the red source jobs are still using
develop's oldfail-on-cache-miss: truerestore path, not this PR's artifact fallback.References
Test plan
pull_request_targetrun that needs a fresh depends build.pull_request_target.depends-built-<target>artifacts.pushruns still save persistent depends caches.