refactor: harden and upgrade bot-verified-commits workflow (#1482)#1494
Conversation
aceppaluni
left a comment
aceppaluni
left a comment
There was a problem hiding this comment.
@cheese-cakee , this is looking super!
Please be sure to add a changelog entry. Thank you!!
|
ready for review ! @aceppaluni |
WalkthroughAdds a new Node.js script and updates the GitHub Actions workflow to verify PR commit GPG signatures. The script paginates PR commits and comments, sanitizes inputs, detects existing bot comments, and posts a single verification failure comment when unsigned commits are found; supports dry-run and manual dispatch. Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as Action Runner
participant Script as .github/scripts/bot-verified-commits.js
participant GH as GitHub API
participant PR as Pull Request
Runner->>Script: invoke main({ github, context })
Script->>GH: validate repo & PR number
Script->>GH: list PR commits (paginated, bounded by CONFIG.MAX_PAGES)
GH-->>Script: commits + verification metadata
Script->>Script: compute unverified commits list
alt unverified commits > 0
Script->>GH: list PR comments (paginated)
GH-->>Script: comments
Script->>Script: detect existing bot comment (marker / bot logins)
alt no existing bot comment
Script->>GH: post verification failure comment (links, commits URL, up to 10 commits)
GH-->>Script: comment created
end
Script-->>Runner: set outputs (unverified_count), exit non-zero (unless DRY_RUN)
else all commits verified
Script-->>Runner: set outputs (unverified_count=0), exit success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/bot-verified-commits.yml (1)
13-15: Add required dry-run mode for a state-mutating workflow.This workflow posts comments to pull requests, so it must support
workflow_dispatchwithdry_run(default true), log dry-run status, skip comment creation, and exit successfully when dry-run is active. Pass the flag into the script so it can short-circuit posting. As per coding guidelines, workflows that mutate GitHub state must have explicit dry-run support.🛠️ Suggested fix
on: pull_request_target: types: [opened, synchronize] + workflow_dispatch: + inputs: + dry_run: + description: "Run without commenting or failing the workflow" + required: false + default: "true" ... env: + DRY_RUN: ${{ github.event.inputs.dry_run || 'false' }} ... - name: Verify PR commits uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1 id: verify with: github-token: ${{ secrets.GITHUB_TOKEN }} result-encoding: json script: | const script = require('./.github/scripts/bot-verified-commits.js'); const result = await script({ github, context }); core.setOutput('success', result.success); core.setOutput('unverified_count', result.unverifiedCount); return result; + env: + DRY_RUN: ${{ env.DRY_RUN }} ... - - name: Fail if unverified commits found - if: steps.verify.outputs.success != 'true' + - name: Fail if unverified commits found + if: steps.verify.outputs.success != 'true' && env.DRY_RUN != 'true' run: | + echo "DRY_RUN=${DRY_RUN}" echo "❌ Pull request has unverified commits." echo "Please sign your commits with GPG and DCO." echo "See: ${{ env.SIGNING_GUIDE_URL }}" exit 1
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/bot-verified-commits.yml (1)
13-48: Add explicit dry-run mode for comment mutations.This workflow mutates GitHub state but lacks a
workflow_dispatchdry-run input and visible dry-run logging. Add a dry-run input (default true), plumb it into env, and ensure the script skips comment creation when dry-run is active. As per coding guidelines.🛠️ Suggested fix
on: pull_request_target: types: [opened, synchronize] + workflow_dispatch: + inputs: + dry_run: + description: "Run without posting comments" + required: false + default: "true"env: + DRY_RUN: ${{ github.event_name == 'workflow_dispatch' && inputs.dry_run || 'false' }}+ - name: Dry-run notice + if: env.DRY_RUN == 'true' + run: echo "Dry-run mode enabled; no comments will be posted."Also applies to: 66-79
♻️ Duplicate comments (3)
.github/scripts/bot-verified-commits.js (2)
33-59: Wrap helper API calls in contextual try/catch blocks.Failures in
listCommitsorlistCommentscurrently bubble tomainwithout clear context. Wrap each helper’s async flow with try/catch and log owner/repo/prNumber before rethrowing. As per coding guidelines.🛠️ Suggested fix
async function getCommitVerificationStatus(github, owner, repo, prNumber) { - console.log(`[${CONFIG.BOT_NAME}] Fetching commits for PR #${prNumber}...`); - - const commits = []; - let page = 0; - for await (const response of github.paginate.iterator( - github.rest.pulls.listCommits, - { owner, repo, pull_number: prNumber, per_page: 100 } - )) { - commits.push(...response.data); - if (++page >= CONFIG.MAX_PAGES) { - console.log(`[${CONFIG.BOT_NAME}] Reached MAX_PAGES (${CONFIG.MAX_PAGES}) limit`); - break; - } - } - - const unverifiedCommits = commits.filter( - commit => commit.commit?.verification?.verified !== true - ); - - console.log(`[${CONFIG.BOT_NAME}] Found ${commits.length} total, ${unverifiedCommits.length} unverified`); - - return { - total: commits.length, - unverified: unverifiedCommits.length, - }; + console.log(`[${CONFIG.BOT_NAME}] Fetching commits for PR #${prNumber}...`); + try { + const commits = []; + let page = 0; + for await (const response of github.paginate.iterator( + github.rest.pulls.listCommits, + { owner, repo, pull_number: prNumber, per_page: 100 } + )) { + commits.push(...response.data); + if (++page >= CONFIG.MAX_PAGES) { + console.log(`[${CONFIG.BOT_NAME}] Reached MAX_PAGES (${CONFIG.MAX_PAGES}) limit`); + break; + } + } + + const unverifiedCommits = commits.filter( + commit => commit.commit?.verification?.verified !== true + ); + + console.log(`[${CONFIG.BOT_NAME}] Found ${commits.length} total, ${unverifiedCommits.length} unverified`); + + return { + total: commits.length, + unverified: unverifiedCommits.length, + }; + } catch (error) { + console.error(`[${CONFIG.BOT_NAME}] Failed to list commits`, { + owner, + repo, + prNumber, + status: error?.status, + message: error?.message, + }); + throw error; + } }async function hasExistingBotComment(github, owner, repo, prNumber) { - console.log(`[${CONFIG.BOT_NAME}] Checking for existing bot comments...`); + console.log(`[${CONFIG.BOT_NAME}] Checking for existing bot comments...`); + try { // Support both with and without [bot] suffix for GitHub Actions bot account const botLogins = new Set([ CONFIG.BOT_LOGIN, `${CONFIG.BOT_LOGIN}[bot]`, 'github-actions[bot]', ]); let page = 0; for await (const response of github.paginate.iterator( github.rest.issues.listComments, { owner, repo, issue_number: prNumber, per_page: 100 } )) { // Early return if marker found if (response.data.some(comment => botLogins.has(comment.user?.login) && typeof comment.body === 'string' && comment.body.includes(CONFIG.COMMENT_MARKER) )) { console.log(`[${CONFIG.BOT_NAME}] Existing bot comment: true`); return true; } if (++page >= CONFIG.MAX_PAGES) { console.log(`[${CONFIG.BOT_NAME}] Reached MAX_PAGES (${CONFIG.MAX_PAGES}) limit`); break; } } console.log(`[${CONFIG.BOT_NAME}] Existing bot comment: false`); return false; + } catch (error) { + console.error(`[${CONFIG.BOT_NAME}] Failed to list comments`, { + owner, + repo, + prNumber, + status: error?.status, + message: error?.message, + }); + throw error; + } }Also applies to: 63-95
38-47: Silent truncation of pagination without explicit error handling violates fail-closed principle.When MAX_PAGES limit is reached, the functions return incomplete results without signaling the limitation:
getCommitVerificationStatusmay report incomplete unverified commit counts, risking false passes if unverified commits exist beyond page 5hasExistingBotCommentmay return false when a marker comment actually exists beyond page 5, risking duplicate commentsThe pagination breaks silently with only a console log, violating the coding guideline that errors must include contextual metadata. Per the guidelines, scripts must fail closed when limits are encountered.
🛠️ Suggested fixes
For commits (lines 43-46):
- if (++page >= CONFIG.MAX_PAGES) { - console.log(`[${CONFIG.BOT_NAME}] Reached MAX_PAGES (${CONFIG.MAX_PAGES}) limit`); - break; - } + if (++page >= CONFIG.MAX_PAGES) { + throw new Error(`Exceeded MAX_PAGES (${CONFIG.MAX_PAGES}) while listing commits`); + }For comments (lines 87-90):
- if (++page >= CONFIG.MAX_PAGES) { - console.log(`[${CONFIG.BOT_NAME}] Reached MAX_PAGES (${CONFIG.MAX_PAGES}) limit`); - break; - } + if (++page >= CONFIG.MAX_PAGES) { + console.warn(`[${CONFIG.BOT_NAME}] Reached MAX_PAGES (${CONFIG.MAX_PAGES}) while listing comments; assuming comment exists to avoid duplicates`); + return true; + }.github/workflows/bot-verified-commits.yml (1)
85-87: Remove DCO mention from the failure message.The workflow only checks commit verification; it doesn’t validate DCO sign-offs. Either implement DCO checks or remove DCO from the user-facing message for accuracy. As per coding guidelines.
🛠️ Suggested fix
- echo "Please sign your commits with GPG and DCO." + echo "Please sign your commits with GPG."
|
Hi, this is MergeConflictBot. Please resolve these conflicts locally and push the changes. To assist you, please read: Thank you for contributing! |
|
Thanks for the review @exploreriii! A few responses: Re: Listing unverified commit SHAs in the comment
We could add the short SHAs to the comment. However, I'm wondering if it might make the comment quite long for PRs with many unverified commits. The 'Commits Tab' link already shows verification status per commit. should we list them, or is the link sufficient? Re: Logging duplicate skip reason
Yes, Line 170 in the main function logs: [VerificationBot] Bot already commented. Skipping duplicate Re: Adding DCO verification
This could be useful but the original issue (#1482) emphasized keeping the logic similar to avoid breaking things so I kept it GPG only. But DCO integration could be a great enhancement. Should I create a separate issue for that, or include it in this PR itself? |
exploreriii
left a comment
exploreriii
left a comment
There was a problem hiding this comment.
Okay, LGTM... regarding the DCO check which i think would be an improvement, could you help to open up that intermediate issue?
So we would refactor the verified commit bot to be a signing bot
It will:
- post if commits are not verified and/or DCO signed
- specify which commits are not verified and/or DCO signed
- state easiest approach is probably a soft revert, and resign with -S -s -m
- add helpful links to solve the issue
Some creativity required to figure out what is the best final comment
Please rebase 👍
cheese-cakee
force-pushed
the
harden-verified-commits-workflow
branch
2 times, most recently
from
726e911 to
3ba7fe3
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/bot-verified-commits.yml (1)
13-83: Add explicit dry‑run mode for this state‑mutating workflow.This workflow posts PR comments but doesn’t provide a
workflow_dispatchdry‑run input or aDRY_RUNguard. Add a dry‑run input (defaulttrue), log dry‑run status, skip comment posting when enabled, and ensure the fail step is gated so the job exits successfully.As per coding guidelines, state‑mutating workflows must support an explicit dry‑run mode.🛠️ Proposed fix (workflow + script guard)
on: pull_request_target: types: [opened, synchronize] + workflow_dispatch: + inputs: + dry_run: + description: "Run without posting comments" + required: false + default: "true" ... env: # Bot identity BOT_NAME: "VerificationBot" BOT_LOGIN: "github-actions" + DRY_RUN: ${{ github.event_name == 'workflow_dispatch' && inputs.dry_run || 'false' }} ... steps: + - name: Log dry-run status + run: echo "Dry run: ${{ env.DRY_RUN }}" ... - name: Fail if unverified commits found - if: steps.verify.outputs.success != 'true' + if: steps.verify.outputs.success != 'true' && env.DRY_RUN != 'true' run: |+ if (process.env.DRY_RUN === 'true') { + console.log(`[${CONFIG.BOT_NAME}] DRY_RUN enabled; skipping comment.`); + return true; + }
cheese-cakee
force-pushed
the
harden-verified-commits-workflow
branch
from
0289b85 to
9fecab2
Compare
|
Hi @cheese-cakee is this rebaed, tested and ready to review again please? |
|
@cheese-cakee This is looking good! Please be sure to address merge conflicts. If you need help, please let us know. We are happy to assist! |
cheese-cakee
force-pushed
the
harden-verified-commits-workflow
branch
from
7387f81 to
8826468
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #1494 +/- ##
=======================================
Coverage 92.89% 92.89%
=======================================
Files 140 140
Lines 8765 8765
=======================================
Hits 8142 8142
Misses 623 623 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/bot-verified-commits.yml (1)
11-11: Consider aligning workflow name with script filename.The workflow name
PythonBot - Verify PR Commitsdoesn't match the script filenamebot-verified-commits.js. Per coding guidelines, workflow names should align with their primary script for maintainability.♻️ Suggested name
-name: PythonBot - Verify PR Commits +name: Bot - Verified Commits
|
ready for review @exploreriii , @aceppaluni |
|
Sorry @cheese-cakee please rebase again |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/bot-verified-commits.yml (1)
16-30:workflow_dispatchlacks PR context, breaking concurrency isolation.When triggered via
workflow_dispatch,github.event.pull_request.numberis undefined, causing the concurrency group to resolve toverify-commits-(empty suffix). All manual runs will share the same concurrency group and cancel each other.The script handles missing PR context gracefully by returning early, but the concurrency collision remains. To make
workflow_dispatchfunctional, add a requiredpr_numberinput and use it in the concurrency group.🛠️ Proposed fix
workflow_dispatch: inputs: dry_run: description: "Run without posting comments" required: false default: "true" + pr_number: + description: "PR number to verify (required for manual runs)" + required: true concurrency: - group: "verify-commits-${{ github.event.pull_request.number }}" + group: "verify-commits-${{ github.event_name == 'workflow_dispatch' && inputs.pr_number || github.event.pull_request.number }}" cancel-in-progress: trueThen update the env block to pass PR_NUMBER and modify the script to use it:
+ # PR number (from input for workflow_dispatch, from event for PR triggers) + PR_NUMBER: ${{ github.event_name == 'workflow_dispatch' && inputs.pr_number || github.event.pull_request.number }}
♻️ Duplicate comments (2)
CHANGELOG.md (1)
207-212: Resolve unresolved merge conflict markers.The changelog contains Git conflict markers (
<<<<<<< HEAD,=======,>>>>>>>) that must be removed. This makes the file invalid and can break release tooling or changelog parsing.🐛 Proposed fix
Resolve the conflict by keeping the appropriate entries (likely both):
### Fixed -<<<<<<< HEAD + - Aligned token freeze example filename references and improved error handling by catching broader exceptions with clearer messages. (`#1412`) - Fixed jq syntax in bot-office-hours.sh (`#1502`) -======= - ->>>>>>> c98d204 (chore: add changelog entry for `#1482`) - Prevented linkbot from commenting on or auto-closing bot-authored pull requests. (`#1516`).github/workflows/bot-verified-commits.yml (1)
70-96: Add richer context to logs for operability.The current logs only show dry-run status. Including repository, PR number, and actor improves audit trails and incident triage. As per coding guidelines, logs should include repository, PR number, actor, dry-run status, and decisions.
🪵 Suggested log enrichment
- name: Log dry-run status run: | + echo "Repository: ${{ github.repository }}" + echo "PR: ${{ github.event.pull_request.number }}" + echo "Actor: ${{ github.actor }}" echo "Dry run mode: ${{ env.DRY_RUN }}" # ... verify step ... - name: Fail if unverified commits found if: steps.verify.outputs.success != 'true' && env.DRY_RUN != 'true' run: | echo "❌ Pull request has unverified commits." + echo "Unverified count: ${{ steps.verify.outputs.unverified_count }}" echo "Please sign your commits with GPG." echo "See: ${{ env.SIGNING_GUIDE_URL }}" exit 1
|
@cheese-cakee sorry I missed this, I think I had previously approved it but I didn't' see it in the pipeline as was in draft |
…ger#1482) - Move inline bash logic to modular JavaScript file (.github/scripts/bot-verified-commits.js) - Add input sanitization and validation - Make all config values configurable via environment variables - Pin all actions to SHA for security - Add named exports for testing and reuse Signed-off-by: cheese-cakee <farzanaman99@gmail.com>
- Use Unicode property escape \\p{Cc} instead of hex escapes for Biome lint
- Add bounded pagination with MAX_PAGES to prevent unbounded API scans
- Support bot login with [bot] suffix for reliable comment detection
- Add try/catch error handling to postVerificationComment
- Add repo context validation with sanitization
- Remove incorrect DCO claim from workflow header
Signed-off-by: cheese-cakee <farzanaman99@gmail.com>
Signed-off-by: cheese-cakee <farzanaman99@gmail.com>
Signed-off-by: cheese-cakee <farzanaman99@gmail.com>
- Fix CHANGELOG.md merge conflict markers - Fix truncation count mismatch in comment (shows 'at least N' when truncated) Signed-off-by: cheese-cakee <farzanaman99@gmail.com>
Signed-off-by: cheese-cakee <farzanaman99@gmail.com>
Signed-off-by: cheese-cakee <farzanaman99@gmail.com>
Signed-off-by: cheese-cakee <farzanaman99@gmail.com>
Signed-off-by: cheese-cakee <farzanaman99@gmail.com>
Signed-off-by: cheese-cakee <farzanaman99@gmail.com>
Signed-off-by: cheese-cakee <farzanaman99@gmail.com>
cheese-cakee
force-pushed
the
harden-verified-commits-workflow
branch
from
2f542c4 to
adc49e5
Compare
cheese-cakee
force-pushed
the
harden-verified-commits-workflow
branch
from
adc49e5 to
f1fb865
Compare
- Update actions/checkout to v4.3.1 (from v4.2.2) - Add pr_number input for workflow_dispatch support - Fix concurrency group to handle manual triggers - Enrich logs with repository, PR number, actor context - Update JS to prefer PR_NUMBER env var for workflow_dispatch Signed-off-by: cheese-cakee <farzanaman99@gmail.com>
cheese-cakee
force-pushed
the
harden-verified-commits-workflow
branch
from
f1fb865 to
7ab3979
Compare
i think its ready for review now |
|
Hello, this is the OfficeHourBot. This is a reminder that the Hiero Python SDK Office Hours are scheduled in approximately 4 hours (14:00 UTC). This session provides an opportunity to ask questions regarding this Pull Request. Details:
Disclaimer: This is an automated reminder. Please verify the schedule here for any changes. From, |
prajeeta15
left a comment
prajeeta15
left a comment
There was a problem hiding this comment.
@cheese-cakee please just address the merge conflict, everything else looks great :)
aceppaluni
left a comment
aceppaluni
left a comment
There was a problem hiding this comment.
@cheese-cakee This is great work!
Once rebased, should be good to go!
Thank you for your contribution!!
Signed-off-by: exploreriii <133720349+exploreriii@users.noreply.github.com>
|
LGTM, thank you @cheese-cakee ! |
hello. sorry! i somehow missed this notification. |
4 participants
Description
This PR refactors the
bot-verified-commits.ymlworkflow from inline bash to a modular JavaScript implementation, as requested in #1482.Changes Made
New file:
.github/scripts/bot-verified-commits.js- Modular JavaScript implementation with:sanitizeString,validatePRNumber)Updated:
.github/workflows/bot-verified-commits.yml:env:block for easy customizationactions/github-scriptto call the JS moduleWhat's Preserved
pull_request_targettrigger (opened, synchronize)Testing
Tested on fork: cheese-cakee#8
Fixes #1482