mirror of
https://github.com/HDFGroup/hdf5.git
synced 2026-09-25 04:09:44 +03:00
develop
21
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Scot Breitenfeld
|
46323c29b8 |
review-checklist: ping assignee(s) and label PRs once fully signed off (#6672)
* review-checklist: ping GitHub assignee(s) once the checklist is fully signed off Adds computeAssigneePing(), a pure function that posts a ready-to-merge comment mentioning the PR's assignees the moment every CODEOWNERS area transitions from pending to signed-off. Gated on the false->true transition so it fires once, not on every later workflow run while the PR stays fully approved, and posted as its own comment since editing an existing comment to add a mention isn't a reliable notification. * review-checklist: sync a checklist-complete label with the all-done state Toggles the "checklist-complete" label (pre-created in the repo, green #0E8A16) on and off to track whether every CODEOWNERS area is currently signed off, so it's visible as a badge on the /pulls list without opening each PR. Unlike the assignee ping, this is level-triggered: the label comes off again if a later change request breaks the sign-off, and reattaches once the PR is fully approved again. |
||
|
Scot Breitenfeld
|
31d9d3c322 |
Fix release-progress badges to read native issue-level Priority field (#6668)
* Fix release-progress badges to read native issue-level Priority field The Project Priority board field was consolidated into a Priority field managed at the issue level (GitHub's org-wide custom issue fields), which the GraphQL API mirrors into projects as ProjectV2ItemIssueFieldValue instead of ProjectV2ItemFieldSingleSelectValue. The badge script's query had no fragment for that type, so it silently found zero priority values and would raise ProjectFieldMissingError on every run. * Show TBD on the Next Release badge when no milestone due date is set Previously the badge silently omitted any due-date annotation when the milestone had none, which read as if the badge just hadn't picked one up. * Update release-schedule image and plantuml source from add-next-release-badge branch * Don't flag CODEOWNERS auto-assignment as a manual reviewer add on a PR's first pass GitHub's own CODEOWNERS engine fires an identical review_requested webhook (sender type User, not Bot) when it auto-assigns owners at PR-open time. If one of those survives the cancel-in-progress race as the run that actually executes, it was mistaken for a human deliberately requesting that reviewer, permanently flagging them "manually added" and requiring separate approval — e.g. a catch-all "*" owner also named on a touched area's CODEOWNERS line got flagged on every PR touching that area, even though no human ever picked them. Gate the manual-add detection and the sticky per-area assignment on !isFirstCoordinationPass (no checklist comment posted yet), since that's exactly the window where GitHub's own auto-assignment is indistinguishable from a real human pick. |
||
|
Scot Breitenfeld
|
9515219a32 |
Review automation: fix comment-only reviewer drop and draft-stale thrash (#6658)
* review-checklist: don't lose a reviewer who only left comments GitHub un-requests a reviewer the instant they submit any review, including a comment-only one from batching several inline comments into a single submission — not just Approve/Request-changes. Unlike a stale approval, this never produces a DISMISSED transition, so the checklist had no way to tell "abandoned the area" apart from "still reviewing, just hasn't finished yet." This silently dropped the reviewer's mention from the checklist display the moment they left comments, and risked the next push's additive-fill picker handing their area to a completely different load-balanced reviewer (observed on PR #6645: jhendersonHDF's batched review comments repeatedly vanished him from src/test's rows mid-review). Track a sticky area assignee as still engaged whenever they have no APPROVED/CHANGES_REQUESTED/DISMISSED review on record, regardless of whether GitHub currently lists them as requested, and use that in the read-only display path, resolveAreaPicks, and the additive-fill picker. * draft-pr-policy: a checked keepalive checkbox is real activity too lastRealActivityAt() deliberately ignores bot comments so a metadata-only bump can't dodge the staleness check forever. But checking the "Still working on this" checkbox is an edit to the bot's own keepalive comment, not a new comment of the human's own — its author stays github-actions[bot], so the edit was invisible to the activity check too. Confirming via the checkbox removed the label and posted "Thanks for confirming" without ever moving the underlying 60-day clock, so the very next scheduled run saw the same stale last-activity timestamp and immediately re-flagged it — contradicting the checkbox's own promise that checking it "resets this". Observed on #6326: once its true last activity fell behind the 60-day window, and only the checkbox (never a new commit or comment) was used to confirm it, the label thrashed on and off on a roughly 1-2 day loop. Now a checked keepalive checkbox counts via the comment's updated_at (when it was toggled), same as any other real activity signal. |
||
 Scot BreitenfeldandH. Joe Lee
|
7b37aac674 |
review-checklist: make avalanche pruning prefer the already-settled reviewer (#6596)
Every CODEOWNERS "avalanche" (multiple owners of one area simultaneously requested — on PR creation, on ready_for_review, or when a push first touches an already-covered area) was resolved by re-running the load-balancer from scratch, with no memory of who was already the settled reviewer for that area. Since open-PR review load drifts over time, this made the pick non-deterministic across repeated events and caused three related symptoms: - marking a draft ready for review could swap out reviewers who were already assigned (manually or from an earlier pass) for different people - a routine follow-up push could re-avalanche an area and bump the already-engaged reviewer for whoever currently has a lighter queue - a manually re-requested reviewer survived the triggering event (the existing forced-pick handling already covers that) but got silently removed again on a later event, since two now-requested owners look identical to an unpruned avalanche without that memory Add a persisted per-area "sticky assignment" record (ASSIGNED_PREFIX, alongside the existing exclusion and manually-added markers) and a resolveAreaPicks() helper that prefers a valid sticky pick, then a lone already-requested owner, before ever falling back to a fresh load-balanced pick. Manual review_requested actions now write to this record too, so a deliberate reviewer choice holds up across future runs, not just the run it was made on. Co-authored-by: H. Joe Lee <hyoklee@hdfgroup.org> |
||
|
Scot Breitenfeld
|
1fc781961d |
Add change-request and manually-added-CODEOWNER lines to review checklist (#6528)
* Add change-request and manually-added-CODEOWNER lines to review checklist Reviewers who request changes now get their own line under each area their inline comments touch, whether or not they're a CODEOWNER or a requested reviewer. CODEOWNERS who are review-requested directly by a human (rather than auto-picked) get a separate required-approval line, and their approval is now required for that area's sign-off in addition to the usual reviewer's. * Prune reviewers whose area dropped out of scope A CODEOWNER requested for an area the PR used to touch, before a later push narrowed the diff, was never removed: touchedAreaOwners only reflects areas touched right now, so none of the existing avalanche pruning ever considered them. Reproduced live on PR #6528, where the branch's diff shrank to just .github/ but gheber/fortnern/mattjala (owners of unrelated areas) stayed as requested reviewers. Manually-added CODEOWNERS are exempt, since a human's deliberate request shouldn't be silently undone by a later push. * review-checklist: fix avalanche exemption regressing genuine avalanches #6524 exempted a review_requested login's whole area from avalanche detection to stop a manual re-request from being undone. But a genuine CODEOWNERS avalanche's own review_requested sub-events look identical to that case, and one of them can survive the ready_for_review vs. review_requested concurrency race (#6530: draft PR marked ready for review left all 3 CODEOWNERS owners requested instead of pruning to one, because the surviving run's action was one of the avalanche's own review_requested events, exempting the whole area). Replace the exemption with a forced pick: the directly-requested login still always survives, but now by replacing the rest of that area's avalanche instead of exempting the area from pruning — so a genuine avalanche still collapses to one person. Also gate justRequestedLogin on !isBotSender, matching updatedManuallyAdded's existing reasoning just above it: the bot's own requestReviewers calls fire this same event and aren't a human decision. |
||
|
Scot Breitenfeld
|
50465b5f6a |
review-checklist: exempt a just-requested reviewer from avalanche pruning (#6524)
A direct review_requested (e.g. manually re-requesting a reviewer via the GitHub UI) adds a second currently-requested owner to that reviewer's area — indistinguishable from an unpruned CODEOWNERS avalanche, so the avalanche-detection pass immediately removed them again on the same run. Carve out the login this run's review_requested action names before running avalanche detection. |
||
|
Scot Breitenfeld
|
7858034164 |
ci(review-checklist): fix three CODEOWNERS reviewer-avalanche bugs (#6485)
Three distinct but related bugs caused the review-checklist bot to @ mention more reviewers than intended. **Bug 1 — bot-sender guard (sticky exclusion)** The bot's own removeRequestedReviewers API calls fire review_request_removed events that self-trigger the workflow. Without a guard, that self-triggered run treated its own bookkeeping removal as a deliberate human decision and added the login to the persisted exclusion set permanently. Fix: check sender.type === 'Bot' and skip the exclusion update for bot-originated removals. **Bug 2 — cancel-in-progress race on opened/ready_for_review** GitHub fires one review_requested event per CODEOWNERS auto-assigned owner; each triggers a workflow run. With cancel-in-progress, the surviving run may be a review_requested rather than the opened event, bypassing the avalanche-prune branch entirely and leaving all CODEOWNERS requested. Fix: track hasExistingComment as a proxy for "first coordination pass" — if no checklist comment exists yet, prune regardless of which action survives the race. **Bug 3 — per-area avalanche on synchronize (PR #6484)** GitHub's CODEOWNERS engine re-fires when a commit first touches a new CODEOWNERS-covered area, not only on PR open but also on synchronize. The surviving run fell through to additive fill, saw the area as "already has owners, skip", and listed all auto-assigned CODEOWNERS. Fix: before the synchronize-swap and additive-fill paths, detect any area whose owner-list ∩ existingRequested > 1 (per-area avalanche) and prune that area to the single load-balanced pick. 84 tests (was 82). |
||
|
Scot Breitenfeld
|
884ce02101 |
ci: replace actions/stale with bot-aware mark-stale script (#6477)
* ci: replace actions/stale with bot-aware mark-stale script actions/stale uses updatedAt to measure inactivity, so any bot event (e.g. the /remove-reviewer acknowledgment comment) resets the stale countdown even when there has been no meaningful human activity for months. PR #6332 was last touched by a human on 2026-04-02 but was not flagged because a reviewer-removal on 2026-06-11 refreshed the timestamp. Replace the actions/stale step with a custom mark-stale.js script (same pattern as alert-stale.js) that only counts non-bot comments, non-bot review submissions, and commits as meaningful activity. The script also removes the stale label if such activity occurs after the label was applied. * ci: fix draft-stale keepalive for external contributors Two bugs with the keepalive checkbox on draft-stale PRs: 1. External contributors (fork authors) lack write access to edit the bot's comment, so clicking the checkbox silently fails for them. Fix: also treat a new non-bot comment posted after the keepalive comment as a sufficient keepalive signal. 2. The stale label could take up to 24 hours to be removed (daily cron only). Fix: add an issue_comment.created trigger so draft-pr-policy fires immediately when someone comments on a stale draft PR. mark-stale and alert-stale are guarded to only run on schedule/workflow_dispatch, not on every comment. Also fix lastRealActivityAt to exclude bot comments (matching mark-stale.js), so the keepalive and "Thanks for confirming" bot comments don't count as real activity when measuring staleness. |
||
|
Scot BreitenfeldandH. Joe Lee
|
0f1dfcc08e |
fix(review-checklist): re-request dismissed reviewer on fixup push (#6479)
* fix(review-checklist): re-request dismissed reviewer on fixup push When a new commit dismisses a prior reviewer's approval, GitHub's CODEOWNERS engine auto-assigns a fresh (possibly different) owner for the changed area. The synchronize handler now detects dismissed area-owners and swaps them back in, removing the fresh CODEOWNERS pick. Adds planSynchronizeSwaps() pure helper (exported) with 7 unit tests covering the PR 6475 scenario and edge cases. * fix(review-checklist): don't remove a fresh pick still needed by another area planSynchronizeSwaps could remove a fresh CODEOWNERS pick to restore a dismissed reviewer even when that pick also covered a different, unrelated touched area — removeRequestedReviewers strips them from the whole PR, silently uncovering the other area. Now skips removal when the candidate owns any other touched area. Also dedupes the consuming loop so a login needed by two areas isn't requested/removed twice. 5 new tests covering the cross-area guard and its boundaries. * fix(review-checklist): don't let bot's own reviewer removal create a sticky exclusion The bot's own removeUnselected/removeRequestedReviewers calls (draft-opened CODEOWNERS cleanup, stale-exclusion enforcement) fire review_request_removed, which the workflow also listens on — self-triggering another run. That run previously read its own bookkeeping removal as a deliberate human decision and added the login to the persisted exclusion set, permanently blocking that owner from ever being auto-assigned to the PR again. Guard on sender.type !== 'Bot' so only human-driven removals become sticky. * fix(review-checklist): prune CODEOWNERS avalanche regardless of which event wins the race GitHub's CODEOWNERS engine fires one review_requested event per auto-assigned owner on PR creation, and each re-triggers this workflow. With concurrency: cancel-in-progress, whichever run starts last wins — and that's just as likely to be one of those review_requested runs as the opened run itself. A surviving review_requested run fell through to the additive-fill branch, saw every area already "covered" by the avalanche, and pruned nothing. This hit PR #6479 itself: all 4 CODEOWNERS for .github/ stayed requested. Branch on whether a checklist comment exists yet instead of which action survived — that signal is race-resistant: no comment means this is the PR's first coordination pass no matter which event got here. Threads hasExistingComment through from run() (defaulting to true, i.e. additive-fill, on a comment-fetch failure so an API hiccup can't be mistaken for a fresh PR). 2 new tests: the #6479 race itself, and a contrast case confirming a routine review_requested on an already-established PR still uses additive-fill. --------- Co-authored-by: H. Joe Lee <hyoklee@hdfgroup.org> |
||
|
Scot Breitenfeld
|
e251b341d8 |
CI: base draft PR staleness on real activity, not pr.updated_at (#6472)
pr.updated_at is bumped by metadata-only changes (reviewer requested/removed, labels, milestone, assignee), letting an abandoned draft dodge the staleness check indefinitely. Use the latest commit/comment/review/review-comment timestamp instead. |
||
|
Scot Breitenfeld
|
2265cf5bdc |
CI: prune CODEOWNERS avalanche on non-draft PR open (#6474)
* CI: prune CODEOWNERS avalanche on non-draft PR open GitHub auto-assigns every touched-area CODEOWNER when a non-draft PR opens. The previous additive-only logic saw all areas already covered and returned the full auto-assigned set as confirmedRequested, so the checklist @-mentioned every owner before the final reviewer set was known. On opened/reopened (non-draft), now behave like the draft case: prune to a single load-balanced pick per area via removeUnselected before posting the checklist, requesting any pick not already on the PR. * CI: guard reviewer-exclusion update against bot-triggered removals The bot's own CODEOWNERS cleanup and draft-handling calls fire review_request_removed events that self-trigger another workflow run. Without this guard, that run would interpret the bot's own removal as a deliberate human decision and permanently add the login to the exclusion set, blocking that owner from ever being auto-assigned to the PR again. |
||
|
Scot BreitenfeldandH. Joe Lee
|
daa1daf640 |
CI: fix reviewer-coordination edge cases — draft transitions, unrelated CODEOWNERS, explicit removals (#6465)
review-checklist.js previously tried to enforce a single load-balanced reviewer per area by actively stripping anyone else who showed up, using a checklist-comment-existence heuristic (isOpeningRace) to decide when it was safe to do so. Walking through real PR timelines surfaced several cases where that heuristic and that stripping behavior did the wrong thing: - An old PR's first encounter with the bot (or one whose checklist comment was deleted) was misdiagnosed as a brand-new PR, forcibly reselecting a human-curated reviewer list. - A CODEOWNER for areas this PR doesn't touch (e.g. a project lead added by hand for their judgment) was swept by the same logic as junk auto-assignment, with no protection for an existing approval. - A reviewer who isn't an owner of any touched area was invisible in the checklist and untouched by any cleanup path either way. - A PR that picked up reviewers while non-draft, then was converted to draft, had those reviewers wiped out by the next push — converted_to_draft isn't in this workflow's trigger list, so there's no event at the actual transition to distinguish "noise from this PR's creation" from "a real assignment from before it became a draft." - A reviewer removed via the PR UI could be silently re-added by GitHub's own CODEOWNERS engine on a later push, since there was no memory of the removal across runs. Redesign around one principle: the bot should never automatically strip a reviewer who's already on the PR, manually added or auto-assigned, except where the policy explicitly calls for it. Concretely: - coordinateReviewers is now purely additive for non-draft PRs: chooseReviewers runs against the *real* existingRequested, so it naturally skips any area that already has someone and only fills gaps. This is idempotent and safe on every event, eliminating the need for the old race-detection heuristic entirely (isOpeningRace, the new-PR/synchronize distinction, and the post-request 15s delayed re-clear are all removed as dead weight). - Pruning — used only where the bot still clears reviewers — is scoped to touchedAreaOwners (owners of areas this PR touches, plus catch-all "*" owners) instead of the repo-wide allCodeOwners, so a CODEOWNER for unrelated areas is never touched. - Draft handling clears auto-assigned reviewers only at the literal opened/reopened-as-draft moment — the one point where "CODEOWNERS noise from this PR's creation" and "this PR's actual reviewer state" are mechanically the same thing. Every other event while draft leaves existing reviewers alone. - An explicit review_request_removed persists that login to an exclusion list embedded as a second hidden marker in the checklist comment (the only durable storage available), and every run strips anyone in that list who reappears — covering GitHub re-assigning them on a later push. A direct review_requested for that exact login overrides the exclusion, since it's the strongest available signal that someone individually decided that person should be back right now. - buildBody adds a catch-all "Additional reviewers" line for anyone requested who isn't an owner of any touched area, so they're visible and their approval is shown (informationally — it doesn't gate any area's sign-off), and accepts any owner's approval for an area's sign-off rather than only the specifically-assigned one's. 12 new tests cover the buildBody changes and the parseExcluded/serializeExcluded round-trip; all existing pure-function tests are unchanged. Co-authored-by: H. Joe Lee <hyoklee@hdfgroup.org> |
||
|
Scot BreitenfeldandH. Joe Lee
|
a7eeb4fa9b |
Add stale PR policy with assignee alerts. (#6463)
* Add stale PR/issue policy with assignee alerts instead of auto-close Ready PRs/issues use actions/stale to label inactivity (30/60 days). Draft PRs get a longer 90-day window and only reset on an explicit "still working on this" comment, since pushes/CI activity alone shouldn't make an abandoned draft look fresh. Nothing is auto-closed: once a staleness label has persisted past its alert threshold, a custom script pings the assignee (falling back to requested reviewers, then the author) to decide whether to keep it open or close it. * Scope stale policy to PRs only, not issues Issue staleness is disabled (days-before-issue-stale: -1) and the alert script now skips any non-PR item defensively, since this workflow is meant to address PRs sitting unmerged/unreviewed, not issue triage. * Draft stale window 60 days (was 90); drop dead pull_request branch alert-stale.js's pickAlertTargets is now only ever called with PR items (filtered upstream in runAlertStale), so the pull_request check around the requested-reviewers lookup was dead code. * Set persist-credentials: false on checkout steps Fixes two zizmor notes: these checkouts only need to read local script files for github-script's require(), so there's no reason to persist the GITHUB_TOKEN in git config afterward. * Use a keep-alive checkbox instead of a magic comment phrase for drafts Checking a box in the bot's own stale-notice comment is more discoverable than requiring an exact phrase, and the live checkbox state can be read straight off that comment's current body on each run instead of scanning new comments for a regex match. --------- Co-authored-by: H. Joe Lee <hyoklee@hdfgroup.org> |
||
|
Scot Breitenfeld
|
4d3559c5b1 |
CI: fix buildBody sign-off and add non-CODEOWNER reviewer tests (#6461)
* CI: fix buildBody to accept any owner approval, not only the assigned reviewer PR #6446 narrowed the approver lookup to effectiveReviewers (the confirmed- requested subset), inadvertently breaking the case where a non-assigned owner approves. Restore the original intent: any CODEOWNER's approval signs off an area; fall back to effectiveReviewers only when no CODEOWNER is assigned. * text updates * CI: add tests for non-CODEOWNER reviewer sign-off path (#6446) Three tests cover the nonOwnerReviewers fallback introduced in #6446: - pending mention when a non-owner is manually assigned - non-owner approval signs off an area with no CODEOWNER assigned - non-owner approval does NOT sign off when a CODEOWNER was assigned * CI: restore review_requested/review_request_removed triggers These were dropped in #6453 when ready_for_review was added, breaking automatic checklist updates on manual reviewer changes. Both events fall through to the preserve-existing path in coordinateReviewers when a checklist already exists, so the checklist body is simply rebuilt with the current requested_reviewers set. Also extend the opening-race detection (previously called isFirstSyncRace) to cover review_requested: for any PR, CODEOWNERS auto-assignment fires review_requested shortly after opened, and with cancel-in-progress: true that run can cancel the opened run. If no checklist exists yet, treat the review_requested event as a new-PR event and run full selection. * CI: show only one reviewer per checklist area Each area's pending mention now shows the primary (first) effective reviewer rather than all confirmed-requested owners joined with commas. * CI: revert to showing all confirmed reviewers per checklist area A manually added person who is also a CODEOWNER for that area should be shown alongside the load-balanced pick, not hidden. "One reviewer per area" only describes chooseReviewers' default selection, not a display constraint. |
||
|
Scot Breitenfeld
|
ec33e7573c |
CI: fix reviewer workflows for already-reviewed and draft PRs (#6453)
* CI: skip reviewer removal when they have already submitted a review
GitHub's API rejects removeRequestedReviewers for users who have already
reviewed; detect that case upfront via listReviews and report it clearly
instead of surfacing a cryptic API error.
* CI: defer reviewer assignment for draft PRs until ready for review
Add ready_for_review to the pull_request_target trigger so the workflow
fires when a draft is promoted. In the script, skip requestReviewers
(and the CODEOWNERS auto-assignment cleanup second pass) while the PR is
a draft; still assign the PR author and strip any reviewers GitHub
auto-assigned. When the PR is marked ready, treat it like a fresh open
and run the full load-balanced reviewer selection.
* CI: fix synchronize race — treat first synchronize as new PR when opened was cancelled
When a fork PR is created, GitHub fires both opened and synchronize.
With cancel-in-progress: true the synchronize run often wins, and the
opened run (which clears CODEOWNERS auto-assignments) is cancelled before
it completes. Detect this: if synchronize fires but no checklist comment
exists yet, run the full new-PR path (enforceSelection + load-balanced
reviewer assignment) instead of silently carrying forward all auto-assigned
reviewers.
* CI: enforce reviewer selection before posting @mentions
On synchronize, run enforceSelection against the ideal load-balanced
pick before building the checklist body so the @mentions are never
sent until the reviewer list is actually correct. Re-fetch the PR
after cleanup so confirmedRequested reflects reality, not the pre-
cleanup snapshot.
For workflow_run (review submitted), reviewer assignment is intentionally
left unchanged but @mentions are filtered to the ideal selection so
CODEOWNERS extras don't generate spurious notifications on comment edits.
* CI: preserve manually added reviewers on synchronize
Reverting the enforceSelection call on the non-first-run synchronize
path. There is no API way to distinguish CODEOWNERS auto-assignments
from manually added reviewers, so enforcing the load-balanced selection
on every synchronize would silently remove intentional additions.
The initial cleanup (opened or first-synchronize via openedWasSkipped)
already produces a correct reviewer list; subsequent synchronize and
workflow_run events carry it forward unchanged.
* CI: pin actions/github-script to commit hash in remove-reviewer.yml
zizmor requires actions to be pinned to a commit hash rather than a tag.
Use the same pinned hash (ed597411d8f924073f98dfc5c65a23a2325f34cd, v8.0.0)
already used in review-checklist.yml.
* CI: refactor review-checklist.js for readability
Extract the reviewer coordination logic out of the monolithic run()
function into named module-level helpers:
checklistExists() — single responsibility: does a checklist
comment already exist on the PR?
removeUnselected() — remove CODEOWNERS auto-assignments not in
the load-balanced selection set
requestReviewers() — request each reviewer individually so one
bad login cannot block the rest
removeUnselectedAfterDelay() — 15-second wait + re-fetch + re-enforce
for GitHub's async auto-assignment race
coordinateReviewers() — top-level dispatcher; the four event paths
(read-only, synchronize-normal, new-PR/draft,
new-PR/non-draft) are now explicit branches
with labelled comments instead of nested ifs
run() is now a straight pipeline of 8 numbered steps with no nested
async functions. Behaviour is unchanged.
* CI: extract convertGlobToRegex and use github.paginate consistently
Extract glob-to-regex conversion into a standalone helper and replace
manual pagination loops for listFiles and listReviews with github.paginate,
matching the existing style used for listComments and pulls.list.
* CI: restrict remove-reviewer workflow to main repo only
Adds a github.repository guard so the job does not run in forks,
preventing unintended resource consumption and command execution
against fork PRs.
* CI: restrict test-maven-packages workflow to main repo only
Adds github.repository guards to all three jobs so the workflow
does not evaluate in forks, preventing spurious "workflow file issue"
failures on push events in forked repositories.
* Revert "CI: restrict test-maven-packages workflow to main repo only"
This reverts commit
|
||
|
Scot Breitenfeld
|
41ea347fb9 |
review-checklist: show non-CODEOWNER reviewer when no area owner is assigned (#6446)
* review-checklist: show non-CODEOWNER reviewer when no owner is assigned If the only assigned CODEOWNER is removed and a non-CODEOWNER is manually added in their place, that person previously got no checklist mention and their approval did not check the box. For any area with no CODEOWNER in the requested set, fall back to non-CODEOWNER reviewers (anyone assigned who is not an owner of any touched area). They are shown in the mention and their approval counts as sign-off for that area. * review-checklist: update checklist on reviewer add/remove Adding or removing a reviewer did not trigger a workflow run, leaving the checklist comment stale until the next push. Add review_requested and review_request_removed to the pull_request_target activity types so the checklist updates immediately when a reviewer is manually added or removed. |
||
|
Scot Breitenfeld
|
65fc219bc9 |
review-checklist: enforce one load-balanced reviewer and fix checklist display (#6439)
* review-checklist: show all requested owners per area in checklist buildBody was using area.owners.find() — picking the first CODEOWNERS-listed owner in the requested set. This broke reviewer swaps (removing @A and adding @B would show @C, the next CODEOWNERS entry, not @B) and also failed to reflect GitHub's CODEOWNERS auto-assignment, which requests all owners. Switch to area.owners.filter() so every requested owner for an area is mentioned in that row. Approval logic is unchanged: any owner approval still checks the box. * review-checklist: enforce one load-balanced reviewer per area GitHub's CODEOWNERS auto-assignment requests all owners of touched files when a PR opens, before the workflow runs. The script was then seeing them already assigned and skipping its own selection entirely. On opened/reopened: select one load-balanced reviewer per area (ignoring GitHub's pre-assigned set), remove any auto-assigned CODEOWNERS not in the selection, then add the chosen reviewer. Only code owners are removed — manually-added non-owner reviewers are left untouched. On synchronize: keep existing assignments (reviewer may have already started). confirmedRequested now starts empty and is populated only with the script's selection, so the checklist only mentions owners that were explicitly chosen. * review-checklist: retry reviewer cleanup to handle GitHub auto-assign race GitHub's CODEOWNERS auto-assignment can fire after the workflow starts, re-adding extra reviewers after we remove them. Add a 15-second wait followed by a second cleanup pass on opened/reopened events. Extract the removal loop into enforceSelection() so both passes share the same logic. * zizmor: suppress template-injection for PR #6356 workflow files Add zizmor_config.yml to ignore template-injection findings in the setup-jextract action and maven/ctest workflow files introduced in PR #6356, where inputs are caller-controlled but not user-controlled. * review-checklist: don't re-assign reviewers on synchronize On synchronize, chooseReviewers saw no owner assigned (because the reviewer was manually removed) and re-added one via requestReviewers, overriding the manual removal. Reviewer assignment now only happens on opened/reopened. Synchronize only updates the checklist display based on whoever is currently assigned — manual removals are respected. * zizmor: skip upload-sarif failure on fork PRs * zizmor: move config out of workflows dir to fix GitHub Actions parse error GitHub Actions parses all .yml files in .github/workflows/ as workflow files; zizmor_config.yml there caused "unexpected value 'rules'" because rules: is not valid workflow syntax. Moved to .github/zizmor.yml and updated the --config path in zizmor.yml accordingly. * review-checklist: retry on transient 401 from GitHub API GitHub's API intermittently returns 401 on write operations (issues.addAssignees, issues.createComment) even when the token has Issues: write and PullRequests: write — read-only calls succeed in the same run. The github-script action excludes 401 from retries by default. Removing 401 from retry-exempt-status-codes and setting retries: 3 handles these transient failures with exponential backoff. * review-checklist: fix checklist mention when non-requested owner approves The mention in each checklist row was derived from the approver (if any), so when a different area owner happened to approve first, the mention changed from the assigned reviewer to the approver. Fix by decoupling sign-off detection from display: signedOff now uses .some() so any owner's approval checks the box, while the mention always shows the confirmed-requested reviewer(s) via .filter(). Also shows multiple reviewers when one is manually added alongside the load-balanced selection. * review-checklist: show approver name when signed off, requested reviewer(s) when pending When an area is signed off, replace the mention with the approver so the checklist shows who actually reviewed it (which may differ from whoever was load-balanced as the requested reviewer). When pending, show all confirmed-requested reviewers — the one load-balanced pick normally, but two if a reviewer was manually added alongside it. * zizmor: remove config file and --config flag The ignored files (ctest.yml, maven-deploy.yml, maven-staging.yml, setup-jextract/action.yml) had template-injection findings on inputs.* references, which are not attacker-controllable. Rather than suppressing them via a config file, let all findings surface in the Security tab and address them individually if needed. |
||
|
Scot BreitenfeldandH. Joe Lee
|
64036c6d03 |
CI: use pull_request_target so review-checklist runs on fork PRs (#6435)
* CI: use pull_request_target so review-checklist runs on fork PRs pull_request events from forks receive a read-only GITHUB_TOKEN and are skipped by the job condition, so no checklist comment is posted and no reviewers are assigned. Switching to pull_request_target fixes this: GitHub always executes the workflow from the base repo (HDFGroup/hdf5) with a full write token, regardless of whether the PR comes from a fork. The fork's code is never checked out or executed. The cross-repo guard (head.repo.full_name == github.repository) is removed since it is no longer needed — pull_request_target only fires for PRs targeting this repo's branches. * CI: replace pull_request_target with safe two-workflow pattern for fork PRs pull_request_target carries a GitHub security warning because it grants write access to the privileged workflow at trigger time, which is risky if the checkout is ever changed to use the fork's head ref. Replace it with the recommended two-workflow pattern: - review-checklist-gather.yml fires on pull_request (read-only, safe for forks) and uploads the PR number as a short-lived artifact. - review-checklist.yml fires on workflow_run when the gather job completes, downloads the artifact for the PR number, then posts the checklist comment and assigns reviewers with a full write token. It never executes any code from the fork. The script gains a prNumber parameter so the main workflow can supply the PR number it read from the artifact; falls back to the event payload for pull_request_review triggers. * review-checklist: fix prAuthor crash and stale comment for workflow_run context In the workflow_run path context.payload.pull_request is undefined, so reading .user.login from it throws a TypeError. Use prData.user.login instead — prData is already fetched from the API at step 5. Also remove the stale comment saying fork PRs require pull_request_target; they are now handled via the two-workflow pattern. * CI: revert to pull_request_target, drop two-workflow pattern The workflow_run approach adds a second hop before the checklist posts and introduced a context.payload.pull_request crash in the gather path. pull_request_target is simpler and equally safe here: GitHub always executes the workflow from HDFGroup/hdf5:develop with a full write token — the fork's code is never checked out or executed. * CI: add zizmor workflow for GitHub Actions security analysis zizmor is a static analyser that catches security issues specific to GitHub Actions: pull_request_target misuse, expression injection, unpinned action SHAs, and overly broad permissions. Runs on push to develop and on PRs that touch .github/, uploads SARIF to the Security tab (free for public repos via Advanced Security), and annotates PR diffs inline with any findings. * CI: run zizmor via pip instead of third-party action Replace zizmorcore/zizmor-action (third-party) with a direct pip install of zizmor==1.25.2. SARIF output is still uploaded to the Security tab via github/codeql-action/upload-sarif (first-party). The only actions used are from actions/ and github/, both trusted. * CI: fix codeql-action pin to commit SHA (was tag object SHA) * CI: add persist-credentials: false to review-checklist checkout * CI: hash-pin zizmor pip install; suppress expected pull-request-target finding - zizmor.yml: pin zizmor wheel to its SHA256 hash via --require-hashes so the install is fully reproducible and passes zizmor's unpinned-tools audit - review-checklist.yml: add zizmor: ignore[pull-request-target] comment so zizmor does not flag its own host workflow; document why the usage is safe * CI: update actions to Node.js 24 ahead of June 16 deprecation - actions/checkout: v4.2.2 → v5.0.1 (node20 → node24) - actions/github-script: v7.0.1 → v8.0.0 (node20 → node24) - github/codeql-action: v3.36.2 → v4.36.2 (node20 → node24; v3 deprecated Dec 2026) * CI: fix pip hash-pinning syntax — hash must be in requirements file * CI: zizmor step continue-on-error so SARIF upload always runs * review-checklist: skip pull_request_review on fork PRs (read-only token) GitHub only grants a read-only GITHUB_TOKEN for pull_request_review events when the PR originates from a fork, so the comment post fails with 403. Restrict the review trigger to same-repo PRs; pull_request_target already handles fork PRs for the open/sync/reopen case with a full write token. * review-checklist: route reviews through workflow_run for fork PR support pull_request_review grants only a read-only token for fork PRs, causing a 403 when posting the checklist comment. Fix with a two-path design: - pull_request_target handles open/sync/reopen for all PRs (full token, immediate) - A new gather workflow (review-checklist-gather.yml) fires on pull_request_review, saves the PR number as an artifact, and exits. The main workflow then fires via workflow_run with a full write token. The script gains prNumber and isReview parameters. isReview suppresses reviewer assignment (which only applies on open/sync/reopen). prAuthor is sourced from prData.user.login since context.payload.pull_request is not populated in the workflow_run context. * review-checklist: simplify to pull_request_target only, drop gather workflow pull_request_review triggers a fork workflow approval gate for first-time contributors, which blocks every PR. There is no pull_request_review_target equivalent in GitHub Actions. Instead, rely on pull_request_target (opened/synchronize/reopened) only. Approval boxes are still evaluated correctly: computeApprovals() re-reads all existing reviews on every push, so boxes auto-check the next time the author pushes after a reviewer approves. Also restores the simpler script signature (no prNumber/isReview params) and sources prAuthor from prData.user.login which works in all contexts. * review-checklist: auto-check approval boxes on review via workflow_run Add a minimal gather workflow (review-checklist-gather.yml) triggered by pull_request_review that completes immediately with a read-only token. The main workflow now also triggers via workflow_run on gather completion, giving it the full write token it needs to update the checklist comment. In workflow_run context, the script looks up the PR by head SHA (since workflow_run.pull_requests is empty for fork PRs) and skips reviewer assignment — only approval box state is updated. Requires the repo "Fork pull request workflows from outside collaborators" setting to be "Require approval for first-time contributors", which is already the current setting. --------- Co-authored-by: H. Joe Lee <hyoklee@hdfgroup.org> |
||
|
Scot Breitenfeld
|
f3e380ce17 |
Add per-area review checklist action and restructure CODEOWNERS (#6418)
* Add per-area review checklist action and restructure CODEOWNERS 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 The previous regex only matched *public.h and *develop.h, missing hdf5.h itself (the umbrella header), all VFD driver headers included by hdf5.h (H5FDcore.h, H5FDmpio.h, H5FDsubfiling.h, etc.), and VOL connector headers (H5VLconnector.h, H5VLnative.h, etc.). Changes to any of these now correctly trigger senior-owner assignment. |
||
 Allen Byrne
|
9b865ef667 |
Fixes for maven verify and testing (#5996)
Add generate-index-html.sh script and update workflows to generate and upload index.html files for HDF5 releases, enhancing Maven package testing and deployment. |
||
|
Allen Byrne
|
b754dcb8f2 |
Move Java wrappers to FFM using jextract and java 25 (#5957)
FFM build requires Java 25, Jextract 25. Generates FFM bindings during configure. JNI is default when the requirements are not met or can be forced. Presets added for maven and FFM - JNI is default selection. Enhanced Maven options will work with either JNI or FFM New Workflows for testing and maven uploads. Extensive documentation changes for java. |