From 1a75ba2787083387c6900f110aaf1f5c37145506 Mon Sep 17 00:00:00 2001 From: hk2166 <9610hemant@gmail.com> Date: Sun, 24 May 2026 11:01:38 +0530 Subject: [PATCH] ci: automate PR feedback when CI lint/build/test checks fail Re-implements the PR-helper CI feedback flow as a scheduled cron driver instead of a workflow_run listener. The previous workflow_run-based approach (PR #1637) was reverted in PR #1638 because workflow_run was flagged as an unsafe trigger pattern. The new on-ci-result workflow runs every 10 minutes, lists open PRs, and for each one looks up the latest "PR Checks" run by head SHA. It updates the dashboard CI section, force-swaps the status label to "needs revision" on failure, and posts a one-time @mention notification on clean->failure transitions. Runs entirely in base-repo context, so fork PRs are handled without exposing any fork-controlled input plane. Signed-off-by: hk2166 <9610hemant@gmail.com> --- .github/scripts/bot-on-ci-result.js | 178 ++++++++++++++++++++++++++++ .github/scripts/helpers/api.js | 4 +- .github/scripts/helpers/ci.js | 178 ++++++++++++++++++++++++++++ .github/scripts/helpers/comments.js | 68 +++++++++-- .github/scripts/helpers/index.js | 2 + .github/workflows/on-ci-result.yaml | 76 ++++++++++++ 6 files changed, 493 insertions(+), 13 deletions(-) create mode 100644 .github/scripts/bot-on-ci-result.js create mode 100644 .github/scripts/helpers/ci.js create mode 100644 .github/workflows/on-ci-result.yaml diff --git a/.github/scripts/bot-on-ci-result.js b/.github/scripts/bot-on-ci-result.js new file mode 100644 index 000000000..563c5c406 --- /dev/null +++ b/.github/scripts/bot-on-ci-result.js @@ -0,0 +1,178 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// bot-on-ci-result.js +// +// Runs on a cron schedule. For each open PR: +// 1. Look up the latest "PR Checks" workflow run by head SHA. +// 2. If completed, classify the result (lint / build / tests / pass). +// 3. Re-render the dashboard comment with the CI section. +// 4. On failure, force-swap the status label to needs-revision and post a +// one-time @mention ping if the prior dashboard didn't already show a +// failure (transition into failure). +// +// This driver runs entirely in the base-repo context, so fork PRs are handled +// without any input from the fork — replacing the previous workflow_run-based +// driver that GitHub flagged as an unsafe trigger pattern. + +const { + createLogger, + swapStatusLabel, + runAllChecksAndComment, + getBotComment, + postComment, + buildCIFailureNotificationComment, + getLatestPRChecksRun, + classifyWorkflowFailure, + hadPriorCIFailure, + MARKER, +} = require('./helpers'); + +const logger = createLogger('on-ci-result'); + +/** + * Lists every open PR in the repository, paginating through results. + */ +async function listAllOpenPRs(github, owner, repo) { + const prs = []; + let page = 1; + const perPage = 100; + + while (true) { + const { data } = await github.rest.pulls.list({ + owner, + repo, + state: 'open', + per_page: perPage, + page, + }); + + prs.push(...data); + + if (data.length < perPage) break; + page++; + } + + return prs; +} + +/** + * Reconciles a single PR's CI dashboard / label / ping state. + * Returns one of: 'no-run', 'no-change', 'success', 'failure'. + */ +async function reconcilePR({ github, owner, repo, pr }) { + const prNumber = pr.number; + const headSha = pr.head?.sha; + + if (!headSha) { + logger.log(`PR #${prNumber}: missing head SHA, skipping`); + return 'no-run'; + } + + const workflowRun = await getLatestPRChecksRun(github, owner, repo, headSha); + if (!workflowRun) { + logger.log(`PR #${prNumber}: no completed PR Checks run for ${headSha}, skipping`); + return 'no-run'; + } + + const ciResult = await classifyWorkflowFailure(github, workflowRun, owner, repo); + logger.log(`PR #${prNumber}: failed=${ciResult.failed}, check=${ciResult.check}`); + + const botContext = { + github, + owner, + repo, + number: prNumber, + pr, + eventType: 'schedule', + }; + + // Read prior dashboard before re-rendering, so we can detect a clean→failure + // transition (only point where we ping the author). + const priorComment = await getBotComment(botContext, MARKER); + const hadPriorFailure = hadPriorCIFailure(priorComment?.body); + + const ci = ciResult.failed + ? { passed: false, check: ciResult.check, runUrl: ciResult.runUrl } + : { passed: true }; + + await runAllChecksAndComment(botContext, { ci }); + + if (!ciResult.failed) { + // On CI success the success-path label management is owned by + // bot-on-pr-update; we leave the label untouched here. + return 'success'; + } + + // Force-swap to needs-revision on any CI failure, even if no prior label + // was present. + const swapResult = await swapStatusLabel(botContext, false, { force: true }); + if (!swapResult.success) { + logger.error(`PR #${prNumber}: failed to swap status label: ${swapResult.errorDetails}`); + } + + // Only ping on the transition into failure — repeat failures stay quiet. + if (!hadPriorFailure) { + const prAuthor = pr.user?.login; + if (prAuthor) { + const body = buildCIFailureNotificationComment(prAuthor, ciResult.runUrl); + const postResult = await postComment(botContext, body); + if (!postResult.success) { + logger.error(`PR #${prNumber}: failed to post notification: ${postResult.error}`); + } else { + logger.log(`PR #${prNumber}: posted CI failure notification`); + } + } + } else { + logger.log(`PR #${prNumber}: prior CI failure detected, skipping notification`); + } + + return 'failure'; +} + +module.exports = async ({ github, context }) => { + try { + const owner = context.repo.owner; + const repo = context.repo.repo; + + const allOpenPRs = await listAllOpenPRs(github, owner, repo); + logger.log(`Found ${allOpenPRs.length} open PR(s)`); + + const counts = { processed: 0, noRun: 0, success: 0, failure: 0, skipped: 0, errors: 0 }; + + for (const pr of allOpenPRs) { + // Skip drafts — they aren't ready for CI feedback yet. + if (pr.draft) { + counts.skipped++; + continue; + } + // Skip bot-authored PRs (matches the on-pr-update bot behaviour). + if (pr.user?.type === 'Bot') { + counts.skipped++; + continue; + } + + try { + const outcome = await reconcilePR({ github, owner, repo, pr }); + counts.processed++; + if (outcome === 'no-run') counts.noRun++; + else if (outcome === 'success') counts.success++; + else if (outcome === 'failure') counts.failure++; + } catch (err) { + counts.errors++; + logger.error(`PR #${pr.number}: reconcile failed: ${err.message}`); + // Continue with the next PR; one bad PR shouldn't kill the tick. + } + } + + logger.log( + `On-CI-result tick complete: processed=${counts.processed}, ` + + `noRun=${counts.noRun}, success=${counts.success}, failure=${counts.failure}, ` + + `skipped=${counts.skipped}, errors=${counts.errors}` + ); + + return counts; + } catch (error) { + logger.error('Error:', { message: error.message }); + throw error; + } +}; diff --git a/.github/scripts/helpers/api.js b/.github/scripts/helpers/api.js index a3769eafb..28f3fa023 100644 --- a/.github/scripts/helpers/api.js +++ b/.github/scripts/helpers/api.js @@ -622,7 +622,7 @@ async function acknowledgeComment(botContext, commentId) { * @returns {Promise<{ allPassed: boolean }>} */ async function runAllChecksAndComment(botContext, precomputed = {}) { - let { dco, gpg, merge, issueLink } = precomputed; + let { dco, gpg, merge, issueLink, ci } = precomputed; let commits = []; try { @@ -654,7 +654,7 @@ async function runAllChecksAndComment(botContext, precomputed = {}) { } const prAuthor = botContext.pr?.user?.login; - const { marker, body, allPassed } = buildBotComment({ prAuthor, dco, gpg, merge, issueLink }); + const { marker, body, allPassed } = buildBotComment({ prAuthor, dco, gpg, merge, issueLink, ci }); await postOrUpdateComment(botContext, marker, body); return { allPassed }; diff --git a/.github/scripts/helpers/ci.js b/.github/scripts/helpers/ci.js new file mode 100644 index 000000000..7e08dc337 --- /dev/null +++ b/.github/scripts/helpers/ci.js @@ -0,0 +1,178 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// helpers/ci.js +// +// CI-related helpers for the cron-driven on-ci-result bot. The bot iterates +// open PRs and reconciles each one's dashboard against the latest "PR Checks" +// run for that PR's head SHA. These helpers do the lookup and classification. + +const { getLogger } = require('./logger'); +const { requireObject, requireNonEmptyString } = require('./validation'); + +const PR_CHECKS_WORKFLOW_NAME = 'PR Checks'; + +/** + * Looks up the latest "PR Checks" workflow run for a given head SHA. + * Returns null if no run has been recorded yet, or if the latest run is still + * in progress / queued (the caller should skip those PRs until the run + * finishes — we don't want to flap the dashboard mid-run). + * + * Runs in the base-repository context, so this works for fork PRs without + * needing any input from the fork. + * + * @param {object} github - Octokit GitHub API client. + * @param {string} owner - Base-repo owner. + * @param {string} repo - Base-repo name. + * @param {string} headSha - PR head commit SHA. + * @returns {Promise} - Latest completed workflow run, or null. + */ +async function getLatestPRChecksRun(github, owner, repo, headSha) { + try { + requireNonEmptyString(owner, 'owner'); + requireNonEmptyString(repo, 'repo'); + requireNonEmptyString(headSha, 'headSha'); + + const { data } = await github.rest.actions.listWorkflowRunsForRepo({ + owner, + repo, + head_sha: headSha, + per_page: 100, + }); + + const runs = (data.workflow_runs || []).filter( + run => run.name === PR_CHECKS_WORKFLOW_NAME + ); + + if (runs.length === 0) { + return null; + } + + // listWorkflowRunsForRepo returns newest-first by default, but be explicit. + runs.sort((a, b) => new Date(b.run_started_at || b.created_at) - new Date(a.run_started_at || a.created_at)); + const latest = runs[0]; + + if (latest.status !== 'completed') { + getLogger().log(`Latest PR Checks run for ${headSha} is ${latest.status}, skipping`); + return null; + } + + return latest; + } catch (error) { + getLogger().error(`Failed to look up PR Checks run for ${headSha}: ${error.message}`); + return null; + } +} + +/** + * Classifies a workflow run's failure by inspecting job and step results. + * Returns the type of failure (lint, build, or tests) and the run URL. + * + * @param {object} github - Octokit GitHub API client. + * @param {object} workflowRun - Workflow run object (from listWorkflowRunsForRepo). + * @param {string} owner - Repository owner. + * @param {string} repo - Repository name. + * @returns {Promise<{ failed: boolean, check: string|null, runUrl: string }>} + */ +async function classifyWorkflowFailure(github, workflowRun, owner, repo) { + const runUrl = workflowRun.html_url; + + try { + requireObject(workflowRun, 'workflowRun'); + requireNonEmptyString(owner, 'owner'); + requireNonEmptyString(repo, 'repo'); + + if (workflowRun.conclusion === 'success') { + return { failed: false, check: null, runUrl }; + } + + if (workflowRun.conclusion === 'cancelled' || workflowRun.conclusion === 'skipped') { + return { failed: false, check: null, runUrl }; + } + + const { data: jobsData } = await github.rest.actions.listJobsForWorkflowRun({ + owner, + repo, + run_id: workflowRun.id, + per_page: 100, + }); + + const jobs = jobsData.jobs || []; + getLogger().log(`Found ${jobs.length} jobs for workflow run ${workflowRun.id}`); + + // Lint job is its own job, named "Lint" in zxc-build-library.yaml. + const lintJob = jobs.find(job => job.name && job.name.toLowerCase().includes('lint')); + if (lintJob && lintJob.conclusion === 'failure') { + getLogger().log('Classified failure as: lint'); + return { failed: true, check: 'lint', runUrl }; + } + + // Build job runs CMake then CTest. Distinguish build vs tests by which + // step failed. + const buildJob = jobs.find(job => + job.name && ( + job.name.toLowerCase().includes('build') || + job.name.toLowerCase().includes('code') + ) + ); + + if (buildJob && buildJob.conclusion === 'failure') { + const buildSteps = buildJob.steps || []; + + const buildStepFailed = buildSteps.some(step => + step.name && + (step.name.toLowerCase().includes('cmake build') || + step.name.toLowerCase().includes('build project')) && + step.conclusion === 'failure' + ); + + if (buildStepFailed) { + getLogger().log('Classified failure as: build'); + return { failed: true, check: 'build', runUrl }; + } + + const testStepFailed = buildSteps.some(step => + step.name && + (step.name.toLowerCase().includes('ctest') || + step.name.toLowerCase().includes('test')) && + step.conclusion === 'failure' + ); + + if (testStepFailed) { + getLogger().log('Classified failure as: tests'); + return { failed: true, check: 'tests', runUrl }; + } + + getLogger().log('Build job failed but step unclear, defaulting to: build'); + return { failed: true, check: 'build', runUrl }; + } + + getLogger().log('Could not classify failure type, returning generic failure'); + return { failed: true, check: null, runUrl }; + } catch (error) { + getLogger().error(`Failed to classify workflow failure: ${error.message}`); + return { failed: true, check: null, runUrl }; + } +} + +/** + * Checks if the prior dashboard comment already shows a CI failure. Used to + * detect state transitions (no failure → failure) so we only post the + * @mention notification once per failure cycle. + * + * @param {string|null} priorCommentBody - Body of the existing dashboard comment, or null. + * @returns {boolean} + */ +function hadPriorCIFailure(priorCommentBody) { + if (!priorCommentBody) return false; + + // Match either ":x: **CI Checks**" or "**CI Checks** ... :x:" + const ciFailurePattern = /:x:.*?\*\*CI Checks\*\*|\*\*CI Checks\*\*.*?:x:/s; + return ciFailurePattern.test(priorCommentBody); +} + +module.exports = { + PR_CHECKS_WORKFLOW_NAME, + getLatestPRChecksRun, + classifyWorkflowFailure, + hadPriorCIFailure, +}; diff --git a/.github/scripts/helpers/comments.js b/.github/scripts/helpers/comments.js index 27e192bfe..38bcd75af 100644 --- a/.github/scripts/helpers/comments.js +++ b/.github/scripts/helpers/comments.js @@ -138,12 +138,28 @@ function buildIssueLinkSection(issueLink) { } /** - * Builds the ### PR Checks section of the dashboard comment. - * @param {{ dco: object, gpg: object, merge: object, issueLink: object }} results + * @param {{ passed: boolean, check?: string, runUrl?: string, error?: boolean, errorMessage?: string }} ci * @returns {string} */ -function buildChecksSection({ dco, gpg, merge, issueLink }) { +function buildCISection(ci) { + const common = buildSection({ title: 'CI Checks', result: ci, passMessage: 'All CI checks passed. Nice work!' }); + if (common) return common; + + const checkName = ci.check === 'lint' ? 'Lint' : ci.check === 'build' ? 'Build' : ci.check === 'tests' ? 'Tests' : 'CI'; return [ + `:x: **CI Checks** -- The ${checkName} check failed.`, + '', + `Take a look at the [failing run](${ci.runUrl}) and push a fix when you're ready. Feel free to ping the maintainers if you need a hand.`, + ].join('\n'); +} + +/** + * Builds the ### PR Checks section of the dashboard comment. + * @param {{ dco: object, gpg: object, merge: object, issueLink: object, ci?: object }} results + * @returns {string} + */ +function buildChecksSection({ dco, gpg, merge, issueLink, ci }) { + const sections = [ '### PR Checks', '', buildDCOSection(dco), @@ -159,39 +175,52 @@ function buildChecksSection({ dco, gpg, merge, issueLink }) { '---', '', buildIssueLinkSection(issueLink), - ].join('\n'); + ]; + + if (ci) { + sections.push('', '---', '', buildCISection(ci)); + } + + return sections.join('\n'); } /** * Determines whether all checks passed (errors count as not passed). - * @param {{ dco: object, gpg: object, merge: object, issueLink: object }} results + * @param {{ dco: object, gpg: object, merge: object, issueLink: object, ci?: object }} results * @returns {boolean} */ -function allChecksPassed({ dco, gpg, merge, issueLink }) { - return ( +function allChecksPassed({ dco, gpg, merge, issueLink, ci }) { + const baseChecks = ( !dco.error && dco.passed && !gpg.error && gpg.passed && !merge.error && merge.passed && !issueLink.error && issueLink.passed ); + + if (!ci) return baseChecks; + + return baseChecks && !ci.error && ci.passed; } /** * Builds the full unified bot comment. - * @param {{ prAuthor: string, dco: object, gpg: object, merge: object, issueLink: object }} params + * @param {{ prAuthor: string, dco: object, gpg: object, merge: object, issueLink: object, ci?: object }} params * @returns {{ marker: string, body: string, allPassed: boolean }} */ -function buildBotComment({ prAuthor, dco, gpg, merge, issueLink }) { +function buildBotComment({ prAuthor, dco, gpg, merge, issueLink, ci }) { const greeting = [ `Hey @${prAuthor} :wave: thanks for the PR!`, "I'm your friendly **PR Helper Bot** :robot: and I'll be riding shotgun on this one, keeping track of your PR's status to help you get it approved and merged.", '', "This comment updates automatically as you push changes -- think of it as your PR's live scoreboard!", + '', + 'When all checks pass, your PR will be labeled **status: needs review** and a maintainer will take a look. If something needs attention, the label will flip to **status: needs revision** so you know to take another pass.', + '', "Here's the latest:", ].join('\n'); - const checksSection = buildChecksSection({ dco, gpg, merge, issueLink }); - const passed = allChecksPassed({ dco, gpg, merge, issueLink }); + const checksSection = buildChecksSection({ dco, gpg, merge, issueLink, ci }); + const passed = allChecksPassed({ dco, gpg, merge, issueLink, ci }); const footer = passed ? ':tada: *All checks passed! Your PR is ready for review. Great job!*' @@ -201,10 +230,27 @@ function buildBotComment({ prAuthor, dco, gpg, merge, issueLink }) { return { marker: MARKER, body, allPassed: passed }; } +/** + * Builds a standalone notification comment to alert a PR author that CI has failed. + * This is posted once when the CI state changes from pass/unknown to failed. + * + * @param {string} prAuthor - GitHub username of the PR author. + * @param {string} runUrl - URL to the failing CI run. + * @returns {string} + */ +function buildCIFailureNotificationComment(prAuthor, runUrl) { + return [ + `Hi @${prAuthor} :wave: — the CI check just failed on this PR.`, + `Take a look at the [failing run](${runUrl}) and push a fix when you're ready.`, + `Feel free to ping the maintainers if you need a hand.`, + ].join(' '); +} + module.exports = { MARKER, buildBotComment, buildChecksSection, allChecksPassed, buildMergeConflictNotificationComment, + buildCIFailureNotificationComment, }; diff --git a/.github/scripts/helpers/index.js b/.github/scripts/helpers/index.js index 867471157..6d07837bd 100644 --- a/.github/scripts/helpers/index.js +++ b/.github/scripts/helpers/index.js @@ -11,6 +11,7 @@ const validation = require('./validation'); const api = require('./api'); const checks = require('./checks'); const comments = require('./comments'); +const ci = require('./ci'); module.exports = { ...constants, @@ -19,4 +20,5 @@ module.exports = { ...api, ...checks, ...comments, + ...ci, }; diff --git a/.github/workflows/on-ci-result.yaml b/.github/workflows/on-ci-result.yaml new file mode 100644 index 000000000..0b909fb59 --- /dev/null +++ b/.github/workflows/on-ci-result.yaml @@ -0,0 +1,76 @@ +name: Bot - On CI Result + +# ────────────────────────────────────────────────────────────────────── +# Workflow: Bot - On CI Result +# +# Purpose: +# Runs every 10 minutes to reconcile open PRs against their latest +# "PR Checks" workflow run. For each open PR: +# - Looks up the latest "PR Checks" run by head SHA +# - Updates the dashboard CI section +# - On CI failure: force-swaps the status label to "needs revision" +# and posts a one-time @mention notification (only on the clean +# → failure transition; repeat failures stay quiet) +# +# Trigger choice: +# Originally implemented as a `workflow_run` listener, which the +# maintainers flagged as unsafe. This scheduled cron driver runs +# entirely in the base-repo context, so no fork-PR input plane is +# exposed and the security objection is resolved. Cost is up to ~10 +# minutes of latency before the dashboard reflects a CI result, which +# is acceptable for a status indicator. +# +# Concurrency: +# Serialized — only one tick runs at a time so two overlapping ticks +# can't double-post the notification comment. +# ────────────────────────────────────────────────────────────────────── +on: + schedule: + - cron: '*/10 * * * *' # Every 10 minutes + workflow_dispatch: # Manual trigger for testing + +defaults: + run: + shell: bash + +permissions: + contents: read # Checkout repository for bot scripts + pull-requests: write # Update dashboard comment, swap status label + issues: write # Add/remove labels (PRs are issues for label APIs), post comments + actions: read # List workflow runs and jobs + +jobs: + reconcile: + name: Reconcile CI Results + runs-on: hiero-client-sdk-linux-large + + concurrency: + group: on-ci-result + cancel-in-progress: false + + steps: + - name: Harden Runner + uses: step-security/harden-runner@9ca718d3bf646d6534007c269a635b3e54cadf99 # v2.19.2 + with: + egress-policy: audit + + - name: Checkout Repository + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + ref: ${{ github.event.repository.default_branch }} + + - name: Setup Node.js + uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0 + with: + node-version: 22 + + - name: Install Dependencies + working-directory: .github/scripts + run: npm ci + + - name: Run Bot + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 + with: + script: | + const bot = require('./.github/scripts/bot-on-ci-result.js'); + await bot({ github, context });