feat: add PR review status evaluator (all 3 phases) - #2503
exploreriii merged 18 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR adds configurable review stages, team-role roster parsing, a read-only GitHub review-status evaluator, workflow reporting, roster updates, and Jest coverage for API retrieval, status computation, formatting, and orchestration. ChangesPR review status evaluation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Workflow
participant evaluateReviewStatus
participant GitHubAPI
participant getTeamRoles
participant computeStatus
Workflow->>evaluateReviewStatus: resolve PR number
evaluateReviewStatus->>GitHubAPI: fetch labels and reviews
evaluateReviewStatus->>GitHubAPI: fetch reviewDecision
evaluateReviewStatus->>getTeamRoles: load role roster
GitHubAPI-->>evaluateReviewStatus: return review data
getTeamRoles-->>evaluateReviewStatus: return roles and availability
evaluateReviewStatus->>computeStatus: compute review status
computeStatus-->>evaluateReviewStatus: return status
evaluateReviewStatus-->>Workflow: publish formatted status
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b308c9fd-59d9-4323-b295-3d0ce2da3d5a
📒 Files selected for processing (5)
.github/scripts/__tests__/jest/review-status-evaluator.test.js.github/scripts/__tests__/jest/shared-review-stages.test.js.github/scripts/review-status-evaluator.js.github/scripts/shared/review-stages.js.github/workflows/pr-review-status-evaluator.yml
github-actions
Bot
added
open to community review
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/scripts/__tests__/jest/review-status-evaluator.test.js (1)
22-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest multiple pagination pages.
Line 46 always yields one response. The tests cannot verify that
getPRLabelsandgetDetailedReviewsaggregate more than one page before processing results.Extend
createMockGithubto yield configured label and review pages. Add assertions for labels from later pages and for a reviewer whose latest submission is on a later review page.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 863b7128-0983-4045-aff7-0cab933f84cc
📒 Files selected for processing (4)
.github/scripts/__tests__/jest/review-status-evaluator.test.js.github/scripts/__tests__/jest/shared-review-stages.test.js.github/scripts/review-status-evaluator.js.github/workflows/pr-review-status-evaluator.yml
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 143da594-e088-40e5-b5e3-46ae0c31b687
📒 Files selected for processing (1)
.github/scripts/__tests__/jest/review-status-evaluator.test.js
exploreriii
left a comment
exploreriii
left a comment
There was a problem hiding this comment.
I have some questions regarding the maintenance burden for this PR since it duplicates some content, want to hear your thoughts
exploreriii
removed
queue:junior-committer
|
Hey @exploreriii, thanks for the review. My thoughts on each:
Only catch: there's no similar file for committers, just the GitHub team in CODEOWNERS, and reading team membership needs an API call. So committer/triage matching stays approximate for now, I'll leave a comment explaining that in the code. Let me know if you want a COMMITTERS.md added later to close that gap too, or if this is fine as is. Will make these changes when you confirm they are the better choices. Thank you. |
|
Sure please review, if the option isn't appropriate, can always create the committer/triage list |
|
I pushed the fixes for all 3 points. One more thing I noticed while working on the committer/triage check: docs/team.md and MAINTAINERS.md don't fully agree on who's a maintainer. rbair23 is listed in MAINTAINERS.md but not in docs/team.md. I used docs/team.md since it has triage and committer too, not just maintainers, but wanted to flag the mismatch in case one of them needs updating. |
exploreriii
left a comment
exploreriii
left a comment
There was a problem hiding this comment.
You are right, maybe all those can be updated as part of the PR here is the accurate list:
- name: hiero-sdk-python-committers
maintainers:
- exploreriii
- manishdait
members:
- aceppaluni
- Adityarya11
- Akshat8510
- AntonioCeppellini
- Dosik13
- parvninama
- danielmarv
- Mounil2005
- name: hiero-sdk-python-maintainers
maintainers:
- exploreriii
- manishdait
members:
- MonaaEid
- name: hiero-sdk-python-triage
maintainers:
- exploreriii
- manishdait
members:
- cheese-cakee
- prajeeta15
- tech0priyanshu
source https://github.com/hiero-ledger/governance/blob/main/config.yaml
github-actions
Bot
added
open to community review
|
Updated docs/team.md to match that list exactly. Two quick checks before I push further:
|
|
rbair23 is not a maintainer, the governance repo is the legit source of who has permissions in this repo |
|
Pushed the update - MAINTAINERS.md and docs/team.md now match the governance list exactly. Let me know if all is settled. |
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/pr-review-status-evaluator.yml (1)
51-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove reporting logic into a dedicated script.
This inline block selects the no-status behavior, publishes five outputs, and builds the job summary. Move this logic to an exported reporter under
.github/scripts/. Keep this workflow step limited to calling that function.As per path instructions, “YAML should orchestrate execution — not implement business logic” and “Non-trivial logic belongs in dedicated scripts under
.github/scripts/.”Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2e06b405-9f0e-460f-b454-1bb72959d1c2
📒 Files selected for processing (5)
.github/scripts/review-status-evaluator.js.github/scripts/shared/team-roles.js.github/workflows/pr-review-status-evaluator.ymlMAINTAINERS.mddocs/team.md
github-actions
Bot
added
the
skill: beginner
exploreriii
left a comment
exploreriii
left a comment
There was a problem hiding this comment.
Hi @phillip-nyinomujuni
Key questions I have is how much already existing logic can we use, and what do we need to build truly as new? if so, what can be done as an easy to maintain function or component, rather than a hardcoded insertion that will become hard to maintain over time
Currently, the PR status will just log to the action logs - this isn't really easily contributor facing, what are your thoughts on creating a comment in the pr
exploreriii
removed
queue:junior-committer
|
Hey @exploreriii , good questions. I went and looked into this more and you're right, I was overcomplicating it. Turns out review-sync/helpers/permissions.js already has getPermissionLevel(), which does live role checks (admin/maintain/write/triage) for the queue label bot. I didn't realize triage was an actual GitHub permission level until I checked bot-intermediate-assignment.yml, it uses it too. So there was no reason for me to build a separate docs/team.md roster parser to answer basically the same question ("what role does this person have"). I ripped that out and switched to using getPermissionLevel() directly. Bonus: this also kills the parse bug you found — there's no file to parse anymore, so it can't silently break like that again. And since nothing reads docs/team.md in this evaluator now, the CODEOWNERS ask for it doesn't really apply to this PR anymore (might still be worth doing for other reasons, just not blocking this one). Also fixed the hardcoded labels in the tests, pulling from shared/labels.js like you said. On the PR comment idea, I like it, but I'd rather leave it out of this PR. Posting a comment needs issues: write, and the original issue was pretty explicit about keeping v1 read-only/non-enforcing. The job summary already covers "contributor can see the status somewhere" without needing write access. Could do a comment mode as a fast follow if people want it, let me know what you think. Pushed the changes, should be ready for another look. |
… yet Signed-off-by: NYINOMUJUNI PHILLIP <phillipnyinomujini@gmail.com>
496fab9 to
3639804
Compare
github-actions
Bot
added
open to community review
|
Hello, this is the OfficeHourBot. This is a reminder that the Hiero Python SDK Office Hours will begin in approximately 3 hours and 38 minutes (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, |
AntonioCeppellini
left a comment
AntonioCeppellini
left a comment
There was a problem hiding this comment.
@andrerovee can you take a look as you are a js dragon?
aceppaluni
added
the
status: update branch
Signed-off-by: exploreriii <133720349+exploreriii@users.noreply.github.com>
Signed-off-by: exploreriii <133720349+exploreriii@users.noreply.github.com>
Signed-off-by: exploreriii <133720349+exploreriii@users.noreply.github.com>
exploreriii
left a comment
exploreriii
left a comment
There was a problem hiding this comment.
Hi @phillip-nyinomujuni I added some of the prior requested feedback e.g. codeowners/hardcoding etc, should be good to try this out and see the results to the review process
Signed-off-by: exploreriii <133720349+exploreriii@users.noreply.github.com>
|
Thank you @phillip-nyinomujuni |
|
👋 Hi @phillip-nyinomujuni! Great work completing a Beginner issue! 🎉 Thanks for your contribution! 🚀 Here are some issues you might want to explore next: 🌟 Stay connected: Happy coding! 🚀 |
6 participants
Summary
Implements a read-only PR review status evaluator: computes what stage of
review a PR is in, who's expected to review it, who it's still waiting on,
and the recommended next action — then logs it and exposes it via job
outputs / job summary. Covers all three phases of the task in one PR.
Changes
1. Review stage model (
.github/scripts/shared/review-stages.js)AWAITING_REVIEW,CHANGES_REQUESTED,APPROVED,AWAITING_TRIAGE(env-overridable, following the pattern inshared/labels.js)TRIAGE,COMMITTER,MAINTAINERgetExpectedReviewers(labels)mapping skill labels to expected reviewer types2. Evaluator script (
.github/scripts/review-status-evaluator.js)getPRLabels(REST, paginated),getReviewState(GraphQLreviewDecision),getDetailedReviews(REST, deduplicated to each reviewer's latest review) —following the existing patterns in
bot-pr-draft-ready-reminder.jsandbot-pr-draft-explainer.jscomputeStatus(labels, reviewDecision, detailedReviews)— returnscurrentStage,expectedReviewers,waitingOn,nextAction,summaryevaluateReviewStatus(github, context)orchestrates the above; supportsDRY_RUNenv varNote on
waitingOn: GitHub returns reviews by username, not CODEOWNERSrole, so mapping a review to a reviewer type would need extra
team-membership API calls. Instead,
expectedReviewersis treated as anordered list of sequential approval gates, and
waitingOnis derived bycounting approvals received. This is an approximation — documented in the
file's header comment — not a true role-aware computation.
3. Workflow (
.github/workflows/pr-review-status-evaluator.yml)pull_request(opened,synchronize,reopened,labeled,unlabeled,ready_for_review) andpull_request_review(submitted,dismissed)contents: read,pull-requests: readstep-security/harden-runnerwithegress-policy: auditcore.info, sets outputs viacore.setOutput, and writes a jobsummary via
core.summaryTesting
npm run test:js— 184/184 tests pass (36 new across the two new test files)uv run ruff check ./uv run ruff format --check .— cleanNotes
__tests__/jest/rather than co-located with theirsource modules, matching the existing convention (
shared-labels.test.jsfor
shared/labels.js).Fixes #2247