Add per-area review checklist action and restructure CODEOWNERS#6418
Open
brtnfld wants to merge 15 commits into
Open
Add per-area review checklist action and restructure CODEOWNERS#6418brtnfld wants to merge 15 commits into
brtnfld wants to merge 15 commits into
Conversation
Collaborator
Author
|
This is a discussion starter, and is not final
Table 1. Lines-changed distribution per CODEOWNERS area across 197 PRs merged to |
mkitti
approved these changes
May 28, 2026
Contributor
mkitti
left a comment
mkitti
left a comment
There was a problem hiding this comment.
Seems fine to me. While I appreciate being notified on some changes, it was not clear what my role was in the CODEOWNERS file.
Collaborator
Author
|
@mkitti, I think there were specific file(s) you were interested in monitoring. I can narrow the focus areas if you can elaborate on them, which will help reduce the noise you receive. |
CODEOWNERS: - Replace 11-person global catch-all with specific path rules per area, assigning reviewers based on their actual strengths - Global fallback is now @fortnern only for uncovered root files - Remove @derobins, @epourmal, @qkoziol, @mkitti per team discussion review-checklist GitHub Action (.github/workflows/review-checklist.yml): - Posts a per-area sign-off checklist on every PR to develop (non-forks only) - Reviewer lists and path patterns derived directly from CODEOWNERS - Assigns ONE reviewer per area using fewest-open-PRs load balancing - Complex changes (≥ 300 lines or any public/developer header modified) always go to the first (senior) owner listed; routine changes are load-balanced across all owners (500-line threshold for test/) - Cohesion: reuses an already-assigned reviewer for related areas where owner lists overlap, avoiding e.g. src/ and test/ going to different people - Skips auto-assign if an area owner is already manually requested - Checklist auto-checks when an owner approves; tracks latest review state so a subsequent "request changes" unchecks the box
- Remove the "Waiting for sign-off on all areas" footer — it was misleading once some (but not all) areas are approved. The ✅ all-done banner is kept and still appears when every row is checked. - Document that ties in review-load are broken by CODEOWNERS order, and that this effectively always picks the first-listed owner in a quiet repo where everyone has zero open requests.
… API use
Fix 1 — Glob anchoring: unanchored patterns (e.g. *.cmake) now use
(^|/) instead of ^ so they match files inside subdirectories.
Unanchored plain paths also match anywhere in the tree.
Fix 2 — CODEOWNERS precedence: each changed file is now attributed
only to the last matching area (highest precedence), mirroring
GitHub's CODEOWNERS engine. Previously a file in src/H5FDsubfiling/
would trigger both the src/ and src/H5FDsubfiling/ checklist rows.
Fix 3 — Review state: COMMENTED reviews no longer overwrite an earlier
APPROVED or CHANGES_REQUESTED state. Only APPROVED, CHANGES_REQUESTED,
and DISMISSED are recorded in latestStateByUser.
Fix 4 — Search API rate limit: pendingReviewCount now caches results in
searchCache so the GitHub Search API is never called twice for the
same user in one workflow run.
Fix 5 — Comment pagination: replaced single-page listComments call with
github.paginate so PRs with > 100 comments don't get a duplicate
checklist comment.
Fix 6 — Threshold key normalization: AREA_THRESHOLDS now keyed by
normalized area label ('test') instead of raw pattern ('/test/'),
and the lookup uses area.label to match.
Fix A — Glob-to-regex: replace /\./g (escapes dots only) with a
comprehensive escape /[.+^${}()|[\]\\]/g so all regex metacharacters
are neutralised before wildcard expansion. Fixes *.cmake matching.
Fix B — Concurrency: add concurrency group keyed on PR number with
cancel-in-progress: true to prevent duplicate checklist comments and
double reviewer requests on rapid pushes.
Fix C — allDone: compute from rowData[].signedOff (boolean) instead of
re-parsing rendered markdown strings for '[x]'.
Fix D — Complexity before cohesion: move the threshold/public-header
check ahead of the cohesion reuse check so a complex area always gets
the senior owner, not a junior owner carried over from an earlier area.
Fix E — Stale checklist: when a sync removes all tracked-area changes
the early return now clears any previously-posted checklist comment
rather than leaving it dangling and misleading.
Fix F — Individual reviewer requests: replaced the single bulk
requestReviewers call with a per-reviewer loop so one invalid/non-
collaborator login does not silently drop the rest.
Fix G — Comments: document that fork PRs are intentionally excluded,
that @org/team owners are not supported, and that addAssignees via
issues.* is covered by pull-requests: write.
Move the ~380-line inline JavaScript block out of the YAML and into .github/scripts/review-checklist.js as a proper Node.js module. The YAML script: block is now two lines (require + await run()). Benefits: - IDE tooling (ESLint, Prettier, autocomplete) now works on the script - matchesPattern and labelFromPattern are exported for direct testing - No YAML escaping risks for future JS changes Add .github/scripts/review-checklist.test.js — 17 unit tests for matchesPattern and labelFromPattern using only Node.js built-ins (no npm install required). Run with: node .github/scripts/review-checklist.test.js Pin actions/github-script to commit SHA f28e40c7f34bde8b3046d885e986cb6290c5673b (v7) to protect against tag-ref mutation.
brtnfld
force-pushed
the
review-checklist-action
branch
from
89542e6 to
1cb44d2
Compare
…ling
- Add missing actions/checkout@v4 step — workspace was empty so
require('./review-checklist.js') failed with Cannot find Module on
every run
- Add issues: write permission for addAssignees compatibility under
strict org policies
- Fix double-star glob zero-depth bug: /src/**/*.h now matches
src/file.h (no subdirectory); old split('**').join('.*') approach
required at least one path component between the ** separators
- Fix unanchored directory matching: tools/src/foo.c now matches src/
per gitignore semantics
- Replace N Search API calls (30 req/min limit) with one pulls.list
paginated call; reviewer load counted in memory
- Wrap CODEOWNERS fetch in try-catch for graceful failure
- Add regression tests for zero-depth ** and unanchored directory cases
…tion, harden error handling - Extract attributeFiles, computeApprovals, chooseReviewers, buildBody as exported pure functions; run() is now I/O only (testability, separation of concerns) - Fix inclusive/exclusive attribution mismatch: linesChanged and touchesPublicHeader now both derive from the same attributeFiles() result, preventing spurious complexity escalation for sibling areas like /src/ when a file is attributed to /src/H5FDsubfiling/ - confirmedRequested grows only on successful requestReviewers calls so the checklist never shows an owner whose request silently failed (5c) - Guard null review.user (ghost accounts) in computeApprovals; filter null logins from requested_reviewers (team reviewers without a login field) (5b) - Wrap listFiles, listReviews, pulls.get, and comment upsert in try/catch with core.setFailed / core.warning as appropriate (5a) - Remove redundant indexOf in sort comparator; stable sort on load count preserves CODEOWNERS order for ties (3a) - Add 30 unit tests covering the new pure functions: boundary conditions, public-header escalation, cohesion reuse, load-balance tie-breaking, sole-author area, pre-assigned skip, per-area threshold override (6a) - SHA-pin actions/checkout in both workflow files to match github-script (4b) - Fix /utils missing trailing slash in CODEOWNERS (1a)
brtnfld
requested review from
bmribler,
derobins,
fortnern,
jhendersonHDF,
lrknox,
qkoziol and
vchoi-hdfgroup
as code owners
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CODEOWNERS:
assigning reviewers based on their actual strengths
review-checklist GitHub Action (.github/workflows/review-checklist.yml):
always go to the first (senior) owner listed; routine changes are
load-balanced across all owners (500-line threshold for test/)
owner lists overlap, avoiding e.g. src/ and test/ going to different people
so a subsequent "request changes" unchecks the box