ci: improve release contributor detection#161
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds release gating and enhanced PR discovery to the build workflow, tweaks Docker push triggers to target Dockerfile/requirements, and refactors the cleanup workflow to support mode-based deletion, retention, and metricized summaries. ChangesRelease Workflow PR Detection Enhancement
Cleanup Workflow Refactor
Sequence Diagram(s)sequenceDiagram
participant Workflow as Build Workflow
participant Version as Version Step
participant GraphQL as GitHub GraphQL
participant REST as GitHub REST
participant Changelog as Changelog Step
Workflow->>Version: generate version (check tags for commit)
Version-->>Workflow: outputs should_release
alt should_release == 'true'
Workflow->>GraphQL: query associatedPullRequests for commit SHAs
GraphQL-->>REST: return PR numbers for SHAs
REST-->>Changelog: return PR title / author / url
Workflow->>Changelog: run changelog generation
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 minutes Poem
🚥 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)
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 |
|
Starting my review of the release contributor detection improvements in |
|
| Filename | Overview |
|---|---|
| .github/workflows/cleanup.yml | Adds manual cleanup modes and stale branch release pruning, with a destructive stale-detection failure path. |
Reviews (3): Last reviewed commit: "ci: extend release cleanup modes" | Re-trigger Greptile
![mirrobot-agent[bot]](https://avatars.githubusercontent.com/in/2140342?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overall Assessment
This is a well-structured improvement to the release contributor detection logic. The two-layer approach (subject-line parsing + GraphQL associatedPullRequests) correctly addresses the gap where squash and rebase merges were being missed by the old merge-commit-only detection. The dedup/sort step and the cleaner PR URL fetching (pulling html_url from the API rather than constructing it) are nice improvements.
Architectural Feedback
The layered strategy (Layer PR-1 for commit subjects, Layer PR-2 for GraphQL) is sound and maintainable. The TODO note at the top of the file is a good touch for keeping this aligned with opencode-agent-variants.
Key Suggestions
- Regex false positives (line 668): The
#numberpattern in Layer PR-1 will match issue references and other incidental#123mentions in commit subjects, not just PR numbers. See the inline suggestion for a tighter pattern. - Per-commit GraphQL calls (lines 673-685): The loop issues one API call per commit. For larger releases this could be slow — consider batching or adding an early-exit optimization.
Questions for the Author
- Have you tested this with a release range that includes both squash-merged and merge-committed PRs to verify both layers contribute results without duplicates?
This review was generated by an AI assistant.
|
|
||
| # Layer PR-1: parse PR numbers from commit subjects. This keeps the old | ||
| # merge-commit behavior and also catches squash commits that include #123. | ||
| SUBJECT_PRS=$(git log "$PREV_TAG".."$CURRENT_SHA" --format='%s' 2>/dev/null | grep -oE 'Merge pull request #[0-9]+|#[0-9]+' | grep -oE '[0-9]+' || true) |
There was a problem hiding this comment.
This regex will match any #123 occurrence in commit subjects — including false positives like issue references (fixes #123, related to #456). Consider tightening to only match explicit PR patterns:
| SUBJECT_PRS=$(git log "$PREV_TAG".."$CURRENT_SHA" --format='%s' 2>/dev/null | grep -oE 'Merge pull request #[0-9]+|#[0-9]+' | grep -oE '[0-9]+' || true) | |
| SUBJECT_PRS=$(git log "$PREV_TAG".."$CURRENT_SHA" --format='%s' 2>/dev/null | grep -oE 'Merge pull request #[0-9]+|\([^)]*#[0-9]+[^)]*\)' | grep -oE '[0-9]+' || true) |
This limits matching to Merge pull request #N subjects and (#N) parenthesized references, which are the conventional squash/merge PR formats.
| COMMIT_SHAS=$(git rev-list "$PREV_TAG".."$CURRENT_SHA" 2>/dev/null || true) | ||
| while read -r SHA; do | ||
| if [ -z "$SHA" ]; then | ||
| continue | ||
| fi | ||
| ASSOCIATED_PRS=$(gh api graphql \ | ||
| -f owner="$OWNER" \ | ||
| -f name="$REPO_NAME" \ | ||
| -f oid="$SHA" \ | ||
| -f query='query($owner:String!, $name:String!, $oid:GitObjectID!) { repository(owner:$owner, name:$name) { object(oid:$oid) { ... on Commit { associatedPullRequests(first: 10) { nodes { number } } } } } }' \ | ||
| --jq '.data.repository.object.associatedPullRequests.nodes[].number' 2>/dev/null || true) | ||
| PR_NUMBERS=$(printf '%s\n%s\n' "$PR_NUMBERS" "$ASSOCIATED_PRS") | ||
| done <<< "$COMMIT_SHAS" |
There was a problem hiding this comment.
This issues a GraphQL API call per commit in the range. For releases with many commits (20-50+), this adds significant runtime. Two options to consider:
- Batch via GraphQL aliases — query multiple OIDs in a single request using
c0: ... on Commit { ... }, c1: ...aliases. - Early exit — break out of the loop once no new PR numbers are being discovered (most PRs are found in the first few commits of a range).
Neither is blocking, but for a CI workflow that may run on every release, the N API calls could become noticeable.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| # Layer PR-1: parse PR numbers from commit subjects. This keeps the old | ||
| # merge-commit behavior and also catches squash commits that include #123. | ||
| SUBJECT_PRS=$(git log "$PREV_TAG".."$CURRENT_SHA" --format='%s' 2>/dev/null | grep -oE 'Merge pull request #[0-9]+|#[0-9]+' | grep -oE '[0-9]+' || true) |
There was a problem hiding this comment.
This scans every commit subject for any #123 token, not just merge commits or squash-merge PR suffixes. When a release range contains a normal commit like fix: handle timeout #42, this code treats 42 as a pull request number. If PR 42 exists, the release notes can add an unrelated Community Contributions entry.
| SUBJECT_PRS=$(git log "$PREV_TAG".."$CURRENT_SHA" --format='%s' 2>/dev/null | grep -oE 'Merge pull request #[0-9]+|#[0-9]+' | grep -oE '[0-9]+' || true) | |
| SUBJECT_PRS=$(git log "$PREV_TAG".."$CURRENT_SHA" --format='%s' 2>/dev/null | grep -oE '^Merge pull request #[0-9]+|\(#[0-9]+\)$' | grep -oE '[0-9]+' || true) |
| COMMIT_SHAS=$(git rev-list "$PREV_TAG".."$CURRENT_SHA" 2>/dev/null || true) | ||
| while read -r SHA; do | ||
| if [ -z "$SHA" ]; then | ||
| continue | ||
| fi | ||
| ASSOCIATED_PRS=$(gh api graphql \ | ||
| -f owner="$OWNER" \ | ||
| -f name="$REPO_NAME" \ | ||
| -f oid="$SHA" \ | ||
| -f query='query($owner:String!, $name:String!, $oid:GitObjectID!) { repository(owner:$owner, name:$name) { object(oid:$oid) { ... on Commit { associatedPullRequests(first: 10) { nodes { number } } } } } }' \ | ||
| --jq '.data.repository.object.associatedPullRequests.nodes[].number' 2>/dev/null || true) | ||
| PR_NUMBERS=$(printf '%s\n%s\n' "$PR_NUMBERS" "$ASSOCIATED_PRS") |
There was a problem hiding this comment.
This makes one GraphQL request for every commit between the previous tag and the current SHA. On a large release range, such as a parent-tag fallback or a long-lived branch, the release job can spend a long time in this loop or hit GitHub API limits. Since errors are redirected and swallowed, failed requests silently drop associated PRs from the Community Contributions section.
![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:
- Around line 671-685: The current loop issues a gh api graphql call per commit
(COMMIT_SHAS -> while read -> ASSOCIATED_PRS), which can exhaust rate limits;
replace it by batching commits into chunks and issuing a single gh api graphql
call per chunk that builds a GraphQL query with aliased object(oid: "<sha>")
fields (e.g., alias1: object(oid: "...") { ... on Commit {
associatedPullRequests { nodes { number } } } } ) for all SHAs in the chunk,
then extract all returned numbers and append them into PR_NUMBERS; implement
chunking (e.g., 50–100 SHAs per request) and fall back gracefully on partial
failures (continue on errors) to preserve robustness while drastically reducing
the number of API calls.
- Around line 662-663: Move direct GitHub context expansions out of the script
body and into the step-level env block: define environment variables (e.g.,
OWNER, REPO_NAME, REPO_FULL) in the step's env: using the GitHub contexts (${ {
github.repository_owner } }, ${ { github.event.repository.name } }, ${ {
github.repository } }) and then update any shell script references that
currently interpolate those contexts (OWNER and REPO_NAME usages in the script)
to read from the environment variables instead; this removes template-injection
instances flagged by static analysis while keeping the same values available to
the step.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 449f1297-ab38-4115-a4ec-123d5b6575d8
📒 Files selected for processing (1)
.github/workflows/build.yml
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Greptile Review
- GitHub Check: review-pr
🧰 Additional context used
🪛 zizmor (1.25.2)
.github/workflows/build.yml
[warning] 662-662: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[error] 663-663: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[warning] 695-695: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🔇 Additional comments (3)
.github/workflows/build.yml (3)
3-9: LGTM!
662-669: ⚡ Quick winVerify Layer PR runs when the changelog has no author placeholders.
This block lives inside the "Resolve GitHub Usernames" step, which exits early at Line 514 (
exit 0) whenTOTAL_PLACEHOLDERS -eq 0. In that case the entire new Layer PR detection is skipped. Placeholders normally exist whenever commits appear in the changelog, but if a community PR's commits fall into cliff-hidden categories (e.g.,chore/ci), the changelog body can have zero placeholders while a PR still exists in range — so the "robust" detection wouldn't run. Please confirm this is intended, or relocate the Layer PR block ahead of the early-exit (or into its own step) so PR detection is independent of placeholder presence.
689-708: LGTM!
| OWNER="${{ github.repository_owner }}" | ||
| REPO_NAME="${{ github.event.repository.name }}" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Move GitHub context expansions into env: to clear the template-injection findings.
zizmor flags lines 662, 663 (error), and 695 as template-injection because the ${{ … }} values are interpolated directly into the script body. Real exploitability here is low (github.repository_owner, github.event.repository.name, and github.repository cannot contain shell metacharacters), but routing them through the step env: block is the standard hardening and resolves the gating static-analysis error on Line 663.
🔒 Proposed hardening via step env
Add to the step's env: block (near Line 486):
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
OWNER: ${{ github.repository_owner }}
REPO_NAME: ${{ github.event.repository.name }}
REPO_FULL: ${{ github.repository }}Then reference the env vars instead of interpolating:
- OWNER="${{ github.repository_owner }}"
- REPO_NAME="${{ github.event.repository.name }}"
PR_NUMBERS=""- PR_INFO=$(gh api "repos/${{ github.repository }}/pulls/$PR_NUM" \
+ PR_INFO=$(gh api "repos/$REPO_FULL/pulls/$PR_NUM" \
--jq '{title: .title, author: .user.login, url: .html_url}' 2>/dev/null || echo "{}")🧰 Tools
🪛 zizmor (1.25.2)
[warning] 662-662: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[error] 663-663: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🤖 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 around lines 662 - 663, Move direct GitHub
context expansions out of the script body and into the step-level env block:
define environment variables (e.g., OWNER, REPO_NAME, REPO_FULL) in the step's
env: using the GitHub contexts (${ { github.repository_owner } }, ${ {
github.event.repository.name } }, ${ { github.repository } }) and then update
any shell script references that currently interpolate those contexts (OWNER and
REPO_NAME usages in the script) to read from the environment variables instead;
this removes template-injection instances flagged by static analysis while
keeping the same values available to the step.
| # Layer PR-2: GraphQL associatedPullRequests catches squash/rebase flows | ||
| # even when the commit subject does not include a PR number. | ||
| COMMIT_SHAS=$(git rev-list "$PREV_TAG".."$CURRENT_SHA" 2>/dev/null || true) | ||
| while read -r SHA; do | ||
| if [ -z "$SHA" ]; then | ||
| continue | ||
| fi | ||
| ASSOCIATED_PRS=$(gh api graphql \ | ||
| -f owner="$OWNER" \ | ||
| -f name="$REPO_NAME" \ | ||
| -f oid="$SHA" \ | ||
| -f query='query($owner:String!, $name:String!, $oid:GitObjectID!) { repository(owner:$owner, name:$name) { object(oid:$oid) { ... on Commit { associatedPullRequests(first: 10) { nodes { number } } } } } }' \ | ||
| --jq '.data.repository.object.associatedPullRequests.nodes[].number' 2>/dev/null || true) | ||
| PR_NUMBERS=$(printf '%s\n%s\n' "$PR_NUMBERS" "$ASSOCIATED_PRS") | ||
| done <<< "$COMMIT_SHAS" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Per-commit GraphQL lookup is O(commits) API calls — watch rate limits on large ranges.
This loop issues one gh api graphql call per commit. Combined with the existing per-commit username resolution (Layer B-2) and the per-email search/users calls in "Generate Build Metadata", a large range — especially the parent-branch fallback path where the range can span many commits — can generate hundreds of API calls and risk secondary rate limiting plus long runtimes. Consider batching the associatedPullRequests lookups into a single GraphQL request using aliased object(oid:) fields (chunked), which keeps the robustness benefit while collapsing N calls into a few.
🤖 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 around lines 671 - 685, The current loop issues
a gh api graphql call per commit (COMMIT_SHAS -> while read -> ASSOCIATED_PRS),
which can exhaust rate limits; replace it by batching commits into chunks and
issuing a single gh api graphql call per chunk that builds a GraphQL query with
aliased object(oid: "<sha>") fields (e.g., alias1: object(oid: "...") { ... on
Commit { associatedPullRequests { nodes { number } } } } ) for all SHAs in the
chunk, then extract all returned numbers and append them into PR_NUMBERS;
implement chunking (e.g., 50–100 SHAs per request) and fall back gracefully on
partial failures (continue on errors) to preserve robustness while drastically
reducing the number of API calls.
Mirrowel
added
Agent Monitored
| paths: | ||
| - 'src/proxy_app/**' # Main application source code | ||
| - 'src/rotator_library/**' # Key rotation library |
There was a problem hiding this comment.
The push.paths filter now only matches source directories, so a commit that changes only this release workflow or .github/cliff.toml on dev/main will not run the build-and-release workflow. That means release workflow fixes and changelog-template changes can merge without exercising the release path they changed, and the updated release notes logic will not run until an unrelated source change is pushed.
| paths: | |
| - 'src/proxy_app/**' # Main application source code | |
| - 'src/rotator_library/**' # Key rotation library | |
| paths: | |
| - 'src/proxy_app/**' # Main application source code | |
| - 'src/rotator_library/**' # Key rotation library | |
| - '.github/workflows/build.yml' # This workflow file | |
| - '.github/cliff.toml' # Changelog configuration |
|
Starting my review of the idempotency and concurrency additions. The build-skip logic for duplicate runs and the path-trigger cleanup look like smart operational improvements — let me dig into the details. |
There was a problem hiding this comment.
Overall Assessment
This is a solid set of operational improvements to the release workflow. The idempotency check (detecting existing releases for the same SHA) combined with the concurrency group and the should_release gate on downstream steps is a clean approach to preventing duplicate builds. The path trigger cleanup (removing workflow files from paths:) is also sensible — manual dispatch remains available for testing workflow changes.
Key Suggestions
- Build job still runs on duplicates (see inline): The
should_releasecheck lives in thereleasejob, so thebuildjob (3 matrix runners) compiles all binaries before discovering the release already exists. A preliminarycheckjob would save significant runner time. --limit 1000edge case (see inline): The existing-release lookup could miss a match if the repo ever exceeds 1000 releases. Low risk with pruning enabled, but worth documenting or hardening.
Path Trigger Changes
Removing .github/workflows/build.yml, cliff.toml, and .github/workflows/docker-build.yml from the paths: triggers is consistent and reasonable. Just be aware that edits to these files won't auto-trigger a build — manual dispatch is the escape hatch.
Prune Behavior Change
The Prune Old Releases step changed from if: always() to if: steps.version.outputs.should_release == 'true'. The removed comments already discussed this — pruning now only runs when a release is actually created. This is the right call.
Previous Review Feedback
My earlier review on commit f2238d4 raised two points on the contributor detection code (lines 692 and 697–709): the regex matching false positives on #number patterns, and the per-commit GraphQL API calls. These haven't been addressed in this commit — they still apply.
Questions for the Author
- Have you observed duplicate workflow runs in practice that motivated the idempotency check, or is this preemptive?
This review was generated by an AI assistant.
| EXISTING_TAG=$(gh release list --repo "${{ github.repository }}" --limit 1000 --json tagName,targetCommitish \ | ||
| --jq ".[] | select(.tagName | startswith(\"$BRANCH_NAME/build-\")) | select(.targetCommitish == \"${{ github.sha }}\") | .tagName" \ | ||
| | head -n 1) | ||
|
|
||
| if [ -n "$EXISTING_TAG" ]; then | ||
| echo "Release already exists for this commit: $EXISTING_TAG" | ||
| echo "release_title=Build ($BRANCH_NAME): ${EXISTING_TAG#*/build-}" >> $GITHUB_OUTPUT | ||
| echo "release_tag=$EXISTING_TAG" >> $GITHUB_OUTPUT | ||
| echo "archive_version_part=${EXISTING_TAG%%/*}-${EXISTING_TAG#*/build-}" >> $GITHUB_OUTPUT | ||
| echo "version=${EXISTING_TAG#*/build-}" >> $GITHUB_OUTPUT | ||
| echo "timestamp=$(date -u +'%Y-%m-%d %H:%M:%S UTC')" >> $GITHUB_OUTPUT | ||
| echo "should_release=false" >> $GITHUB_OUTPUT | ||
| exit 0 | ||
| fi |
There was a problem hiding this comment.
The idempotency check correctly prevents duplicate releases, but the build job (3 OS matrix runners, lines 127–214) still compiles all binaries before the release job discovers the duplicate and skips. For repos where duplicate triggers are common, this wastes significant runner time.
Consider adding a lightweight check job before build that performs this same existing-tag lookup and gates the build job with if: needs.check.outputs.should_build == true. This would short-circuit the expensive cross-platform compilation when no release is needed.
| EXISTING_TAG=$(gh release list --repo "${{ github.repository }}" --limit 1000 --json tagName,targetCommitish \ | ||
| --jq ".[] | select(.tagName | startswith(\"$BRANCH_NAME/build-\")) | select(.targetCommitish == \"${{ github.sha }}\") | .tagName" \ | ||
| | head -n 1) |
There was a problem hiding this comment.
The --limit 1000 fetches up to 1000 releases to search for an existing tag targeting this SHA. If the repo ever exceeds 1000 releases (unlikely with pruning, but possible if pruning is disabled or during heavy development), this check could silently miss an existing release and create a duplicate.
Consider adding a targeted check via gh release view with the expected tag prefix, or document the 1000-release assumption with a brief comment for future maintainers.
| if branch_exists "$branch"; then | ||
| continue |
There was a problem hiding this comment.
In stale_branches mode, branch_exists returns nonzero for both “this branch is gone” and “GitHub could not be queried”. If git ls-remote has a transient network, auth, or service failure, this path treats every unprotected branch as stale and proceeds to delete its matching releases and tags. The stale cleanup should abort or otherwise distinguish lookup failures from confirmed missing branches before deleting releases.
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:
- Around line 247-260: The gh release list call uses an unsupported field
targetCommitish; update the logic that sets EXISTING_TAG to only request
supported fields (e.g., tagName) and then, for each tagName that starts with
"$BRANCH_NAME/build-", resolve that tag's commit and compare it to "${{
github.sha }}" to decide if a release for this commit already exists;
specifically, keep the EXISTING_TAG variable and BRANCH_NAME usage but replace
the single gh release list --json ... --jq pipeline with a two-step approach:
(1) call gh release list --json tagName (or use gh api to list releases) and
filter tagName values that startwith "$BRANCH_NAME/build-", and (2) for each
candidate tagName use git (git rev-list --max-count=1 <tag> or git rev-parse
refs/tags/<tag>) or gh api (repos/:owner/:repo/git/ref/tags/:tag) to get the
tag's target commit SHA and compare to "${{ github.sha }}" to set EXISTING_TAG
when equal so the rest of the existing early-exit code (release_title,
release_tag, archive_version_part, version, timestamp, should_release) can
remain unchanged.
- Line 282: The workflow uses mutable action tags like
actions/download-artifact@v4 (and many others such as actions/checkout@v4,
actions/checkout@v5, docker/build-push-action@v6, etc. listed by the review),
which must be replaced with immutable 40-character commit SHAs; update each
`uses: owner/repo@v*` entry (including the one referenced as
actions/download-artifact@v4) to `owner/repo@<40-char-commit-sha>` and add the
original tag as a trailing comment (e.g., # v4) so the version is documented
while ensuring the action is pinned to a specific commit SHA.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 250596b7-87c6-4e26-b9cd-a908c4e171d2
📒 Files selected for processing (2)
.github/workflows/build.yml.github/workflows/docker-build.yml
💤 Files with no reviewable changes (1)
- .github/workflows/docker-build.yml
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Greptile Review
- GitHub Check: review-pr
🧰 Additional context used
🪛 zizmor (1.25.2)
.github/workflows/build.yml
[warning] 247-247: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[warning] 248-248: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[error] 282-282: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🔇 Additional comments (6)
.github/workflows/build.yml (6)
247-248: 💤 Low valueRoute GitHub context expansions through
env:to clear thetemplate-injectionfindings.zizmor flags lines 247-248 (
${{ github.repository }},${{ github.sha }}). Real exploitability is low (these can't contain shell metacharacters), but moving them into the stepenv:block is the standard hardening — same pattern already noted for the Layer PR section. If you adopt the local-tag fix above, only${{ github.sha }}remains to be routed throughenv.
692-692: The bare#[0-9]+alternative matches any#123in a commit subject (e.g.fixes#45, `reverts `#100,related to#200), not just the source PR. These leak into `PR_NUMBERS`; issue numbers are filtered out later by the 404 from the `pulls/$PR_NUM` fetch, but unrelated *PR* references still get included in Community Contributions. Tightening to `Merge pull request `#Nand parenthesized(#N)squash subjects avoids the false positives.
697-709: This issues onegh api graphqlcall per commit in the range, on top of the existing per-commit username resolution (Layer B-2) and per-emailsearch/userscalls. On large ranges — especially the parent-branch fallback path — this can generate hundreds of calls and risk secondary rate limiting. Batching theassociatedPullRequestslookups into a single chunked GraphQL request with aliasedobject(oid:)fields keeps the robustness while collapsing N calls into a few.
118-120: LGTM!
281-281: LGTM!Also applies to: 288-288, 351-351, 369-369, 378-378, 507-507, 782-782, 799-799, 862-862, 974-974
711-747: LGTM!
| EXISTING_TAG=$(gh release list --repo "${{ github.repository }}" --limit 1000 --json tagName,targetCommitish \ | ||
| --jq ".[] | select(.tagName | startswith(\"$BRANCH_NAME/build-\")) | select(.targetCommitish == \"${{ github.sha }}\") | .tagName" \ | ||
| | head -n 1) | ||
|
|
||
| if [ -n "$EXISTING_TAG" ]; then | ||
| echo "Release already exists for this commit: $EXISTING_TAG" | ||
| echo "release_title=Build ($BRANCH_NAME): ${EXISTING_TAG#*/build-}" >> $GITHUB_OUTPUT | ||
| echo "release_tag=$EXISTING_TAG" >> $GITHUB_OUTPUT | ||
| echo "archive_version_part=${EXISTING_TAG%%/*}-${EXISTING_TAG#*/build-}" >> $GITHUB_OUTPUT | ||
| echo "version=${EXISTING_TAG#*/build-}" >> $GITHUB_OUTPUT | ||
| echo "timestamp=$(date -u +'%Y-%m-%d %H:%M:%S UTC')" >> $GITHUB_OUTPUT | ||
| echo "should_release=false" >> $GITHUB_OUTPUT | ||
| exit 0 | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What JSON fields does gh release list --json support, and does it include targetCommitish?
💡 Result:
The gh release list command supports the --json flag, which allows you to output specific fields for each release in the repository [1][2]. According to the official GitHub CLI documentation, the following fields are supported [1][2]: createdAt, isDraft, isImmutable, isLatest, isPrerelease, name, publishedAt, tagName The field targetCommitish is not included in the supported fields for gh release list [1][2]. While targetCommitish is available as a JSON field in other commands like gh release view [3], it is currently not an option for the list subcommand [1][2]. If you need to access targetCommitish for releases, you would typically need to use gh release view for individual releases or query the GitHub API directly [4][3].
Citations:
- 1: https://cli.github.com/manual/gh_release_list
- 2: https://man.archlinux.org/man/extra/github-cli/gh-release-list.1.en
- 3: https://cli.github.com/manual/gh_release_view
- 4:
gh release edit --target commit_SHAcannot change the release commit range cli/cli#8669
Fix unsupported targetCommitish in gh release list --json
gh release list --json only supports createdAt, isDraft, isImmutable, isLatest, isPrerelease, name, publishedAt, tagName and does not include targetCommitish, so this command will error and abort the “Generate Build Version” step.
🐛 Proposed fix using local tags
- EXISTING_TAG=$(gh release list --repo "${{ github.repository }}" --limit 1000 --json tagName,targetCommitish \
- --jq ".[] | select(.tagName | startswith(\"$BRANCH_NAME/build-\")) | select(.targetCommitish == \"${{ github.sha }}\") | .tagName" \
- | head -n 1)
+ EXISTING_TAG=$(git tag --list "$BRANCH_NAME/build-*" --points-at "${{ github.sha }}" | head -n 1)🧰 Tools
🪛 zizmor (1.25.2)
[warning] 247-247: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[warning] 248-248: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🤖 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 around lines 247 - 260, The gh release list call
uses an unsupported field targetCommitish; update the logic that sets
EXISTING_TAG to only request supported fields (e.g., tagName) and then, for each
tagName that starts with "$BRANCH_NAME/build-", resolve that tag's commit and
compare it to "${{ github.sha }}" to decide if a release for this commit already
exists; specifically, keep the EXISTING_TAG variable and BRANCH_NAME usage but
replace the single gh release list --json ... --jq pipeline with a two-step
approach: (1) call gh release list --json tagName (or use gh api to list
releases) and filter tagName values that startwith "$BRANCH_NAME/build-", and
(2) for each candidate tagName use git (git rev-list --max-count=1 <tag> or git
rev-parse refs/tags/<tag>) or gh api (repos/:owner/:repo/git/ref/tags/:tag) to
get the tag's target commit SHA and compare to "${{ github.sha }}" to set
EXISTING_TAG when equal so the rest of the existing early-exit code
(release_title, release_tag, archive_version_part, version, timestamp,
should_release) can remain unchanged.
|
|
||
| - name: Download build artifacts | ||
| if: steps.version.outputs.should_release == 'true' | ||
| uses: actions/download-artifact@v4 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# List action references not pinned to a 40-char commit SHA
rg -nP 'uses:\s*\S+@(?![0-9a-f]{40}\b)\S+' -g '*.yml' -g '*.yaml' .githubRepository: Mirrowel/LLM-API-Key-Proxy
Length of output: 1990
🏁 Script executed:
#!/bin/bash
# List action references not pinned to a 40-char commit SHA
rg -nP 'uses:\s*\S+@(?![0-9a-f]{40}\b)\S+' -g '*.yml' -g '*.yaml' .githubRepository: Mirrowel/LLM-API-Key-Proxy
Length of output: 1990
🏁 Script executed:
#!/bin/bash
# List action references not pinned to a 40-char commit SHA
rg -nP 'uses:\s*\S+@(?![0-9a-f]{40}\b)\S+' -g '*.yml' -g '*.yaml' .githubRepository: Mirrowel/LLM-API-Key-Proxy
Length of output: 1990
🏁 Script executed:
#!/bin/bash
# List action references not pinned to a 40-char commit SHA
rg -nP 'uses:\s*\S+@(?![0-9a-f]{40}\b)\S+' -g '*.yml' -g '*.yaml' .githubRepository: Mirrowel/LLM-API-Key-Proxy
Length of output: 1990
🏁 Script executed:
#!/bin/bash
# List action references not pinned to a 40-char commit SHA
rg -nP 'uses:\s*\S+@(?![0-9a-f]{40}\b)\S+' -g '*.yml' -g '*.yaml' .githubRepository: Mirrowel/LLM-API-Key-Proxy
Length of output: 1990
🏁 Script executed:
#!/bin/bash
# List action references not pinned to a 40-char commit SHA
rg -nP 'uses:\s*\S+@(?![0-9a-f]{40}\b)\S+' -g '*.yml' -g '*.yaml' .githubRepository: Mirrowel/LLM-API-Key-Proxy
Length of output: 1990
Pin zizmor-flagged uses: actions to commit SHAs
.github/workflows/build.yml:282 uses actions/download-artifact@v4 (mutable tag), which zizmor reports as an unpinned-uses error. Other unpinned uses: refs found:
.github/workflows/issue-comment.yml:29actions/checkout@v4.github/workflows/issue-comment.yml:56actions/checkout@v4.github/workflows/pr-review.yml:98actions/checkout@v4.github/workflows/pr-review.yml:527actions/checkout@v4.github/workflows/docker-build.yml:38actions/checkout@v5.github/workflows/docker-build.yml:43docker/setup-qemu-action@v3.github/workflows/docker-build.yml:46docker/setup-buildx-action@v3.github/workflows/docker-build.yml:50docker/login-action@v3.github/workflows/docker-build.yml:90docker/metadata-action@v5.github/workflows/docker-build.yml:101docker/build-push-action@v6.github/workflows/docker-build.yml:113actions/attest-build-provenance@v3.github/workflows/docker-build.yml:127actions/delete-package-versions@v5.github/workflows/cleanup.yml:34actions/checkout@v4.github/workflows/compliance-check.yml:217actions/checkout@v4.github/workflows/compliance-check.yml:376actions/checkout@v4.github/workflows/bot-reply.yml:79actions/checkout@v4.github/workflows/bot-reply.yml:536actions/checkout@v4.github/workflows/bot-reply.yml:599actions/checkout@v4.github/workflows/build.yml:134actions/checkout@v5.github/workflows/build.yml:137astral-sh/setup-uv@v4.github/workflows/build.yml:169actions/cache@v4.github/workflows/build.yml:211actions/upload-artifact@v4.github/workflows/build.yml:224actions/checkout@v5.github/workflows/build.yml:282actions/download-artifact@v4.github/actions/bot-setup/action.yml:34actions/create-github-app-token@v1.github/actions/bot-setup/action.yml:159astral-sh/setup-uv@v4
Replace each owner/repo@v* with owner/repo@<40-char-commit-sha> (keep the version as a trailing comment) to satisfy the policy and avoid gating.
🧰 Tools
🪛 zizmor (1.25.2)
[error] 282-282: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 282, The workflow uses mutable action
tags like actions/download-artifact@v4 (and many others such as
actions/checkout@v4, actions/checkout@v5, docker/build-push-action@v6, etc.
listed by the review), which must be replaced with immutable 40-character commit
SHAs; update each `uses: owner/repo@v*` entry (including the one referenced as
actions/download-artifact@v4) to `owner/repo@<40-char-commit-sha>` and add the
original tag as a trailing comment (e.g., # v4) so the version is documented
while ensuring the action is pinned to a specific commit SHA.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/cleanup.yml:
- Around line 112-116: The releases_for_branch function currently interpolates
$branch into the gh --jq string which prevents using jq CLI args and risks
injection; instead have gh release list emit raw JSON (use --jq '.' or no --jq)
and pipe that output into standalone jq, passing the branch via --arg branch
"$branch" and using that variable in the filter (e.g., sort_by(.createdAt) |
reverse | .[] | select(.tagName | startswith($branch + "/")) | .tagName) so the
branch value is bound safely to jq rather than interpolated into the gh --jq
expression.
- Around line 108-110: branch_exists currently treats any non-zero exit from git
ls-remote as “branch absent”; change the branch_exists() implementation to run
git ls-remote --exit-code --heads "$1" capturing its exit code, and only return
“absent” when the exit code equals 2 (no matching refs); for any other non-zero
exit code (transport/auth/fatal errors) propagate/fail (e.g., return a different
non-zero status or exit) so callers know the lookup failed rather than assuming
the branch was deleted.
- Around line 45-84: The run step "Determine cleanup config" currently injects
`${{ github.event.* }}` and `${{ github.event.ref }}` directly into the script
body (populating CLEANUP_MODE, BRANCH_NAME, KEEP_LATEST, DRY_RUN) which risks
command injection; move the event inputs and ref into the step's env: (e.g.,
EVENT_CLEANUP_MODE, EVENT_BRANCH_NAME, EVENT_KEEP_LATEST, EVENT_DRY_RUN) using
`${{ github.event.inputs.* }}` and `${{ github.event.ref }}` there, then inside
the run script assign CLEANUP_MODE, BRANCH_NAME, KEEP_LATEST, DRY_RUN from those
env vars (e.g., CLEANUP_MODE="$EVENT_CLEANUP_MODE") and proceed with the
existing validation (case, integer check, branch required) and outputs; this
keeps runner-safe data handling while leaving the rest of the logic (echo to
GITHUB_OUTPUT, Mode/Branch/Keep latest/Dry run printing) unchanged.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 14d316a0-33d9-496f-ae03-2a1a46bdaec3
📒 Files selected for processing (1)
.github/workflows/cleanup.yml
📜 Review details
🧰 Additional context used
🪛 zizmor (1.25.2)
.github/workflows/cleanup.yml
[warning] 40-43: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 49-49: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[error] 50-50: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[error] 51-51: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[error] 52-52: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[error] 53-53: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[error] 56-56: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[warning] 114-114: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[warning] 119-119: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[warning] 184-184: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[error] 41-41: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🔇 Additional comments (6)
.github/workflows/cleanup.yml (6)
7-29: LGTM!
31-43: LGTM!
96-106: LGTM!
118-130: LGTM!
132-199: LGTM!
201-226: LGTM!
| - name: Determine cleanup config | ||
| id: config | ||
| shell: bash | ||
| run: | | ||
| # Determine branch name based on trigger type | ||
| if [ "${{ github.event_name }}" == "workflow_dispatch" ]; then | ||
| if [ "${{ github.event_name }}" = "workflow_dispatch" ]; then | ||
| CLEANUP_MODE="${{ github.event.inputs.cleanup_mode }}" | ||
| BRANCH_NAME="${{ github.event.inputs.branch_name }}" | ||
| KEEP_LATEST="${{ github.event.inputs.keep_latest }}" | ||
| DRY_RUN="${{ github.event.inputs.dry_run }}" | ||
| echo "🔧 Manual trigger detected" | ||
| else | ||
| CLEANUP_MODE="branch" | ||
| BRANCH_NAME="${{ github.event.ref }}" | ||
| KEEP_LATEST="0" | ||
| DRY_RUN="false" | ||
| echo "🗑️ Branch deletion detected" | ||
| fi | ||
|
|
||
| echo "branch_name=$BRANCH_NAME" >> $GITHUB_OUTPUT | ||
| echo "dry_run=$DRY_RUN" >> $GITHUB_OUTPUT | ||
|
|
||
| echo "Branch: $BRANCH_NAME" | ||
| echo "Dry Run: $DRY_RUN" | ||
|
|
||
| - name: Validate branch is not protected | ||
| shell: bash | ||
| env: | ||
| BRANCH_NAME: ${{ steps.config.outputs.branch_name }} | ||
| run: | | ||
| echo "🔍 Checking if branch '$BRANCH_NAME' is protected..." | ||
|
|
||
| # Convert comma-separated list to array | ||
| IFS=',' read -ra PROTECTED <<< "$PROTECTED_BRANCHES" | ||
|
|
||
| # Check if branch is in protected list | ||
| for protected in "${PROTECTED[@]}"; do | ||
| # Trim whitespace | ||
| protected=$(echo "$protected" | xargs) | ||
| if [ "$BRANCH_NAME" == "$protected" ]; then | ||
| echo "❌ ERROR: Branch '$BRANCH_NAME' is protected and cannot be cleaned up." | ||
| echo "" | ||
| echo "Protected branches: $PROTECTED_BRANCHES" | ||
| echo "" | ||
| echo "If you need to clean up this branch, please remove it from the" | ||
| echo "PROTECTED_BRANCHES environment variable in .github/workflows/cleanup.yml" | ||
| exit 1 | ||
| fi | ||
| done | ||
|
|
||
| echo "✅ Branch '$BRANCH_NAME' is not protected. Proceeding with cleanup." | ||
| case "$CLEANUP_MODE" in | ||
| branch|stale_branches) ;; | ||
| *) echo "Unsupported cleanup mode: $CLEANUP_MODE" >&2; exit 1 ;; | ||
| esac | ||
|
|
||
| if ! printf '%s\n' "$KEEP_LATEST" | grep -Eq '^[0-9]+$'; then | ||
| echo "keep_latest must be a non-negative integer, got: $KEEP_LATEST" >&2 | ||
| exit 1 | ||
| fi | ||
|
|
||
| if [ "$CLEANUP_MODE" = "branch" ] && [ -z "$BRANCH_NAME" ]; then | ||
| echo "branch_name is required for branch mode" >&2 | ||
| exit 1 | ||
| fi | ||
|
|
||
| echo "cleanup_mode=$CLEANUP_MODE" >> "$GITHUB_OUTPUT" | ||
| echo "branch_name=$BRANCH_NAME" >> "$GITHUB_OUTPUT" | ||
| echo "keep_latest=$KEEP_LATEST" >> "$GITHUB_OUTPUT" | ||
| echo "dry_run=$DRY_RUN" >> "$GITHUB_OUTPUT" | ||
|
|
||
| echo "Mode: $CLEANUP_MODE" | ||
| echo "Branch: ${BRANCH_NAME:-'(none)'}" | ||
| echo "Keep latest: $KEEP_LATEST" | ||
| echo "Dry run: $DRY_RUN" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What characters are allowed in git branch and ref names (git check-ref-format), and can a branch name contain backticks, dollar signs, or semicolons?
💡 Result:
Git branch and reference names are governed by the rules enforced by the git check-ref-format command [1][2]. While Git is generally permissive regarding character usage, it strictly prohibits several characters and patterns to ensure system compatibility and prevent conflicts with shell operations or internal Git mechanisms [3][4]. Specifically, a branch name can contain backticks (`) and dollar signs ($), as these are not explicitly forbidden by the Git ref-format rules [1][4]. However, a branch name cannot contain a semicolon (;) because it is a shell control character, and although not explicitly listed in the core git-check-ref-format rules, it is often grouped with other characters that cause issues in command-line environments [3][5]. More importantly, if you use characters like backticks, dollar signs, or semicolons, you must quote or escape the branch name when interacting with it via the command-line interface (CLI) [3][5]. Without quoting (e.g., using 'branch-name'), your shell may interpret these characters as commands or variables (for instance, treating $ as a variable reference or ; as a command separator), leading to errors or unintended side effects [3][5][6]. The prohibited characters and patterns for Git references include [1][2]: - ASCII control characters (bytes lower than \040, or \177 DEL) [1][7] - Space, tilde (~), caret (^), or colon (:) [1][8] - Question mark (?), asterisk (*), or open bracket ([) [1][9] - A backslash () [1][2] - Consecutive dots (..) [1][7] - The sequence @{ [1][2] - The single character @ [1][8] - Starting or ending with a slash (/) or having multiple consecutive slashes (/) [1][9] - Starting with a dot (.) or ending with a dot (.) [1][2] - Ending with the sequence.lock [1][2] Due to potential shell conflicts and general best practices, it is strongly recommended to restrict branch names to alphanumeric characters, hyphens (-), underscores (_), periods (.), and forward slashes (/) [3][5].
Citations:
- 1: https://git-scm.com/docs/git-check-ref-format
- 2: https://www.kernel.org/pub/software/scm/git/docs/git-check-ref-format.html
- 3: https://docs.github.com/en/get-started/using-git/dealing-with-special-characters-in-branch-and-tag-names
- 4: https://www.youtube.com/watch?v=n0w4oL4bJo8
- 5: https://github.com/github/docs/blob/main/content/get-started/using-git/dealing-with-special-characters-in-branch-and-tag-names.md
- 6: https://stackoverflow.com/questions/3651860/which-characters-are-illegal-within-a-branch-name
- 7: https://man.archlinux.org/man/git-check-ref-format.1.en.txt
- 8: https://kernel.googlesource.com/pub/scm/git/git/+/HEAD/Documentation/git-check-ref-format.adoc
- 9: https://mirrors.edge.kernel.org/pub/software/scm/git/docs/git-check-ref-format.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".github/workflows/cleanup.yml"
echo "=== File exists? ==="
ls -la "$FILE"
echo "=== First 220 lines (to include inputs + referenced lines) ==="
nl -ba "$FILE" | sed -n '1,220p'
echo "=== Search for github.event.inputs.* usage ==="
rg -n "github\.event\.inputs\." "$FILE" || true
echo "=== Search for github\.repository usage ==="
rg -n "github\.repository" "$FILE" || trueRepository: Mirrowel/LLM-API-Key-Proxy
Length of output: 295
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".github/workflows/cleanup.yml"
echo "=== Total lines ==="
wc -l "$FILE"
echo "=== First 240 lines with line numbers ==="
awk '{printf "%5d %s\n", NR, $0}' "$FILE" | sed -n '1,240p'
echo "=== Search for github.event.inputs.* usage ==="
rg -n "github\.event\.inputs\." "$FILE" || true
echo "=== Search for github.event.ref usage ==="
rg -n "github\.event\.ref" "$FILE" || true
echo "=== Search for github.repository usage ==="
rg -n "github\.repository" "$FILE" || true
echo "=== Search for BRANCH_NAME usage (to see quoting later) ==="
rg -n "\\bBRANCH_NAME\\b" "$FILE" || true
echo "=== Search for git commands involving BRANCH_NAME / CLEANUP_MODE ==="
rg -n "git .*BRANCH_NAME|BRANCH_NAME.*git|git .*CLEANUP_MODE|CLEANUP_MODE.*git" "$FILE" || trueRepository: Mirrowel/LLM-API-Key-Proxy
Length of output: 11861
Fix command-injection risk from ${{ github.event.* }} interpolation in run
In .github/workflows/cleanup.yml, step Determine cleanup config assigns BRANCH_NAME/KEEP_LATEST/DRY_RUN directly from ${{ github.event.inputs.* }} and ${{ github.event.ref }} into the run script body (e.g., lines 49-59). Git ref/branch names can contain backticks (`) and $, which bash will treat as command/parameter expansions when they’re interpolated into the script text—allowing attacker-controlled command execution in a job with permissions: contents: write. Move these event values into env: and reference them as variables inside the script so the runner treats them as data, not shell syntax.
🔒 Proposed fix: pass event data via env
- name: Determine cleanup config
id: config
shell: bash
+ env:
+ EVENT_NAME: ${{ github.event_name }}
+ INPUT_CLEANUP_MODE: ${{ github.event.inputs.cleanup_mode }}
+ INPUT_BRANCH_NAME: ${{ github.event.inputs.branch_name }}
+ INPUT_KEEP_LATEST: ${{ github.event.inputs.keep_latest }}
+ INPUT_DRY_RUN: ${{ github.event.inputs.dry_run }}
+ EVENT_REF: ${{ github.event.ref }}
run: |
- if [ "${{ github.event_name }}" = "workflow_dispatch" ]; then
- CLEANUP_MODE="${{ github.event.inputs.cleanup_mode }}"
- BRANCH_NAME="${{ github.event.inputs.branch_name }}"
- KEEP_LATEST="${{ github.event.inputs.keep_latest }}"
- DRY_RUN="${{ github.event.inputs.dry_run }}"
+ if [ "$EVENT_NAME" = "workflow_dispatch" ]; then
+ CLEANUP_MODE="$INPUT_CLEANUP_MODE"
+ BRANCH_NAME="$INPUT_BRANCH_NAME"
+ KEEP_LATEST="$INPUT_KEEP_LATEST"
+ DRY_RUN="$INPUT_DRY_RUN"
else
CLEANUP_MODE="branch"
- BRANCH_NAME="${{ github.event.ref }}"
+ BRANCH_NAME="$EVENT_REF"
KEEP_LATEST="0"
DRY_RUN="false"
fiZizmor warnings for ${{ github.repository }} usage in the later gh release list/delete calls are false positives (it’s not attacker-controlled).
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 49-49: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[error] 50-50: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[error] 51-51: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[error] 52-52: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[error] 53-53: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[error] 56-56: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🤖 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/cleanup.yml around lines 45 - 84, The run step "Determine
cleanup config" currently injects `${{ github.event.* }}` and `${{
github.event.ref }}` directly into the script body (populating CLEANUP_MODE,
BRANCH_NAME, KEEP_LATEST, DRY_RUN) which risks command injection; move the event
inputs and ref into the step's env: (e.g., EVENT_CLEANUP_MODE,
EVENT_BRANCH_NAME, EVENT_KEEP_LATEST, EVENT_DRY_RUN) using `${{
github.event.inputs.* }}` and `${{ github.event.ref }}` there, then inside the
run script assign CLEANUP_MODE, BRANCH_NAME, KEEP_LATEST, DRY_RUN from those env
vars (e.g., CLEANUP_MODE="$EVENT_CLEANUP_MODE") and proceed with the existing
validation (case, integer check, branch required) and outputs; this keeps
runner-safe data handling while leaving the rest of the logic (echo to
GITHUB_OUTPUT, Mode/Branch/Keep latest/Dry run printing) unchanged.
| branch_exists() { | ||
| git ls-remote --exit-code --heads origin "$1" >/dev/null 2>&1 | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What exit status codes does "git ls-remote --exit-code" return when no matching refs are found versus when the command fails due to a network or transport error?
💡 Result:
When using git ls-remote --exit-code: 1) No matching refs found: exits with status 2 [1][2]. 2) Network/transport error (e.g., cannot connect / transport failure): git ls-remote does not map those failures to --exit-code’s “no matching refs” status; instead it exits with a different non-zero status used for fatal errors. Git’s git-ls-remote documentation explicitly only defines the --exit-code behavior for the “no matching refs” case (status 2) and says the usual “talked to remote successfully” exit is 0; it does not document an alternate status number for network/transport errors under --exit-code [1][2]. Practical implication: you can reliably treat exit code 2 as “remote reachable, but pattern matched no refs.” For any other non-zero exit, handle it as an error condition such as authentication failure, repository not found, or network/transport failure [2].
Citations:
- 1: https://git-scm.com/docs/git-ls-remote
- 2: https://man7.org/linux/man-pages/man1/git-ls-remote.1.html
Fix branch_exists to only treat git ls-remote --exit-code exit code 2 as “branch absent”
git ls-remote --exit-code exits 2 for “no matching refs”; other non-zero exits cover fatal/transport/auth failures. The current implementation treats any non-zero as “branch deleted”, which can cause stale branch cleanup to delete releases during transient lookup failures.
🛡️ Proposed fail-safe lookup
branch_exists() {
- git ls-remote --exit-code --heads origin "$1" >/dev/null 2>&1
+ local out status
+ out="$(git ls-remote --exit-code --heads origin "$1" 2>/dev/null)"
+ status=$?
+ if [ "$status" -eq 2 ]; then
+ # No matching refs
+ return 1
+ elif [ "$status" -ne 0 ]; then
+ # Transient/network/auth/etc error: assume the branch exists
+ echo "warning: 'git ls-remote' failed for '$1'; assuming branch exists" >&2
+ return 0
+ fi
+ [ -n "$out" ]
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| branch_exists() { | |
| git ls-remote --exit-code --heads origin "$1" >/dev/null 2>&1 | |
| } | |
| branch_exists() { | |
| local out status | |
| out="$(git ls-remote --exit-code --heads origin "$1" 2>/dev/null)" | |
| status=$? | |
| if [ "$status" -eq 2 ]; then | |
| # No matching refs | |
| return 1 | |
| elif [ "$status" -ne 0 ]; then | |
| # Transient/network/auth/etc error: assume the branch exists | |
| echo "warning: 'git ls-remote' failed for '$1'; assuming branch exists" >&2 | |
| return 0 | |
| fi | |
| [ -n "$out" ] | |
| } |
🤖 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/cleanup.yml around lines 108 - 110, branch_exists
currently treats any non-zero exit from git ls-remote as “branch absent”; change
the branch_exists() implementation to run git ls-remote --exit-code --heads "$1"
capturing its exit code, and only return “absent” when the exit code equals 2
(no matching refs); for any other non-zero exit code (transport/auth/fatal
errors) propagate/fail (e.g., return a different non-zero status or exit) so
callers know the lookup failed rather than assuming the branch was deleted.
| releases_for_branch() { | ||
| local branch="$1" | ||
| gh release list --repo "${{ github.repository }}" --limit 1000 --json tagName,createdAt \ | ||
| --jq "sort_by(.createdAt) | reverse | .[] | select(.tagName | startswith(\"$branch/\")) | .tagName" | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does the GitHub CLI gh --jqflag support passing jq named arguments such as--arg, or must filtering with variables be done by piping output to standalone jq?
💡 Result:
No: gh --jq does not support passing through jq named arguments like --arg (e.g., you can’t do gh ... --jq '...' --arg foo bar). The --jq flag only accepts a single jq program/expression string, and gh does not expose a mechanism to pass additional jq CLI arguments/variables to that program; if you need jq variables, you must do it via piping to standalone jq (or by composing values into the jq expression itself). Evidence: - gh documents --jq as “Query to select values from the response using jq syntax” and shows usage as --jq '<jq expression>' (with no mention of forwarding jq CLI arguments such as --arg). [1] - The gh help formatting docs state that formatting uses --json to convert to JSON and then --jq “requires a string argument in jq query syntax” to “only print those JSON values which match the query.” [2] - In the cli/cli source, the --jq flag is stored as a string “expression” (and --jq cannot be used without also specifying --json), indicating gh treats it purely as the jq filter expression rather than something that can accept jq’s own --arg/etc. arguments. [3] - A related GitHub CLI issue explicitly describes a workaround where users shell out to standalone jq specifically “so that I could pass in an arg, rather than being able to use gh's --jq for it,” implying --jq alone can’t take --arg-style bindings. [4] Example workaround (pipe to standalone jq to use --arg): - gh ... --json <fields> | jq --arg name "$NAME" '... use $name ...' So: if you need jq named variables, use standalone jq after gh (or inline everything into the --jq expression); you can’t rely on gh --jq to accept/forward --arg.
Citations:
- 1: https://cli.github.com/manual/gh_api
- 2: https://cli.github.com/manual/gh_help_formatting
- 3: https://github.com/cli/cli/blob/b07f955c23fb54c400b169d39255569e240b324e/pkg/cmdutil/json_flags.go
- 4: Allow passing additional arguments through to
--jqscript, just like we can withjq --argcli/cli#10263
Bind the branch into standalone jq via --arg (don’t interpolate into gh --jq).
gh --jq only accepts a single jq expression string and can’t take jq CLI named arguments like --arg, so $branch must be passed to standalone jq to avoid jq-filter breakage/injection.
♻️ Proposed refactor
releases_for_branch() {
local branch="$1"
gh release list --repo "${{ github.repository }}" --limit 1000 --json tagName,createdAt \
- --jq "sort_by(.createdAt) | reverse | .[] | select(.tagName | startswith(\"$branch/\")) | .tagName"
+ | jq -r --arg b "$branch" 'sort_by(.createdAt) | reverse | .[] | select(.tagName | startswith($b + "/")) | .tagName'
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| releases_for_branch() { | |
| local branch="$1" | |
| gh release list --repo "${{ github.repository }}" --limit 1000 --json tagName,createdAt \ | |
| --jq "sort_by(.createdAt) | reverse | .[] | select(.tagName | startswith(\"$branch/\")) | .tagName" | |
| } | |
| releases_for_branch() { | |
| local branch="$1" | |
| gh release list --repo "${{ github.repository }}" --limit 1000 --json tagName,createdAt \ | |
| | jq -r --arg b "$branch" 'sort_by(.createdAt) | reverse | .[] | select(.tagName | startswith($b + "/")) | .tagName' | |
| } |
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 114-114: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🤖 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/cleanup.yml around lines 112 - 116, The
releases_for_branch function currently interpolates $branch into the gh --jq
string which prevents using jq CLI args and risks injection; instead have gh
release list emit raw JSON (use --jq '.' or no --jq) and pipe that output into
standalone jq, passing the branch via --arg branch "$branch" and using that
variable in the filter (e.g., sort_by(.createdAt) | reverse | .[] |
select(.tagName | startswith($branch + "/")) | .tagName) so the branch value is
bound safely to jq rather than interpolated into the gh --jq expression.
1 participant
Summary
Validation
Notes