feat: Add PR Dashboard#2464
Conversation
Signed-off-by: aceppaluni <aceppaluni@gmail.com>
github-actions
Bot
added
approved
aceppaluni
added
reviewer: maintainer
WalkthroughAdds a configuration-driven PR dashboard that validates DCO, GPG, mergeability, and issue assignment, posts unified comments, manages labels and assignees, and runs through PR-open and PR-update GitHub Actions workflows. ChangesPR Dashboard Automation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant GitHub
participant Workflow
participant PRHandler
participant GitHubAPI
participant PRDashboard
GitHub->>Workflow: emit pull_request event
Workflow->>PRHandler: invoke handler with github and context
PRHandler->>GitHubAPI: validate context and fetch PR data
PRHandler->>GitHubAPI: run PR checks and resolve linked issues
PRHandler->>PRDashboard: build dashboard result
PRHandler->>GitHubAPI: update comment, assignee, and status label
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
📋 Issue PlannerLet us write the prompt for your AI agent so you can ship faster (with fewer bugs). View plan for ticket: ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3b8ebac8-68c2-49c5-b398-ccd35797cd52
📒 Files selected for processing (13)
.github/hiero-automation.json.github/scripts/bot-on-pr-open.js.github/scripts/bot-on-pr-update.js.github/scripts/helpers/api.js.github/scripts/helpers/checks.js.github/scripts/helpers/comments.js.github/scripts/helpers/config-loader.js.github/scripts/helpers/constants.js.github/scripts/helpers/index.js.github/scripts/helpers/logger.js.github/scripts/helpers/validation.js.github/workflows/on-pr-update.yml.github/workflows/on-pr.yml
| function checkGPG(commits) { | ||
| const failures = []; | ||
| for (const c of commits) { | ||
| const shortSha = (c.sha || '').slice(0, 7); | ||
| const message = c.commit?.message || ''; | ||
| const firstLine = message.split('\n')[0] || '(no message)'; | ||
| if (!hasVerifiedGPGSignature(c)) { | ||
| failures.push({ sha: shortSha, message: firstLine }); | ||
| } | ||
| } | ||
| getLogger().log(`GPG check: ${commits.length - failures.length}/${commits.length} passed`); | ||
| return { passed: failures.length === 0, failures }; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
GPG check doesn't exempt merge commits, unlike DCO check.
checkDCO explicitly skips merge commits via isMergeCommit (Line 50-53), but checkGPG runs hasVerifiedGPGSignature on every commit including merge commits. Auto-generated merge commits (e.g. from merging base into a feature branch) are typically not GPG-signed by the contributor, so this will produce false GPG failures for PRs containing such commits.
🐛 Proposed fix
function checkGPG(commits) {
const failures = [];
for (const c of commits) {
+ if (isMergeCommit(c)) continue;
const shortSha = (c.sha || '').slice(0, 7);
const message = c.commit?.message || '';
const firstLine = message.split('\n')[0] || '(no message)';
if (!hasVerifiedGPGSignature(c)) {
failures.push({ sha: shortSha, message: firstLine });
}
}📝 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.
| function checkGPG(commits) { | |
| const failures = []; | |
| for (const c of commits) { | |
| const shortSha = (c.sha || '').slice(0, 7); | |
| const message = c.commit?.message || ''; | |
| const firstLine = message.split('\n')[0] || '(no message)'; | |
| if (!hasVerifiedGPGSignature(c)) { | |
| failures.push({ sha: shortSha, message: firstLine }); | |
| } | |
| } | |
| getLogger().log(`GPG check: ${commits.length - failures.length}/${commits.length} passed`); | |
| return { passed: failures.length === 0, failures }; | |
| } | |
| function checkGPG(commits) { | |
| const failures = []; | |
| for (const c of commits) { | |
| if (isMergeCommit(c)) continue; | |
| const shortSha = (c.sha || '').slice(0, 7); | |
| const message = c.commit?.message || ''; | |
| const firstLine = message.split('\n')[0] || '(no message)'; | |
| if (!hasVerifiedGPGSignature(c)) { | |
| failures.push({ sha: shortSha, message: firstLine }); | |
| } | |
| } | |
| getLogger().log(`GPG check: ${commits.length - failures.length}/${commits.length} passed`); | |
| return { passed: failures.length === 0, failures }; | |
| } |
| const maxAttempts = 5; | ||
| const delayMs = 2000; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Hoist retry configuration to top-level constants.
maxAttempts/delayMs are function-local; per path instructions, configuration values should be top-level constants rather than embedded inside function bodies.
As per path instructions, "Use top-level constants for configuration — avoid hardcoded values scattered through the script."
Source: Path instructions
| * Identifies the issue numbers referenced in the PR title so they can be excluded from the GraphQL fallback results. | ||
| * @param {string} value | ||
| * @returns {Set<number>} | ||
| */ | ||
| function extractNumbersFromTitle(value) { | ||
| const numbers = new Set(); | ||
| const pattern = /#(\d+)/g; | ||
| if (!value) return numbers; | ||
|
|
||
| let match; | ||
| while ((match = pattern.exec(value)) !== null) { | ||
| numbers.add(parseInt(match[1], 10)); | ||
| } | ||
| return numbers; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '120,240p' .github/scripts/helpers/checks.jsRepository: hiero-ledger/hiero-sdk-python
Length of output: 4269
🏁 Script executed:
set -e
printf '\n## checks.js excerpt\n'
sed -n '120,240p' .github/scripts/helpers/checks.js
printf '\n## call sites / references\n'
rg -n "extractNumbersFromTitle|closingIssuesReferences|no_issue_linked|title" .github/scripts/helpers/checks.js .github/scripts -g '!**/node_modules/**'Repository: hiero-ledger/hiero-sdk-python
Length of output: 14246
🏁 Script executed:
set -e
sed -n '1,260p' .github/scripts/helpers/checks.jsRepository: hiero-ledger/hiero-sdk-python
Length of output: 8521
🌐 Web query:
GitHub GraphQL closingIssuesReferences PR title issue reference title text closing keywords semantics
💡 Result:
In GitHub, the semantics for automatically closing issues via pull requests (PRs) are based on the presence of specific keywords followed by an issue reference in the PR's description or commit messages [1][2][3]. When these conditions are met, GitHub establishes a formal link between the PR and the issue [1]. The GraphQL API provides the closingIssuesReferences field on a PullRequest object, which allows you to programmatically retrieve a list of all issues that are linked to be closed by a given pull request [4][5][6]. Conversely, an Issue object provides the closedByPullRequestsReferences field, which lists the pull requests that, when merged, will close that issue [7][8]. Key details regarding this functionality include: 1. Supported Keywords: The system recognizes a specific set of keywords to trigger the linking mechanism: close, closes, closed, fix, fixes, fixed, resolve, resolves, and resolved [1][2][9][3]. These are case-insensitive and can be followed by a colon (e.g., Closes: #10) [1][9][3]. 2. Linking Requirement: These keywords must be included in the PR's description (the body text) or in commit messages [1][2][3]. If the keyword is in a commit message, the issue will close upon merging that commit to the default branch, but the PR itself will not be listed as a "linked" pull request in the issue's metadata [1][9]. 3. Repository Scope: Linking works for issues in the same repository (e.g., Closes #10) or in different repositories (e.g., Fixes owner/repo#10) [1][9][3]. 4. Default Branch Restriction: The automatic closing behavior is active only when the pull request targets the repository's default branch [1][9]. 5. GraphQL Usage: The closingIssuesReferences field is a connection that returns a list of issues [4]. Developers often use this API to replace manual regex-based parsing of PR bodies when building custom automation or integrations, as it provides a reliable, natively supported way to identify these relationships [5][6]. While these keywords are part of the platform's standard text parsing logic, they are not "semantics" in the sense of a dedicated GraphQL input field; rather, the GraphQL API exposes the result of this parsing logic as a queryable relationship [4][1][5].
Citations:
- 1: https://docs.github.com/en/issues/tracking-your-work-with-issues/using-issues/linking-a-pull-request-to-an-issue
- 2: https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/using-keywords-in-issues-and-pull-requests
- 3: https://docs.github.com/en/enterprise/2.16/user/articles/closing-issues-using-keywords
- 4: https://docs.github.com/en/graphql/reference/pulls
- 5: Simplify
closing-pr.jswithclosingIssuesReferencesGraphQL API mlflow/mlflow#21912 - 6: Add field to retrieve linked issues (
closingIssuesReferences) ingh pr view <number>cli/cli#10529 - 7: https://docs.github.com/en/graphql/reference/issues
- 8: Support GraphQL closingIssuesReferences PyGithub/PyGithub#2567
- 9: https://docs.github.com/en/enterprise-server@3.19/issues/tracking-your-work-with-issues/using-issues/linking-a-pull-request-to-an-issue
Don't filter closingIssuesReferences by PR-title numbers. In .github/scripts/helpers/checks.js:208-214, extractNumbersFromTitle() can drop a real GraphQL-linked issue whenever the PR title also mentions the same #NNN, which makes a correctly linked PR look unlinked. If the goal is to ignore title-only mentions, keep the GraphQL results intact and dedupe against body parsing instead.
🧰 Tools
🪛 OpenGrep (1.25.0)
[ERROR] 157-157: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
| async function fetchAndCheckAssignees(botContext, fetchIssue, issueNumbers, prAuthor) { | ||
| const logger = getLogger(); | ||
| const results = []; | ||
| for (const num of issueNumbers) { | ||
| try { | ||
| const issue = await fetchIssue(botContext, num); | ||
| const isAssigned = (issue.assignees || []).some( | ||
| a => a.login.toLowerCase() === prAuthor.toLowerCase() | ||
| ); | ||
| results.push({ number: num, title: issue.title, isAssigned }); | ||
| } catch (err) { | ||
| logger.log(`Issue link check: could not fetch issue #${num}: ${err.message}`); | ||
| } | ||
| } | ||
| return results; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fetch failures are silently dropped instead of surfaced, weakening the assignment check.
When fetchIssue throws for a linked issue number (rate limit, transient error, etc.), the issue is simply excluded from results (Lines 182-184). If some issues fail to fetch while others succeed, checkIssueLink will only validate the ones that succeeded and can report passed: true even though the un-fetched issue's assignment was never actually verified. If all issues fail to fetch, the reason returned is 'no_issue_linked' (Line 224-226) which is misleading — the PR is linked, the fetch just failed — and will tell an already-compliant author to "link an issue" they already linked.
Consider surfacing fetch failures as an error state (so the dashboard shows the maintainer-review message) rather than silently treating them as absent.
| const isAssigned = (issue.assignees || []).some( | ||
| a => a.login.toLowerCase() === prAuthor.toLowerCase() | ||
| ); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard against missing prAuthor before calling .toLowerCase().
prAuthor (from botContext.pr?.user?.login, Line 203) can be undefined if the PR author's account was deleted (user is null in that case). prAuthor.toLowerCase() here will throw; it's caught by the outer try/catch in runAllChecksAndComment, but the check just degrades to a generic "error" state instead of a clear message. Add an explicit guard.
🛡️ Proposed fix
const issue = await fetchIssue(botContext, num);
+ if (!prAuthor) {
+ throw new Error('PR author is unknown (deleted account?)');
+ }
const isAssigned = (issue.assignees || []).some(As per path instructions, "Validate context.payload fields before use."
📝 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.
| const isAssigned = (issue.assignees || []).some( | |
| a => a.login.toLowerCase() === prAuthor.toLowerCase() | |
| ); | |
| const issue = await fetchIssue(botContext, num); | |
| if (!prAuthor) { | |
| throw new Error('PR author is unknown (deleted account?)'); | |
| } | |
| const isAssigned = (issue.assignees || []).some( | |
| a => a.login.toLowerCase() === prAuthor.toLowerCase() | |
| ); |
Source: Path instructions
| function buildDCOSection(dco) { | ||
| const common = buildSection({ title: 'DCO Sign-off', result: dco, passMessage: 'All commits have valid sign-offs. Nice work!' }); | ||
| if (common) return common; | ||
|
|
||
| const failList = (dco.failures || []).map(f => `- \`${f.sha}\` ${f.message}`).join('\n'); | ||
| return [ | ||
| ':x: **DCO Sign-off** -- Uh oh! The following commits are missing the required DCO sign-off:', | ||
| failList, | ||
| '', | ||
| `No worries, this is an easy fix! Add \`Signed-off-by: Your Name <email>\` to each commit (e.g. \`git commit -s\`). See the [Signing Guide](${SIGNING_GUIDE}).`, | ||
| ].join('\n'); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Unescaped commit-message text is rendered directly into the bot's markdown comment.
f.message (the first line of a commit message — fully attacker/contributor-controlled free-form text) is interpolated into the comment body without any escaping or code-fencing. A crafted commit message containing markdown/HTML could break the dashboard's rendering, inject misleading links, or spoof content in an automated, semi-trusted comment. This pattern is duplicated in buildGPGSection below (Lines 72-79).
🛡️ Proposed fix
- const failList = (dco.failures || []).map(f => `- \`${f.sha}\` ${f.message}`).join('\n');
+ const failList = (dco.failures || []).map(f => `- \`${f.sha}\` \`${f.message.replace(/`/g, '\u02CB')}\``).join('\n');As per path instructions, "Do not trust free-form text from issues, PRs, or comments."
📝 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.
| function buildDCOSection(dco) { | |
| const common = buildSection({ title: 'DCO Sign-off', result: dco, passMessage: 'All commits have valid sign-offs. Nice work!' }); | |
| if (common) return common; | |
| const failList = (dco.failures || []).map(f => `- \`${f.sha}\` ${f.message}`).join('\n'); | |
| return [ | |
| ':x: **DCO Sign-off** -- Uh oh! The following commits are missing the required DCO sign-off:', | |
| failList, | |
| '', | |
| `No worries, this is an easy fix! Add \`Signed-off-by: Your Name <email>\` to each commit (e.g. \`git commit -s\`). See the [Signing Guide](${SIGNING_GUIDE}).`, | |
| ].join('\n'); | |
| } | |
| function buildDCOSection(dco) { | |
| const common = buildSection({ title: 'DCO Sign-off', result: dco, passMessage: 'All commits have valid sign-offs. Nice work!' }); | |
| if (common) return common; | |
| const failList = (dco.failures || []).map(f => `- \`${f.sha}\` \`${f.message.replace(/`/g, '\u02CB')}\``).join('\n'); | |
| return [ | |
| ':x: **DCO Sign-off** -- Uh oh! The following commits are missing the required DCO sign-off:', | |
| failList, | |
| '', | |
| `No worries, this is an easy fix! Add \`Signed-off-by: Your Name <email>\` to each commit (e.g. \`git commit -s\`). See the [Signing Guide](${SIGNING_GUIDE}).`, | |
| ].join('\n'); | |
| } |
Source: Path instructions
| function buildGPGSection(gpg) { | ||
| const common = buildSection({ title: 'GPG Signature', result: gpg, passMessage: 'All commits have verified GPG signatures. Locked and loaded!' }); | ||
| if (common) return common; | ||
|
|
||
| const failList = (gpg.failures || []).map(f => `- \`${f.sha}\` ${f.message}`).join('\n'); | ||
| return [ | ||
| ':x: **GPG Signature** -- Heads up! The following commits don\'t have a verified GPG signature:', | ||
| failList, | ||
| '', | ||
| `You'll need to sign your commits with GPG (e.g. \`git commit -S\`). See the [Signing Guide](${SIGNING_GUIDE}) for a step-by-step walkthrough.`, | ||
| ].join('\n'); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Same unsanitized commit-message rendering as buildDCOSection.
See the comment on buildDCOSection (Lines 51-62) — the same root cause applies here to f.message.
github-actions
Bot
added
open to community review
exploreriii
left a comment
exploreriii
left a comment
There was a problem hiding this comment.
Hi @aceppaluni
I do not want to block this PR because there are some useful things in this.
However, the scope of the PR is much broader and deeper than the issue stated (create a PR dashboard). The changes proposed here create many new features, some of which we already have in the automation suite and will cause duplication and also some conflicts (mergeable state here is different because we have more reviewers). Additionally, we have another PR that will conflict with yours, even though they have solved their issue #2458, since you are adding assignment checks and label things.
Overall, this creates a surface area like the C++ but does not work to consolidate what we currently have, to allow for that transition. The result will be some new functionalities and robsustness, but also some technical debt that will linger and conflict.
I would propose creating a slim PR dashboard that is tailored to what the python SDK conventions currently have, work to delete and remove other scripts that do similar things, consolidate the things that will now need to be reconciled with this new proposal, then we can proceed with it. Let me know what you agree/disagree with and appreciate the time and effort into making this
exploreriii
removed
reviewer: maintainer
|
Will close and reopen fresh when ready |
4 participants
Description:
This PR adds the PR Dashboard as seen in the C++, and Enterprise Java Repositories. This Dashboard is updated with each new commit so contributors are able to view the PR status in Realtime.
Related issue(s):
Fixes #2463
Notes for reviewer:
Testing done in Hiero-Website
aceppaluni/hiero-website#14
aceppaluni/hiero-website#12
Checklist