From f3e380ce170f5aed5d84bbe839e7895ea431ebf5 Mon Sep 17 00:00:00 2001 From: Scot Breitenfeld Date: Thu, 4 Jun 2026 16:21:53 -0500 Subject: [PATCH] Add per-area review checklist action and restructure CODEOWNERS (#6418) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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. --- .github/CODEOWNERS | 53 ++- .github/scripts/review-checklist.js | 448 ++++++++++++++++++++ .github/scripts/review-checklist.test.js | 393 +++++++++++++++++ .github/workflows/review-checklist-test.yml | 23 + .github/workflows/review-checklist.yml | 43 ++ 5 files changed, 951 insertions(+), 9 deletions(-) create mode 100644 .github/scripts/review-checklist.js create mode 100644 .github/scripts/review-checklist.test.js create mode 100644 .github/workflows/review-checklist-test.yml create mode 100644 .github/workflows/review-checklist.yml diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index c41b2a9ecfd..62574c1543b 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -1,13 +1,48 @@ # Lines starting with '#' are comments. # Each line is a file pattern followed by one or more owners. +# Last matching pattern takes the most precedence. -# These owners will be the default owners for everything in the repo. -* @lrknox @derobins @fortnern @jhendersonHDF @qkoziol @vchoi-hdfgroup @bmribler @glennsong09 @mattjala @brtnfld @mkitti +# Global fallback — fires only for files not covered by a more specific rule +# below. Keep this list small; add specific path rules instead of expanding it. +* @jhendersonHDF + +# ── Language bindings ───────────────────────────────────────────────────────── +/fortran/ @brtnfld @bmribler +/java/ @jhendersonHDF @mattjala +/c++/ @bmribler @glennsong09 + +# ── Core C library ──────────────────────────────────────────────────────────── +/src/ @fortnern @jhendersonHDF @mattjala @vchoi-hdfgroup @glennsong09 +/src/H5FDsubfiling/ @jhendersonHDF @brtnfld @fortnern +/utils/ @brtnfld @mattjala @jhendersonHDF @bmribler + +# ── Tests ───────────────────────────────────────────────────────────────────── +/test/ @fortnern @jhendersonHDF @mattjala @vchoi-hdfgroup @glennsong09 +/testpar/ @jhendersonHDF @fortnern @mattjala @brtnfld + +# ── High-level API ──────────────────────────────────────────────────────────── +/hl/ @brtnfld @mattjala + +# ── Tools ───────────────────────────────────────────────────────────────────── +/tools/ @jhendersonHDF @mattjala @bmribler + +# ── Examples ────────────────────────────────────────────────────────────────── +/HDF5Examples/ @brtnfld @jhendersonHDF @mattjala @vchoi-hdfgroup @glennsong09 + +# ── Build system ────────────────────────────────────────────────────────────── +/CMakeLists.txt @jhendersonHDF @mattjala @lrknox +/CMakePresets.json @jhendersonHDF @mattjala @lrknox +*.cmake @jhendersonHDF @mattjala @lrknox +/config/ @jhendersonHDF @mattjala @lrknox + +# ── Documentation ───────────────────────────────────────────────────────────── +/docs/ @brtnfld @gheber @vchoi-hdfgroup @glennsong09 @lrknox +/docs/INSTALL* @lrknox @hyoklee +/release_docs/ @lrknox @hyoklee @glennsong09 + +# ── CI / GitHub Actions ─────────────────────────────────────────────────────── +/.github/ @hyoklee @lrknox @jhendersonHDF @glennsong09 + +# The HDF Group website verification — must stay in sync with hdfgroup.org +/.github/.well-known @lkurz @lrknox -# Order is important. The last matching pattern has the most precedence. -# So if a pull request only touches javascript files, only these owners -# will be requested to review. -/fortran/ @brtnfld @derobins @epourmal -/java/ @jhendersonHDF @mattjala -# The HDF Group website needs to be updated to reflect any changes made here. -/.github/.well-known @lkurz @loricooperhdf diff --git a/.github/scripts/review-checklist.js b/.github/scripts/review-checklist.js new file mode 100644 index 00000000000..ed9adfb9c37 --- /dev/null +++ b/.github/scripts/review-checklist.js @@ -0,0 +1,448 @@ +'use strict'; + +const MARKER = ''; + +function labelFromPattern(pattern) { + // /fortran/ → "fortran", /.github/.well-known → ".github/.well-known" + return pattern.replace(/^\//, '').replace(/\/$/, '') || pattern; +} + +// Returns true if `file` (repo-relative, no leading slash) matches +// a CODEOWNERS-style gitignore pattern. +function matchesPattern(file, pattern) { + let p = pattern; + const anchored = p.startsWith('/'); + + if (anchored) p = p.slice(1); + + // Directory pattern: /fortran/ → matches fortran/ + if (p.endsWith('/')) { + return anchored + ? file.startsWith(p) + : (file === p.slice(0, -1) || file.startsWith(p) || file.includes('/' + p)); + } + + // Glob pattern: convert * and ** to regex equivalents. + // Process ** before * so the single-star replacement cannot corrupt double-star tokens. + if (p.includes('*')) { + // Escape regex metacharacters, leaving * intact for the steps below + let escaped = p.replace(/[.+^${}()|[\]\\]/g, '\\$&'); + // /**/ → zero or more path components (zero depth = just a single slash separator) + escaped = escaped.replace(/\/\*\*\//g, '/(?:.+/)?'); + // **/ at start → any leading directories (including none) + escaped = escaped.replace(/^\*\*\//, '(?:.+/)?'); + // /** at end → any trailing path (including none) + escaped = escaped.replace(/\/\*\*$/, '(?:/.+)?'); + // bare ** → anything (fallback for patterns like **) + escaped = escaped.replace(/\*\*/g, '.*'); + // single * → within one path component only + escaped = escaped.replace(/\*/g, '[^/]*'); + const re = new RegExp((anchored ? '^' : '(^|/)') + escaped + '($|/)'); + return re.test(file); + } + + // Plain path: exact match or directory prefix + if (anchored) { + return file === p || file.startsWith(p + '/'); + } else { + return file === p || file.startsWith(p + '/') || file.endsWith('/' + p) || file.includes('/' + p + '/'); + } +} + +// Returns Map — each file attributed to exactly one area +// (the most-precedent match; last entry in areas[] wins, as in CODEOWNERS). +// Using a single attribution pass here means linesChanged and touchesPublicHeader +// in chooseReviewers both operate on the identical file set — no double-counting. +function attributeFiles(changedFileData, areas) { + const filesByArea = new Map(areas.map(a => [a.pattern, []])); + for (const file of changedFileData) { + for (let i = areas.length - 1; i >= 0; i--) { + if (matchesPattern(file.filename, areas[i].pattern)) { + filesByArea.get(areas[i].pattern).push(file); + break; + } + } + } + return filesByArea; +} + +// Returns Set of logins whose most-recent substantive review state is APPROVED. +// COMMENTED reviews are ignored — they don't change the approval state. +// A CHANGES_REQUESTED or DISMISSED review after an APPROVED one cancels the approval. +function computeApprovals(reviews) { + const latest = {}; + for (const review of reviews) { + if (!review.user) continue; // ghost / deleted account + const { state } = review; + if (state === 'APPROVED' || state === 'CHANGES_REQUESTED' || state === 'DISMISSED') { + latest[review.user.login] = state; + } + } + return new Set( + Object.entries(latest) + .filter(([, s]) => s === 'APPROVED') + .map(([login]) => login) + ); +} + +// Pure reviewer selection. Returns { selected, updatedRequested, log }. +// +// `touchedAreas` entries must carry `.files` (array of file objects with +// `.filename`) and `.linesChanged` (number), produced by attributeFiles(). +// +// Returns: +// selected — Set of newly chosen reviewers (to be requested) +// updatedRequested — Set of existingRequested ∪ selected (for callers +// that need the full post-assignment picture before API calls) +// log — string[] of per-decision messages for core.info() +function chooseReviewers(touchedAreas, { + prAuthor, + existingRequested, + reviewerLoad, + LINE_THRESHOLD, + AREA_THRESHOLDS, + PUBLIC_HEADER, +}) { + const selected = new Set(); + const updatedRequested = new Set(existingRequested); + const log = []; + + for (const area of touchedAreas) { + if (area.owners.some(o => updatedRequested.has(o))) { + log.push(`Area "${area.label}": already has owner assigned — skipping`); + continue; + } + + const threshold = (AREA_THRESHOLDS && AREA_THRESHOLDS[area.label]) ?? LINE_THRESHOLD; + const touchesPublicHeader = area.files.some(f => PUBLIC_HEADER.test(f.filename)); + const isComplex = area.linesChanged >= threshold || touchesPublicHeader; + + if (isComplex) { + const pick = area.owners.find(u => u !== prAuthor) ?? null; + const reason = touchesPublicHeader + ? 'public header modified' + : `${area.linesChanged} lines ≥ ${threshold}`; + log.push(`Area "${area.label}" is complex (${reason}) — primary owner: ${pick ?? '(none)'}`); + if (pick) { selected.add(pick); updatedRequested.add(pick); } + continue; + } + + // Routine change: cohesion — reuse an already-assigned owner if they also + // cover this area, to avoid splitting related areas across reviewers. + const cohesionPick = [...selected].find(u => area.owners.includes(u) && u !== prAuthor); + if (cohesionPick) { + updatedRequested.add(cohesionPick); + log.push(`Area "${area.label}": reusing ${cohesionPick} for cohesion`); + continue; + } + + const candidates = area.owners.filter(u => u !== prAuthor); + if (candidates.length === 0) { + log.push(`Area "${area.label}": all owners are the PR author — no reviewer assigned`); + continue; + } + + // Load-balance: pick the candidate with the fewest open review requests. + // Ties are broken by CODEOWNERS order (stable sort preserves input order). + const counts = candidates.map(u => ({ u, n: (reviewerLoad && reviewerLoad[u]) || 0 })); + counts.sort((a, b) => a.n - b.n); + const pick = counts[0].u; + log.push(`Area "${area.label}": load [${counts.map(c => `${c.u}=${c.n}`).join(', ')}] → ${pick}`); + selected.add(pick); + updatedRequested.add(pick); + } + + return { selected, updatedRequested, log }; +} + +// Builds the markdown checklist comment body (pure, no I/O). +function buildBody(touchedAreas, approvedUsers, confirmedRequested) { + const rowData = touchedAreas.map(area => { + const approver = area.owners.find(o => approvedUsers.has(o)); + const assigned = approver ?? area.owners.find(o => confirmedRequested.has(o)); + const signedOff = !!approver; + const box = signedOff ? 'x' : ' '; + const tick = signedOff ? ' ✅' : ''; + const mention = assigned ? ` — @${assigned}` : ''; + return { text: `- [${box}] **${area.label}**${tick}${mention}`, signedOff }; + }); + + const allDone = rowData.every(r => r.signedOff); + const rows = rowData.map(r => r.text); + + const parts = [ + MARKER, + '## Review Checklist', + '', + 'This PR touches the following areas. Each needs at least one', + 'sign-off from its listed owners before merging — an approval', + 'covering only one area does **not** satisfy the others.', + '', + ...rows, + ]; + if (allDone) parts.push('', '> ✅ All areas have been signed off.'); + return parts.join('\n'); +} + +module.exports = async function run({ github, context, core }) { + const { owner, repo } = context.repo; + const pr_number = context.payload.pull_request.number; + + // ---------------------------------------------------------------- + // Configuration + // + // LINE_THRESHOLD: lines changed within a single area at or above + // which the change is considered complex → first (senior) owner + // in CODEOWNERS is always assigned. + // + // PUBLIC_HEADER: files matching this pattern are always treated as + // complex regardless of line count — any change to the public or + // developer API surface warrants the senior owner. + // + // Covers: hdf5.h (umbrella), H5*public.h / H5*develop.h (per-module), + // VFD driver headers included by hdf5.h, and VOL connector headers. + // + // NOTE: Fork PRs (head.repo != base.repo) are intentionally excluded. + // They run with a read-only token and cannot post comments or request + // reviewers. Fork coverage would require a pull_request_target job. + // + // NOTE: Team owners (@org/team) in CODEOWNERS are not supported. + // Only individual GitHub logins are handled. If teams are added, + // extend parsing and reviewer requests to use team_reviewers. + // ---------------------------------------------------------------- + const LINE_THRESHOLD = 300; // default for all areas + const AREA_THRESHOLDS = { // per-area overrides + 'test': 500, // test files are verbose; raise bar for senior + }; + const PUBLIC_HEADER = /(?:^|\/)hdf5\.h$|public\.h$|develop\.h$|H5FD(?:core|direct|family|hdfs|ioc|log|mirror|mpio?|multi|onion|ros3|sec2|splitter|stdio|subfiling|windows)\.h$|H5VL(?:connector|connector_passthru|native|passthru)\.h$/; + + // ---------------------------------------------------------------- + // 1. Parse CODEOWNERS into a list of { pattern, label, owners } + // ---------------------------------------------------------------- + let coText; + try { + const { data: coData } = await github.rest.repos.getContent({ + owner, repo, path: '.github/CODEOWNERS', + }); + coText = Buffer.from(coData.content, 'base64').toString('utf-8'); + } catch (error) { + core.setFailed(`Failed to load CODEOWNERS: ${error.message}`); + return; + } + + const areas = []; + const allCodeOwners = new Set(); + for (const rawLine of coText.split('\n')) { + const line = rawLine.trim(); + if (!line || line.startsWith('#')) continue; + + const tokens = line.split(/\s+/); + const pattern = tokens[0]; + const owners = tokens.slice(1) + .filter(t => t.startsWith('@')) + .map(t => t.slice(1)); + + owners.forEach(o => allCodeOwners.add(o)); + if (pattern === '*') continue; + if (owners.length === 0) continue; + + areas.push({ pattern, label: labelFromPattern(pattern), owners }); + } + + if (areas.length === 0) { + core.info('No path-specific rules found in CODEOWNERS — skipping checklist.'); + return; + } + + // ---------------------------------------------------------------- + // 2. Collect all changed files with line counts. + // ---------------------------------------------------------------- + const changedFileData = []; + try { + for (let page = 1; ; page++) { + const { data } = await github.rest.pulls.listFiles({ + owner, repo, pull_number: pr_number, per_page: 100, page, + }); + changedFileData.push(...data); + if (data.length < 100) break; + } + } catch (error) { + core.setFailed(`Failed to list PR files: ${error.message}`); + return; + } + + // ---------------------------------------------------------------- + // 3. Attribute files to areas (each file → one area, most-precedent). + // Derive per-area line totals and file lists from the same map so + // linesChanged and touchesPublicHeader always agree. + // ---------------------------------------------------------------- + const filesByArea = attributeFiles(changedFileData, areas); + const touchedAreas = areas + .map(area => { + const files = filesByArea.get(area.pattern) || []; + return { + ...area, + files, + linesChanged: files.reduce((sum, f) => sum + f.changes, 0), + }; + }) + .filter(area => area.linesChanged > 0); + + if (touchedAreas.length === 0) { + core.info('No CODEOWNERS-tracked areas changed — skipping checklist.'); + try { + const allComments = await github.paginate(github.rest.issues.listComments, { + owner, repo, issue_number: pr_number, per_page: 100, + }); + const stale = allComments.find(c => c.body.includes(MARKER)); + if (stale) { + await github.rest.issues.updateComment({ + owner, repo, comment_id: stale.id, + body: MARKER + '\n_No CODEOWNERS-tracked areas are touched by this PR — no review checklist required._', + }); + core.info(`Cleared stale checklist comment #${stale.id}`); + } + } catch (e) { + core.warning(`Could not clean up stale checklist comment: ${e.message}`); + } + return; + } + + // ---------------------------------------------------------------- + // 4. Determine current approvals. + // ---------------------------------------------------------------- + const allReviews = []; + try { + for (let page = 1; ; page++) { + const { data } = await github.rest.pulls.listReviews({ + owner, repo, pull_number: pr_number, per_page: 100, page, + }); + allReviews.push(...data); + if (data.length < 100) break; + } + } catch (error) { + core.warning(`Failed to fetch reviews; approval state may be stale: ${error.message}`); + } + const approvedUsers = computeApprovals(allReviews); + + // ---------------------------------------------------------------- + // 5. Fetch current PR state (requested reviewers). + // ---------------------------------------------------------------- + let prData; + try { + ({ data: prData } = await github.rest.pulls.get({ + owner, repo, pull_number: pr_number, + })); + } catch (error) { + core.setFailed(`Failed to fetch PR data: ${error.message}`); + return; + } + const existingRequested = new Set( + prData.requested_reviewers.map(r => r.login).filter(Boolean) + ); + + // confirmedRequested tracks what was actually successfully requested + // (used in buildBody so the checklist never shows an owner whose + // requestReviewers call silently failed). + let confirmedRequested = new Set(existingRequested); + + // ---------------------------------------------------------------- + // 6. Auto-assign reviewers (pull_request events only, not reviews). + // ---------------------------------------------------------------- + if (context.eventName !== 'pull_request_review') { + const prAuthor = context.payload.pull_request.user.login; + + // Assign the PR author only if they are a code owner. + if (allCodeOwners.has(prAuthor)) { + try { + await github.rest.issues.addAssignees({ + owner, repo, issue_number: pr_number, assignees: [prAuthor], + }); + core.info(`Assigned PR to author ${prAuthor} (is a code owner)`); + } catch (e) { + core.warning(`Could not assign PR to author: ${e.message}`); + } + } else { + core.info(`Author ${prAuthor} is not a code owner — skipping assignee`); + } + + // One paginated pulls.list call instead of N Search API calls — the Search API + // allows only 30 req/min; with several candidates per area that limit is easy to hit. + // Note: cost scales with the number of open PRs in the repo. + let reviewerLoad = {}; + try { + const openPRs = await github.paginate(github.rest.pulls.list, { + owner, repo, state: 'open', per_page: 100, + }); + for (const openPR of openPRs) { + if (openPR.number === pr_number) continue; + for (const r of openPR.requested_reviewers) { + if (r.login) reviewerLoad[r.login] = (reviewerLoad[r.login] || 0) + 1; + } + } + } catch (e) { + core.warning(`Could not fetch open PRs for load balancing; falling back to CODEOWNERS order: ${e.message}`); + } + + const { selected, log } = chooseReviewers(touchedAreas, { + prAuthor, + existingRequested, + reviewerLoad, + LINE_THRESHOLD, + AREA_THRESHOLDS, + PUBLIC_HEADER, + }); + for (const msg of log) core.info(msg); + + // Request one at a time so a single invalid login cannot block the rest. + // Only add to confirmedRequested on success — the checklist must not show + // an owner whose request call failed. + for (const reviewer of selected) { + try { + await github.rest.pulls.requestReviewers({ + owner, repo, pull_number: pr_number, + reviewers: [reviewer], + }); + confirmedRequested.add(reviewer); + } catch (e) { + core.warning(`Could not request reviewer ${reviewer}: ${e.message}`); + } + } + } + + // ---------------------------------------------------------------- + // 7. Build the checklist body. + // ---------------------------------------------------------------- + const body = buildBody(touchedAreas, approvedUsers, confirmedRequested); + + // ---------------------------------------------------------------- + // 8. Create or update the checklist comment (idempotent via marker). + // ---------------------------------------------------------------- + try { + const comments = await github.paginate(github.rest.issues.listComments, { + owner, repo, issue_number: pr_number, per_page: 100, + }); + const existing = comments.find(c => c.body.includes(MARKER)); + + if (existing) { + await github.rest.issues.updateComment({ + owner, repo, comment_id: existing.id, body, + }); + core.info(`Updated checklist comment #${existing.id}`); + } else { + await github.rest.issues.createComment({ + owner, repo, issue_number: pr_number, body, + }); + core.info('Created checklist comment'); + } + } catch (error) { + core.setFailed(`Failed to post checklist comment: ${error.message}`); + } +}; + +module.exports.matchesPattern = matchesPattern; +module.exports.labelFromPattern = labelFromPattern; +module.exports.attributeFiles = attributeFiles; +module.exports.computeApprovals = computeApprovals; +module.exports.chooseReviewers = chooseReviewers; +module.exports.buildBody = buildBody; diff --git a/.github/scripts/review-checklist.test.js b/.github/scripts/review-checklist.test.js new file mode 100644 index 00000000000..1cbf8fcc094 --- /dev/null +++ b/.github/scripts/review-checklist.test.js @@ -0,0 +1,393 @@ +'use strict'; +// Run with: node .github/scripts/review-checklist.test.js + +const assert = require('assert'); +const { + matchesPattern, + labelFromPattern, + attributeFiles, + computeApprovals, + chooseReviewers, + buildBody, +} = require('./review-checklist.js'); + +let passed = 0; +let failed = 0; + +function test(name, fn) { + try { + fn(); + console.log(`✓ ${name}`); + passed++; + } catch (e) { + console.log(`✗ ${name} — ${e.message}`); + failed++; + } +} + +// ---------------------------------------------------------------- +// matchesPattern — anchored directory patterns +// ---------------------------------------------------------------- + +test('anchored dir: fortran/src/H5f.F90 matches /fortran/', () => { + assert.strictEqual(matchesPattern('fortran/src/H5f.F90', '/fortran/'), true); +}); + +test('anchored dir: src/H5public.h does not match /fortran/', () => { + assert.strictEqual(matchesPattern('src/H5public.h', '/fortran/'), false); +}); + +test('anchored dir: src/H5FDsubfiling/foo.c matches /src/H5FDsubfiling/', () => { + assert.strictEqual(matchesPattern('src/H5FDsubfiling/foo.c', '/src/H5FDsubfiling/'), true); +}); + +test('anchored dir: src/H5FDsubfiling/foo.c matches less-specific /src/', () => { + assert.strictEqual(matchesPattern('src/H5FDsubfiling/foo.c', '/src/'), true); +}); + +// ---------------------------------------------------------------- +// matchesPattern — anchored plain-file patterns +// ---------------------------------------------------------------- + +test('anchored file: CMakeLists.txt matches /CMakeLists.txt', () => { + assert.strictEqual(matchesPattern('CMakeLists.txt', '/CMakeLists.txt'), true); +}); + +test('anchored file: src/CMakeLists.txt does not match /CMakeLists.txt', () => { + assert.strictEqual(matchesPattern('src/CMakeLists.txt', '/CMakeLists.txt'), false); +}); + +// ---------------------------------------------------------------- +// matchesPattern — unanchored glob (*.cmake) +// ---------------------------------------------------------------- + +test('unanchored glob: config/foo.cmake matches *.cmake', () => { + assert.strictEqual(matchesPattern('config/foo.cmake', '*.cmake'), true); +}); + +test('unanchored glob: a/b/deep/x.cmake matches *.cmake', () => { + assert.strictEqual(matchesPattern('a/b/deep/x.cmake', '*.cmake'), true); +}); + +test('unanchored glob: src/H5public.h does not match *.cmake', () => { + assert.strictEqual(matchesPattern('src/H5public.h', '*.cmake'), false); +}); + +test('unanchored glob: config/foo.cmake with pattern /config/*.cmake is anchored, not a bare wildcard', () => { + // The spec lists this under the unanchored-glob section to contrast *.cmake (matches anywhere) + // with /config/*.cmake (anchored prefix glob, a distinct category). The definitive expected + // value from the "Anchored glob with path prefix" section (and the implementation) is true. + assert.strictEqual(matchesPattern('config/foo.cmake', '/config/*.cmake'), true); +}); + +// ---------------------------------------------------------------- +// matchesPattern — anchored glob with path prefix +// ---------------------------------------------------------------- + +test('anchored prefix glob: config/cmake/foo.cmake does not match /config/*.cmake (subdir, * no cross /)', () => { + assert.strictEqual(matchesPattern('config/cmake/foo.cmake', '/config/*.cmake'), false); +}); + +test('anchored prefix glob: config/foo.cmake matches /config/*.cmake', () => { + assert.strictEqual(matchesPattern('config/foo.cmake', '/config/*.cmake'), true); +}); + +// ---------------------------------------------------------------- +// matchesPattern — double-star glob +// ---------------------------------------------------------------- + +test('double-star glob: src/deep/nested/file.h matches /src/**/*.h', () => { + assert.strictEqual(matchesPattern('src/deep/nested/file.h', '/src/**/*.h'), true); +}); + +test('double-star glob: docs/file.h does not match /src/**/*.h', () => { + assert.strictEqual(matchesPattern('docs/file.h', '/src/**/*.h'), false); +}); + +test('double-star glob: src/file.h matches /src/**/*.h (zero-depth directory)', () => { + assert.strictEqual(matchesPattern('src/file.h', '/src/**/*.h'), true); +}); + +test('unanchored directory: tools/src/foo.c matches src/', () => { + assert.strictEqual(matchesPattern('tools/src/foo.c', 'src/'), true); +}); + +// ---------------------------------------------------------------- +// labelFromPattern +// ---------------------------------------------------------------- + +test('labelFromPattern: /fortran/ => "fortran"', () => { + assert.strictEqual(labelFromPattern('/fortran/'), 'fortran'); +}); + +test('labelFromPattern: *.cmake => "*.cmake"', () => { + assert.strictEqual(labelFromPattern('*.cmake'), '*.cmake'); +}); + +test('labelFromPattern: /CMakeLists.txt => "CMakeLists.txt"', () => { + assert.strictEqual(labelFromPattern('/CMakeLists.txt'), 'CMakeLists.txt'); +}); + +// ---------------------------------------------------------------- +// attributeFiles +// ---------------------------------------------------------------- + +test('attributeFiles: file goes to most-precedent (last) matching area', () => { + const areas = [ + { pattern: '/src/', label: 'src', owners: ['alice'] }, + { pattern: '/src/H5FDsubfiling/', label: 'src/H5FDsubfiling', owners: ['bob'] }, + ]; + const files = [{ filename: 'src/H5FDsubfiling/foo.c', changes: 10 }]; + const byArea = attributeFiles(files, areas); + assert.strictEqual(byArea.get('/src/').length, 0); + assert.strictEqual(byArea.get('/src/H5FDsubfiling/').length, 1); +}); + +test('attributeFiles: file in /src/ is not stolen by /src/H5FDsubfiling/', () => { + const areas = [ + { pattern: '/src/', label: 'src', owners: ['alice'] }, + { pattern: '/src/H5FDsubfiling/', label: 'src/H5FDsubfiling', owners: ['bob'] }, + ]; + const files = [{ filename: 'src/H5public.h', changes: 5 }]; + const byArea = attributeFiles(files, areas); + assert.strictEqual(byArea.get('/src/').length, 1); + assert.strictEqual(byArea.get('/src/H5FDsubfiling/').length, 0); +}); + +test('attributeFiles: unmatched file appears in no area', () => { + const areas = [{ pattern: '/src/', label: 'src', owners: ['alice'] }]; + const files = [{ filename: 'fortran/H5f.F90', changes: 3 }]; + const byArea = attributeFiles(files, areas); + assert.strictEqual(byArea.get('/src/').length, 0); +}); + +// ---------------------------------------------------------------- +// computeApprovals +// ---------------------------------------------------------------- + +test('computeApprovals: basic approval', () => { + const approved = computeApprovals([{ user: { login: 'alice' }, state: 'APPROVED' }]); + assert.ok(approved.has('alice')); +}); + +test('computeApprovals: CHANGES_REQUESTED after APPROVED cancels approval', () => { + const approved = computeApprovals([ + { user: { login: 'alice' }, state: 'APPROVED' }, + { user: { login: 'alice' }, state: 'CHANGES_REQUESTED' }, + ]); + assert.strictEqual(approved.has('alice'), false); +}); + +test('computeApprovals: DISMISSED after APPROVED cancels approval', () => { + const approved = computeApprovals([ + { user: { login: 'alice' }, state: 'APPROVED' }, + { user: { login: 'alice' }, state: 'DISMISSED' }, + ]); + assert.strictEqual(approved.has('alice'), false); +}); + +test('computeApprovals: COMMENTED after APPROVED does not cancel approval', () => { + const approved = computeApprovals([ + { user: { login: 'alice' }, state: 'APPROVED' }, + { user: { login: 'alice' }, state: 'COMMENTED' }, + ]); + assert.ok(approved.has('alice')); +}); + +test('computeApprovals: null user is skipped (ghost / deleted account)', () => { + const approved = computeApprovals([ + { user: null, state: 'APPROVED' }, + { user: { login: 'bob' }, state: 'APPROVED' }, + ]); + assert.ok(approved.has('bob')); + assert.strictEqual(approved.size, 1); +}); + +test('computeApprovals: independent approvals from two users', () => { + const approved = computeApprovals([ + { user: { login: 'alice' }, state: 'APPROVED' }, + { user: { login: 'bob' }, state: 'APPROVED' }, + ]); + assert.ok(approved.has('alice')); + assert.ok(approved.has('bob')); +}); + +// ---------------------------------------------------------------- +// chooseReviewers helpers +// ---------------------------------------------------------------- + +function makeArea(label, owners, linesChanged, files) { + return { pattern: `/${label}/`, label, owners, linesChanged, files: files || [] }; +} + +const BASE_CONFIG = { + prAuthor: 'charlie', + existingRequested: new Set(), + reviewerLoad: {}, + LINE_THRESHOLD: 300, + AREA_THRESHOLDS: {}, + PUBLIC_HEADER: /public\.h$/, +}; + +// ---------------------------------------------------------------- +// chooseReviewers +// ---------------------------------------------------------------- + +test('chooseReviewers: complex area (lines >= threshold) picks first non-author owner', () => { + const area = makeArea('src', ['alice', 'bob'], 400); + const { selected } = chooseReviewers([area], { ...BASE_CONFIG, prAuthor: 'bob' }); + assert.ok(selected.has('alice')); + assert.strictEqual(selected.has('bob'), false); +}); + +test('chooseReviewers: linesChanged === threshold is complex (boundary >=)', () => { + const area = makeArea('src', ['alice'], 300); + const { selected, log } = chooseReviewers([area], { ...BASE_CONFIG }); + assert.ok(selected.has('alice')); + assert.ok(log.some(l => l.includes('complex'))); +}); + +test('chooseReviewers: linesChanged === threshold - 1 is NOT complex', () => { + const area = makeArea('src', ['alice'], 299); + const { selected, log } = chooseReviewers([area], { ...BASE_CONFIG }); + assert.ok(selected.has('alice')); + assert.ok(!log.some(l => l.includes('complex'))); +}); + +test('chooseReviewers: public header triggers complexity regardless of line count', () => { + const area = makeArea('src', ['alice', 'bob'], 1, [{ filename: 'src/H5public.h', changes: 1 }]); + const { selected } = chooseReviewers([area], { ...BASE_CONFIG }); + assert.ok(selected.has('alice')); +}); + +test('chooseReviewers: per-area threshold override (test area at 400 lines is NOT complex at 500 threshold)', () => { + const area = makeArea('test', ['alice'], 400); + const { selected, log } = chooseReviewers([area], { + ...BASE_CONFIG, + AREA_THRESHOLDS: { test: 500 }, + }); + assert.ok(selected.has('alice')); + assert.ok(!log.some(l => l.includes('complex'))); +}); + +test('chooseReviewers: per-area threshold override (test area at 500 lines IS complex at 500 threshold)', () => { + const area = makeArea('test', ['alice'], 500); + const { selected, log } = chooseReviewers([area], { + ...BASE_CONFIG, + AREA_THRESHOLDS: { test: 500 }, + }); + assert.ok(selected.has('alice')); + assert.ok(log.some(l => l.includes('complex'))); +}); + +test('chooseReviewers: cohesion reuses already-selected owner for second area', () => { + const areas = [ + makeArea('src', ['alice', 'bob'], 10), + makeArea('test', ['alice', 'charlie'], 10), + ]; + const { selected } = chooseReviewers(areas, { ...BASE_CONFIG }); + // First area load-balances to alice (equal loads, alice is first). + // Second area reuses alice via cohesion instead of picking charlie. + assert.ok(selected.has('alice')); + assert.strictEqual(selected.has('charlie'), false); + assert.strictEqual(selected.size, 1); +}); + +test('chooseReviewers: load-balanced pick selects owner with fewer open requests', () => { + const area = makeArea('src', ['alice', 'bob'], 10); + const { selected } = chooseReviewers([area], { + ...BASE_CONFIG, + reviewerLoad: { alice: 5, bob: 2 }, + }); + assert.ok(selected.has('bob')); + assert.strictEqual(selected.has('alice'), false); +}); + +test('chooseReviewers: tie in load broken by CODEOWNERS order (first-listed wins)', () => { + const area = makeArea('src', ['alice', 'bob'], 10); + const { selected } = chooseReviewers([area], { + ...BASE_CONFIG, + reviewerLoad: { alice: 3, bob: 3 }, + }); + assert.ok(selected.has('alice')); +}); + +test('chooseReviewers: author-is-sole-owner produces empty selection without crash', () => { + const area = makeArea('src', ['alice'], 10); + const { selected, log } = chooseReviewers([area], { ...BASE_CONFIG, prAuthor: 'alice' }); + assert.strictEqual(selected.size, 0); + assert.ok(log.some(l => l.includes('all owners are the PR author'))); +}); + +test('chooseReviewers: area already in existingRequested is skipped', () => { + const area = makeArea('src', ['alice', 'bob'], 10); + const { selected } = chooseReviewers([area], { + ...BASE_CONFIG, + existingRequested: new Set(['alice']), + }); + assert.strictEqual(selected.size, 0); +}); + +test('chooseReviewers: updatedRequested contains both existing and newly selected', () => { + const area = makeArea('src', ['alice'], 10); + const { updatedRequested } = chooseReviewers([area], { + ...BASE_CONFIG, + existingRequested: new Set(['bob']), + }); + assert.ok(updatedRequested.has('bob')); + assert.ok(updatedRequested.has('alice')); +}); + +// ---------------------------------------------------------------- +// buildBody +// ---------------------------------------------------------------- + +test('buildBody: unchecked area shows open box and owner mention', () => { + const areas = [makeArea('src', ['alice'], 10)]; + const body = buildBody(areas, new Set(), new Set(['alice'])); + assert.ok(body.includes('- [ ] **src**')); + assert.ok(body.includes('— @alice')); + assert.ok(!body.includes('✅')); +}); + +test('buildBody: approved area shows checked box and tick', () => { + const areas = [makeArea('src', ['alice'], 10)]; + const body = buildBody(areas, new Set(['alice']), new Set(['alice'])); + assert.ok(body.includes('- [x] **src** ✅')); +}); + +test('buildBody: all areas done appends global sign-off line', () => { + const areas = [makeArea('src', ['alice'], 10)]; + const body = buildBody(areas, new Set(['alice']), new Set(['alice'])); + assert.ok(body.includes('> ✅ All areas have been signed off.')); +}); + +test('buildBody: partial approval does not show global sign-off line', () => { + const areas = [ + makeArea('src', ['alice'], 10), + makeArea('test', ['bob'], 10), + ]; + const body = buildBody(areas, new Set(['alice']), new Set(['alice', 'bob'])); + assert.ok(!body.includes('> ✅ All areas have been signed off.')); +}); + +test('buildBody: area with no confirmed reviewer shows no @-mention', () => { + const areas = [makeArea('src', ['alice'], 10)]; + const body = buildBody(areas, new Set(), new Set()); + assert.ok(body.includes('- [ ] **src**')); + assert.ok(!body.includes('@alice')); +}); + +test('buildBody: always contains the marker', () => { + const areas = [makeArea('src', ['alice'], 10)]; + const body = buildBody(areas, new Set(), new Set()); + assert.ok(body.includes('')); +}); + +// ---------------------------------------------------------------- +// Summary +// ---------------------------------------------------------------- + +console.log(''); +console.log(`${passed} passed, ${failed} failed`); +process.exit(failed > 0 ? 1 : 0); diff --git a/.github/workflows/review-checklist-test.yml b/.github/workflows/review-checklist-test.yml new file mode 100644 index 00000000000..0a865607ad4 --- /dev/null +++ b/.github/workflows/review-checklist-test.yml @@ -0,0 +1,23 @@ +name: Review Checklist Tests + +on: + push: + paths: + - .github/scripts/review-checklist.js + - .github/scripts/review-checklist.test.js + pull_request: + branches: [develop] + paths: + - .github/scripts/review-checklist.js + - .github/scripts/review-checklist.test.js + +permissions: + contents: read + +jobs: + test: + runs-on: ubuntu-latest + if: github.repository == 'HDFGroup/hdf5' && (github.event_name == 'push' || github.event.pull_request.head.repo.full_name == github.repository) + steps: + - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4 + - run: node .github/scripts/review-checklist.test.js diff --git a/.github/workflows/review-checklist.yml b/.github/workflows/review-checklist.yml new file mode 100644 index 00000000000..097f8702762 --- /dev/null +++ b/.github/workflows/review-checklist.yml @@ -0,0 +1,43 @@ +name: Review Checklist + +# Posts a per-area sign-off checklist on every PR and auto-checks each item +# when one of that area's designated owners submits an approval. +# +# Reviewer lists are derived entirely from .github/CODEOWNERS — no duplication. +# To add an area or change owners, edit only CODEOWNERS. + +on: + pull_request: + types: [opened, synchronize, reopened] + branches: [develop] + pull_request_review: + types: [submitted] + +concurrency: + group: review-checklist-${{ github.event.pull_request.number }} + cancel-in-progress: true + +permissions: + pull-requests: write + issues: write + contents: read + +jobs: + checklist: + runs-on: ubuntu-latest + # Only run on PRs targeting develop from within the same repo (not forks). + # For review events, only run on approvals targeting develop. + if: | + github.event.pull_request.base.ref == 'develop' && + github.event.pull_request.head.repo.full_name == github.repository && + (github.event_name == 'pull_request' || + (github.event_name == 'pull_request_review' && + github.event.review.state == 'approved')) + + steps: + - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4 + - uses: actions/github-script@f28e40c7f34bde8b3046d885e986cb6290c5673b # v7 + with: + script: | + const run = require('./.github/scripts/review-checklist.js'); + await run({ github, context, core });