ci: harden GitHub Actions workflows#1156
Merged
Merged
Conversation
📝 WalkthroughWalkthroughModernizes CI/CD: restructures labeler matching, pins GitHub Actions to commit SHAs, updates runner images, and adds job-level timeouts and permissions across workflows. ChangesCI/CD Infrastructure Modernization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/pages-status-check.yml (1)
13-15:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix assert message formatting in Python shell step.
Line 15 uses named placeholders but passes positional args to
.format(...). On assertion failure, this throwsKeyErrorinstead of surfacing the intended status/error message.Suggested patch
- assert status == 'built', 'Status: {status}\nError: {errormsg}'.format(status, errormsg) + assert status == 'built', 'Status: {status}\nError: {errormsg}'.format(status=status, errormsg=errormsg)🤖 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/pages-status-check.yml around lines 13 - 15, The assert line currently uses named placeholders 'Status: {status}\nError: {errormsg}' but calls .format(status, errormsg), causing a KeyError; update the format call in the Python shell step so the template and arguments match — either pass keyword args to .format(status=status, errormsg=errormsg), switch the template to positional placeholders '{}' and use .format(status, errormsg), or replace with an f-string using status and errormsg; modify the assertion around the variables status and errormsg accordingly.
🧹 Nitpick comments (1)
.github/workflows/purge-readme-image-cache.yml (1)
15-19: ⚡ Quick winConsider adding error handling and empty-results protection.
The shell pipeline lacks explicit error handling and will fail if no camo images are found. Consider these improvements:
- Add
set -eo pipefailto fail fast on errors.- Use
xargs -rto handle empty input gracefully (or check grep exit code first).- Note that the unauthenticated
curlwill fail for private repositories.♻️ Suggested improvements
- - run: > - curl -sL "https://github.com/${GITHUB_REPOSITORY}" | - grep -oE '<img src="https?://camo.githubusercontent.com(/[^"]+' | - sed -e 's/<img src="//' | - xargs -I % curl -sX PURGE "%" + - run: | + set -eo pipefail + curl -sL "https://github.com/${GITHUB_REPOSITORY}" | + grep -oE '<img src="https?://camo.githubusercontent.com/[^"]+' | + sed -e 's/<img src="//' | + xargs -r -I % curl -sX PURGE "%"🤖 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/purge-readme-image-cache.yml around lines 15 - 19, The pipeline that fetches and purges camo images should fail fast and handle empty results; update the script wrapper to enable safe shell options (set -eo pipefail), use xargs -r (or conditional check of grep's exit status) so PURGE is skipped when no matches are found, and surface curl failures for private repos by using an authenticated curl when a GITHUB_TOKEN is available (or explicitly check curl's exit code before piping to grep); apply these changes around the commands shown (curl | grep -oE '<img src="https?://camo.githubusercontent.com/[^"]+' | sed -e 's/<img src="//' | xargs -I % curl -sX PURGE "%") and ensure errors are logged and cause the job to fail.
🤖 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.
Outside diff comments:
In @.github/workflows/pages-status-check.yml:
- Around line 13-15: The assert line currently uses named placeholders 'Status:
{status}\nError: {errormsg}' but calls .format(status, errormsg), causing a
KeyError; update the format call in the Python shell step so the template and
arguments match — either pass keyword args to .format(status=status,
errormsg=errormsg), switch the template to positional placeholders '{}' and use
.format(status, errormsg), or replace with an f-string using status and
errormsg; modify the assertion around the variables status and errormsg
accordingly.
---
Nitpick comments:
In @.github/workflows/purge-readme-image-cache.yml:
- Around line 15-19: The pipeline that fetches and purges camo images should
fail fast and handle empty results; update the script wrapper to enable safe
shell options (set -eo pipefail), use xargs -r (or conditional check of grep's
exit status) so PURGE is skipped when no matches are found, and surface curl
failures for private repos by using an authenticated curl when a GITHUB_TOKEN is
available (or explicitly check curl's exit code before piping to grep); apply
these changes around the commands shown (curl | grep -oE '<img
src="https?://camo.githubusercontent.com/[^"]+' | sed -e 's/<img src="//' |
xargs -I % curl -sX PURGE "%") and ensure errors are logged and cause the job to
fail.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a47c9355-9cf3-4b71-8e89-c44a02c3b420
📒 Files selected for processing (10)
.github/labeler.yml.github/workflows/codeql.yml.github/workflows/dependency-review.yml.github/workflows/label-commenter.yml.github/workflows/labeler.yml.github/workflows/pages-status-check.yml.github/workflows/purge-readme-image-cache.yml.github/workflows/release.yml.github/workflows/test.yml.github/workflows/update-major-tag.yml
Pin workflow actions to immutable commits, add explicit permissions and timeouts, and update labeler for the current runtime. Co-Authored-By: Codex <noreply@openai.com>
peaceiris
force-pushed
the
codex/review-github-actions-workflows
branch
from
2d1d460 to
9dcb5b9
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/test.yml (1)
66-73: ⚡ Quick winScope coverage publishing to one matrix leg to avoid redundant uploads.
Consider gating artifact upload + Codecov to
ubuntu-24.04only, matching the single-leg quality-check pattern and reducing CI noise/cost.♻️ Suggested diff
- name: Upload test coverage as artifact + if: startsWith(matrix.os, 'ubuntu-24.04') uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 with: name: coverage-${{ matrix.os }} path: coverage - - uses: codecov/codecov-action@b9fd7d16f6d7d1b5d2bec1a2887e65ceed900238 # v4.6.0 + - if: startsWith(matrix.os, 'ubuntu-24.04') + uses: codecov/codecov-action@b9fd7d16f6d7d1b5d2bec1a2887e65ceed900238 # v4.6.0🤖 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/test.yml around lines 66 - 73, The coverage artifact upload and Codecov step are running on every matrix leg; restrict them to a single leg by adding a matrix guard so they only run when matrix.os == 'ubuntu-24.04' (apply an if: condition) for the "Upload test coverage as artifact" step (actions/upload-artifact) and the codecov/codecov-action step; this keeps the existing steps and names but prevents redundant uploads on other matrix entries.
🤖 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.
Nitpick comments:
In @.github/workflows/test.yml:
- Around line 66-73: The coverage artifact upload and Codecov step are running
on every matrix leg; restrict them to a single leg by adding a matrix guard so
they only run when matrix.os == 'ubuntu-24.04' (apply an if: condition) for the
"Upload test coverage as artifact" step (actions/upload-artifact) and the
codecov/codecov-action step; this keeps the existing steps and names but
prevents redundant uploads on other matrix entries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0a0a75de-3526-43c5-9f12-3f7658fb4054
📒 Files selected for processing (10)
.github/labeler.yml.github/workflows/codeql.yml.github/workflows/dependency-review.yml.github/workflows/label-commenter.yml.github/workflows/labeler.yml.github/workflows/pages-status-check.yml.github/workflows/purge-readme-image-cache.yml.github/workflows/release.yml.github/workflows/test.yml.github/workflows/update-major-tag.yml
✅ Files skipped from review due to trivial changes (3)
- .github/workflows/update-major-tag.yml
- .github/workflows/purge-readme-image-cache.yml
- .github/workflows/dependency-review.yml
🚧 Files skipped from review as they are similar to previous changes (5)
- .github/workflows/label-commenter.yml
- .github/workflows/pages-status-check.yml
- .github/labeler.yml
- .github/workflows/labeler.yml
- .github/workflows/release.yml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
概要
ubuntu-slimorubuntu-24.04.actions/labelerto v6.1.0 and migrate.github/labeler.ymlto the current config format.参考
lib/index.jswas generated locally only soactionlintcould resolveuses: ./; it was not committed.Test plan
npm ci --ignore-scriptsnpm run buildactionlintnpm run lintnpm testgit diff --checkSummary by CodeRabbit