refactor(ci): extract reusable CI checks and setup-bun composite action#152
refactor(ci): extract reusable CI checks and setup-bun composite action#152
Conversation
|
Stack: slack-notifications
Part of a stacked PR chain. Do not merge manually. |
57bd831 to
3e2a016
Compare
4bc124f to
546e8d5
Compare
3e2a016 to
82653fa
Compare
546e8d5 to
7d6c0cf
Compare
a52dece to
f771daf
Compare
2503552 to
c6b93f8
Compare
|
|
c6b93f8 to
0e5c2d0
Compare
484281b to
d181be8
Compare
f9e59a5 to
21ed234
Compare
d181be8 to
402aa66
Compare
21ed234 to
ba656ce
Compare
3858f38 to
23b792d
Compare
23b792d to
50707ee
Compare
📝 WalkthroughWalkthroughAdds a composite GitHub Action at .github/actions/setup-bun/action.yml that installs Bun, optionally restores or saves Bun’s dependency cache, then runs Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Summary of changes
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/release.yml (1)
368-372: Consider using the setup-bun composite action here for consistency.The
snapshotjob still uses the inlineoven-sh/setup-bun@v2andbun install --frozen-lockfilepattern, whileversioningandcanary-versionhave been updated to use./.github/actions/setup-bun. For consistency and maintainability, consider updating this job as well.♻️ Suggested refactor
- uses: actions/checkout@v6 with: ref: ${{ steps.pr.outputs.sha }} - - uses: oven-sh/setup-bun@v2 - - run: bun install --frozen-lockfile + - uses: ./.github/actions/setup-bun🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml around lines 368 - 372, Replace the inline bun setup in the snapshot job: remove the two steps that use oven-sh/setup-bun@v2 and run "bun install --frozen-lockfile" and instead call the local composite action ./.github/actions/setup-bun (same pattern used by versioning and canary-version). Keep the existing checkout step and its ref, and ensure the new uses: ./.github/actions/setup-bun step provides any required inputs or outputs that the rest of the job needs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/release.yml:
- Around line 368-372: Replace the inline bun setup in the snapshot job: remove
the two steps that use oven-sh/setup-bun@v2 and run "bun install
--frozen-lockfile" and instead call the local composite action
./.github/actions/setup-bun (same pattern used by versioning and
canary-version). Keep the existing checkout step and its ref, and ensure the new
uses: ./.github/actions/setup-bun step provides any required inputs or outputs
that the rest of the job needs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aacbe784-6a49-4a9a-a968-48bfcabf49c3
📒 Files selected for processing (4)
.github/actions/setup-bun/action.yml.github/workflows/ci.yml.github/workflows/notify-failure.yml.github/workflows/release.yml
- Add .github/actions/setup-bun composite action that encapsulates Bun installation, optional dependency caching, and bun install - Add .github/workflows/ci-checks.yml reusable workflow containing the full CI suite (build, lint, test, e2e) with configurable ref and run-e2e inputs - Simplify ci.yml to a single ci-checks.yml call - Gate release.yml stable/canary/snapshot paths on CI checks passing before versioning or building - Update notify-failure.yml to use the setup-bun composite action
Add workflow_call trigger to ci.yml so it serves as both the PR workflow and a reusable workflow called by release.yml. This eliminates the separate ci-checks.yml indirection.
50707ee to
522ce87
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.github/workflows/release.yml (1)
39-39: Inconsistentcacheinput usage compared to ci.yml.The
versioningjob (line 39) andcanary-versionjob (line 186) use the composite action without specifyingcache:, which defaults to"none"(no caching). In contrast, ci.yml always explicitly setscacheto eitherread-writeorrestore.For consistency and to match the caching behavior pattern used in ci.yml, explicitly specify the cache mode:
Add explicit cache specification
- uses: ./.github/actions/setup-bun + with: + cache: restore🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml at line 39, The composite action invocation "uses: ./.github/actions/setup-bun" in the release workflow's versioning and canary-version jobs omits the cache input; update those invocations to explicitly include a cache mode (e.g., add "cache: read-write" or "cache: restore" as used in ci.yml) so the action's caching behavior is consistent with ci.yml—locate the two occurrences of "uses: ./.github/actions/setup-bun" in the release.yml (in the versioning and canary-version jobs) and add the chosen "cache:" input to each..github/workflows/ci.yml (1)
60-66: Review the cache poisoning risk through a composite action in the untrusted ref.The
setup-buncomposite action is executed from the checked-out untrustedref, allowing a maliciousaction.ymlto run arbitrary steps. While the specific code path (lines 60-66) usescache: restore(read-only), thebuildjob earlier in the workflow usescache: read-writewith the same deterministic cache key (bun-${{ runner.os }}-${{ hashFiles('bun.lock') }}). A malicious build job could write poisoned content to the cache, which subsequent jobs would then read.Current mitigations:
pull_requesttrigger: limited write accessworkflow_callrestrictions: called only on push to main or snapshot buildsHowever, if the untrusted ref shares the same
bun.lockhash as a previous build, the cache key would collide. Consider pinning the composite action checkout to a trusted ref (e.g., main) separately from the code checkout to prevent executing a malicious action definition, though this requires additional workflow restructuring.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 60 - 66, The composite action ./.github/actions/setup-bun is being checked out from the untrusted ref (actions/checkout@v6) which permits a malicious action.yml to run and poison the shared cache (cache key bun-${{ runner.os }}-${{ hashFiles('bun.lock') }}); fix by pinning the composite action to a trusted ref: either (A) replace the local-relative use with an explicit repository reference pinned to a trusted ref (e.g., uses: your-org/your-repo/.github/actions/setup-bun@main) or (B) perform a second, separate actions/checkout@v6 for the .github/actions/setup-bun path with ref: main (or other trusted branch) before the step that uses ./ .github/actions/setup-bun, so the composite action is always sourced from a trusted commit while leaving the main checkout for the untrusted ref intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 60-66: The composite action ./.github/actions/setup-bun is being
checked out from the untrusted ref (actions/checkout@v6) which permits a
malicious action.yml to run and poison the shared cache (cache key bun-${{
runner.os }}-${{ hashFiles('bun.lock') }}); fix by pinning the composite action
to a trusted ref: either (A) replace the local-relative use with an explicit
repository reference pinned to a trusted ref (e.g., uses:
your-org/your-repo/.github/actions/setup-bun@main) or (B) perform a second,
separate actions/checkout@v6 for the .github/actions/setup-bun path with ref:
main (or other trusted branch) before the step that uses ./
.github/actions/setup-bun, so the composite action is always sourced from a
trusted commit while leaving the main checkout for the untrusted ref intact.
In @.github/workflows/release.yml:
- Line 39: The composite action invocation "uses: ./.github/actions/setup-bun"
in the release workflow's versioning and canary-version jobs omits the cache
input; update those invocations to explicitly include a cache mode (e.g., add
"cache: read-write" or "cache: restore" as used in ci.yml) so the action's
caching behavior is consistent with ci.yml—locate the two occurrences of "uses:
./.github/actions/setup-bun" in the release.yml (in the versioning and
canary-version jobs) and add the chosen "cache:" input to each.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d70cc922-6e8a-4e58-bd4a-c81466bd1943
📒 Files selected for processing (4)
.github/actions/setup-bun/action.yml.github/workflows/ci.yml.github/workflows/notify-failure.yml.github/workflows/release.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/notify-failure.yml
- .github/actions/setup-bun/action.yml
|
- uses: actions/checkout@v6
with:
ref: ${{ steps.pr.outputs.sha }}
- - uses: oven-sh/setup-bun@v2
- - run: bun install --frozen-lockfile
+ - uses: ./.github/actions/setup-bun |
Add cache-writable input to ci.yml (default true). snapshot-ci passes false so the build job on an untrusted PR head SHA cannot populate the main-branch-scoped cache via the issue_comment reusable-workflow context, closing a cache-poisoning vector surfaced by CodeQL through the snapshot path. Also set cache: restore explicitly on the versioning and canary-version jobs in release.yml, matching the pattern already used in ci.yml so both consume the cache populated by the push-to-main build.
There was a problem hiding this comment.
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)
25-27:⚠️ Potential issue | 🟠 MajorUse
inputs.refin concurrency group to prevent cross-caller cancellations.Lines 25-27 group runs by
github.ref, which in a reusable workflow equals the caller's ref. Whensnapshot-ciin.github/workflows/release.ymlcalls this workflow with a different SHA via therefinput, the concurrency bucket remains the same (both use the caller's default branch ref), causing unrelated reusable-workflow runs to cancel each other.Suggested fix
concurrency: - group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} + group: ${{ github.workflow }}-${{ github.event.pull_request.number || inputs.ref || github.ref }} cancel-in-progress: true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 25 - 27, The concurrency group currently uses github.ref which can be the caller's ref for reusable workflows and causes unrelated runs to cancel; update the concurrency group expression to prefer the caller-provided ref input (github.event.inputs.ref) before falling back to github.ref, e.g. change group to use ${{ github.workflow }}-${{ github.event.pull_request.number || github.event.inputs.ref || github.ref }}, referencing the concurrency/group key and the github.event.inputs.ref fallback to prevent cross-caller cancellations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 25-27: The concurrency group currently uses github.ref which can
be the caller's ref for reusable workflows and causes unrelated runs to cancel;
update the concurrency group expression to prefer the caller-provided ref input
(github.event.inputs.ref) before falling back to github.ref, e.g. change group
to use ${{ github.workflow }}-${{ github.event.pull_request.number ||
github.event.inputs.ref || github.ref }}, referencing the concurrency/group key
and the github.event.inputs.ref fallback to prevent cross-caller cancellations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 802fd3be-f5f0-42e8-9222-bfcb8a7e1938
📒 Files selected for processing (2)
.github/workflows/ci.yml.github/workflows/release.yml
…nding Revert ci.yml jobs to inline oven-sh/setup-bun@v2 + actions/cache + bun install, instead of ./.github/actions/setup-bun. CodeQL flags uses: ./<local-action> after checkout of an untrusted ref when the workflow is reachable from an issue_comment trigger (via snapshot-ci), because a malicious action.yml would execute in the default-branch cache scope regardless of inputs passed to the composite. The cache-writable input added in the previous commit did not close this vector because CodeQL (correctly) does not assume action.yml honors inputs. Drop the input and the snapshot-ci override. Composite action remains in use by release.yml's versioning and canary-version jobs, which run on push to main (trusted context).
Summary
.github/actions/setup-buncomposite action (Bun install + optionaldependency caching +
bun install). Used byrelease.yml'sversioning,canary-version, andnotify-failurejobs, all of which run in trustedcontexts (push to main or workflow_call from push to main).
snapshot-cijob inrelease.ymlthat reusesci.ymlviaworkflow_call, gating snapshot publishing on CI passing first.before versioning or building.
ci.ymlkeeps inlineoven-sh/setup-bun@v2+actions/cache+bun installin each job rather than adopting the composite action. Switching would trip
CodeQL's "checkout of untrusted code in a privileged context" finding via
the
issue_comment->snapshot-ci->ci.ymlpath, sinceuses: ./...would resolve
action.ymlfrom the untrusted PR ref.Test plan