From b9f82ed39f42e6427b1cb1dba68159c0d50df0a3 Mon Sep 17 00:00:00 2001 From: ethernet Date: Thu, 16 Jul 2026 18:17:49 -0400 Subject: [PATCH] ci: live-updating PR review comment with structured job statuses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the static comment-pending + comment-results two-job pattern with a live-updating comment system that polls the GitHub Actions API every 15s, re-assembles the review comment from whatever results are available, and upserts it via the marker. The comment updates in real time as each job finishes — no waiting for the full pipeline. Every CI job that wants to appear in the review comment emits a review_status output — a JSON array of objects, each with a source and a results array: [ { "source": "review-label-gate", "results": [ {"kind": "action_required", "title": "...", "summary": "...", "how_to_fix": "..."}, {"kind": "info", "title": "...", "summary": "..."} ] }, { "source": "ci timing", "results": [ {"kind": "warning", "title": "CI timings", "summary": "...", "detail": "...", "link": "..."} ] } ] One job can emit multiple results of different kinds. The source field is used to exclude the corresponding job from the synthesized error list (case-insensitive, hyphen-normalized matching against GitHub Actions job display names). | job | source | kind (on failure) | section | |----------------------------|--------------------------|---------------------------|----------------------| | review-labels | review label gate | action_required / info | Action required | | lockfile-diff | lockfile-diff | action_required | Action required | | ci-timings | ci timing | warning / info | Warnings | | supply-chain scan | supply chain | error / (none) | Job failures | | supply-chain dep-bounds | supply chain | action_required / (none) | Action required | | osv-scanner | osv scan | warning / (none) | Warnings | | uv-lockfile-check | uv.lock check | action_required / (none) | Action required | | history-check | unrelated histories | action_required | Action required | | contributor-check | contributor attribution | action_required | Action required | Jobs that find nothing emit [] (empty array) — no noise info items. A single comment-live job polls the GitHub Actions API every 15s, classifies jobs into (completed, pending), assembles the comment, and upserts it. Merges review_status outputs from all needs jobs via toJSON(needs.*.outputs.review_status), and downloads the ci-timings artifact when it becomes available. Shows commit SHA + message below the header. The assembler has ZERO job-specific knowledge. It just: 1. collect_from_statuses() — flattens all nested status objects into ReviewItems 2. collect_failed_jobs() — synthesizes errors for failed jobs with no declared status 3. _attach_job_urls() — fills in per-job log links for ALL items 4. render_comment() — groups by severity, renders with group headers Each item shows links inline next to the title: View report (job-emitted URL) and View job (auto-attached logs link). Each info item is its own collapsible
block. # ૮ >ﻌ< ა ci review running on abc1234 — commit message first line ## ❌ Job failures ### {title} · [View job](url) {summary} ## ⚠️ Action required ### {title} · [View job](url) {summary} **How to fix:** {how_to_fix} ## ⚠️ Warnings ### {title} · [View report](url) · [View job](url) {summary} {detail}
{title} {content}
Still running 3 jobs: ci-timings, docker - test_assemble_review_comment.py (48 tests): collect_from_statuses, collect_failed_jobs with exclude_sources, _attach_job_urls, render_comment (group headers, inline links, commit info, per-item details, pending footer), assemble integration - test_live_comment.py (16 tests): classify_jobs pure function - test_timings_report.py (10 tests): generate_review_status nested format - test_lockfile_diff.py (6 tests) - test_classify_changes.py (32 tests, pre-existing) --- .github/workflows/ci.yml | 116 +++- .github/workflows/contributor-check.yml | 18 + .github/workflows/history-check.yml | 12 +- .github/workflows/label-rerun.yml | 81 +++ .github/workflows/lint.yml | 126 +---- .github/workflows/lockfile-diff.yml | 81 ++- .github/workflows/osv-scanner.yml | 139 +++-- .github/workflows/review-labels.yml | 92 ++++ .github/workflows/supply-chain-audit.yml | 168 +++--- .github/workflows/uv-lockfile-check.yml | 11 + scripts/ci/assemble_review_comment.py | 424 +++++++++++++++ scripts/ci/emit_review_status.py | 143 +++++ scripts/ci/live_comment.py | 526 ++++++++++++++++++ scripts/ci/lockfile_diff.py | 34 +- scripts/ci/timings_report.py | 93 ++++ tests/ci/test_assemble_review_comment.py | 643 +++++++++++++++++++++++ tests/ci/test_live_comment.py | 162 ++++++ tests/ci/test_lockfile_diff.py | 7 +- tests/ci/test_timings_report.py | 143 +++++ 19 files changed, 2696 insertions(+), 323 deletions(-) create mode 100644 .github/workflows/label-rerun.yml create mode 100644 .github/workflows/review-labels.yml create mode 100644 scripts/ci/assemble_review_comment.py create mode 100644 scripts/ci/emit_review_status.py create mode 100644 scripts/ci/live_comment.py create mode 100644 tests/ci/test_assemble_review_comment.py create mode 100644 tests/ci/test_live_comment.py create mode 100644 tests/ci/test_timings_report.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c9dd08790778..96ff02597f0e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -17,7 +17,7 @@ on: permissions: contents: read - pull-requests: write # needed by lint (PR comment) + supply-chain (PR comment) + pull-requests: write # needed by lint (PR comment) + supply-chain review_status actions: read # needed by osv-scanner (SARIF upload) security-events: write # needed by osv-scanner (SARIF upload) packages: write # needed by docker build @@ -73,11 +73,10 @@ jobs: lint: name: Python lints needs: detect - if: needs.detect.outputs.python == 'true' || needs.detect.outputs.ci_review == 'true' + if: needs.detect.outputs.python == 'true' uses: ./.github/workflows/lint.yml with: event_name: ${{ needs.detect.outputs.event_name }} - ci_review: ${{ needs.detect.outputs.ci_review == 'true' }} secrets: inherit js-tests: @@ -144,12 +143,20 @@ jobs: supply-chain: name: Supply-chain scan needs: detect - if: needs.detect.outputs.event_name == 'pull_request' && (needs.detect.outputs.scan == 'true' || needs.detect.outputs.deps == 'true' || needs.detect.outputs.mcp_catalog == 'true') + if: needs.detect.outputs.event_name == 'pull_request' && (needs.detect.outputs.scan == 'true' || needs.detect.outputs.deps == 'true') uses: ./.github/workflows/supply-chain-audit.yml with: event_name: ${{ needs.detect.outputs.event_name }} scan: ${{ needs.detect.outputs.scan == 'true' }} deps: ${{ needs.detect.outputs.deps == 'true' }} + + review-labels: + name: Review label gate + needs: detect + if: needs.detect.outputs.event_name == 'pull_request' && (needs.detect.outputs.ci_review == 'true' || needs.detect.outputs.mcp_catalog == 'true') + uses: ./.github/workflows/review-labels.yml + with: + ci_review: ${{ needs.detect.outputs.ci_review == 'true' }} mcp_catalog: ${{ needs.detect.outputs.mcp_catalog == 'true' }} secrets: inherit @@ -158,12 +165,90 @@ jobs: uses: ./.github/workflows/osv-scanner.yml secrets: inherit + # ───────────────────────────────────────────────────────────────────── + # Live-updating PR review comment. + # + # A single ``comment-live`` job polls the GitHub Actions API every 15s + # for job statuses in this run, re-assembles the review comment from + # whatever results are available, and upserts it via the + # ```` marker. + # + # The poller exits when all non-infra jobs are completed (or on + # timeout). ci-timings' review_status is picked up automatically when + # its artifact becomes available — the poller downloads and merges it. + # ───────────────────────────────────────────────────────────────────── + comment-live: + name: CI review comment (live) + needs: [detect, review-labels, lockfile-diff, supply-chain, osv-scanner, uv-lockfile, history-check, contributor-check] + if: always() && github.event_name == 'pull_request' && github.event.pull_request.head.repo.fork != true + runs-on: ubuntu-latest + timeout-minutes: 40 + steps: + - name: Checkout code + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + + - name: Run live comment poller + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + GITHUB_REPOSITORY: ${{ github.repository }} + GITHUB_RUN_ID: ${{ github.run_id }} + PR_NUMBER: ${{ github.event.pull_request.number }} + RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} + # Commit info for the review comment header. + COMMIT_SHA: ${{ github.event.pull_request.head.sha }} + COMMIT_MESSAGE: ${{ github.event.pull_request.head.commit.message }} + COMMIT_URL: ${{ github.server_url }}/${{ github.repository }}/pull/${{ github.event.pull_request.number }}/commits/${{ github.event.pull_request.head.sha }} + # Structured review statuses from workflow_call jobs. + # Each job outputs a JSON array of {source, results: [...]} objects + # that the assembler renders directly — no hardcoded job-name + # matching. We merge all available outputs into one array. + REVIEW_STATUSES: ${{ toJSON(needs.*.outputs.review_status) }} + run: | + set -uo pipefail + + # REVIEW_STATUSES is a JSON array of strings (some may be empty + # when a job was skipped). Parse each string and merge into one + # flat array for the assembler. + python3 - <<'PYEOF' + import json, os, sys + + raw = os.environ.get("REVIEW_STATUSES", "") + merged = [] + if raw: + try: + arr = json.loads(raw) + except (json.JSONDecodeError, TypeError): + arr = [] + for item in arr: + if not item: + continue + try: + statuses = json.loads(item) + except (json.JSONDecodeError, TypeError): + continue + if isinstance(statuses, list): + merged.extend(statuses) + + # Write merged array to a temp file the poller reads. + with open("/tmp/review_statuses.json", "w") as f: + json.dump(merged, f) + print(f"Merged {len(merged)} review status entries") + PYEOF + + python3 scripts/ci/live_comment.py \ + --interval 15 \ + --timeout 2100 \ + --review-statuses-file /tmp/review_statuses.json + # ───────────────────────────────────────────────────────────────────── # Gate: runs after everything. ``if: always()`` ensures it reports a # status even when some deps were skipped. Only actual ``failure`` # results cause it to fail; ``skipped`` is treated as success. # # Branch protection should require ONLY this check. + # + # Outputs ``needs-json`` — a compact ``{job_name: result}`` dict — so + # the live comment poller can list failed jobs in the PR comment. # ───────────────────────────────────────────────────────────────────── all-checks-pass: name: All required checks pass @@ -179,20 +264,30 @@ jobs: - lockfile-diff - docker-lint - supply-chain + - review-labels - osv-scanner + # comment-live is a polling job — it doesn't block the gate. # we don't require docker to pass rn because it's so slow lol # - docker if: always() runs-on: ubuntu-latest timeout-minutes: 10 + outputs: + needs-json: ${{ steps.evaluate.outputs.needs-json }} steps: - name: Evaluate job results + id: evaluate env: NEEDS: ${{ toJSON(needs) }} run: | echo "$NEEDS" | python3 -c " import json, sys needs = json.load(sys.stdin) + # Emit compact {job_name: result} for the comment assembler. + compact = {name: info['result'] for name, info in needs.items()} + print(f'needs-json={json.dumps(compact)}') + with open('$GITHUB_OUTPUT', 'a') as f: + f.write(f'needs-json={json.dumps(compact)}\n') failed = [name for name, info in needs.items() if info['result'] == 'failure'] for name, info in sorted(needs.items()): result = info['result'] @@ -209,6 +304,9 @@ jobs: # cache them on main (as a baseline), and on PRs generate an HTML diff # report with a gantt chart + per-step breakdown. The report is uploaded # as an artifact and a markdown summary is written to $GITHUB_STEP_SUMMARY. + # + # The live comment poller picks up ci-timings' completion automatically — + # it reads review-status.json from the artifact when the job finishes. # ───────────────────────────────────────────────────────────────────── ci-timings: name: CI timing report @@ -242,18 +340,20 @@ jobs: --baseline ci-timings-baseline.json \ --output ci-timings-report.html \ --json-out ci-timings.json \ - --summary-out ci-timings-summary.md + --summary-out ci-timings-summary.md \ + --review-status-out review-status.json - - name: Upload HTML report + - name: Upload HTML report + review status # Advisory report — artifact-service blips must not fail the job. continue-on-error: true uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7 id: ci-timings-artifact with: name: ci-timings-report - path: ci-timings-report.html + path: | + ci-timings-report.html + review-status.json retention-days: 14 - archive: false - name: Output summary env: diff --git a/.github/workflows/contributor-check.yml b/.github/workflows/contributor-check.yml index 2c5db6f311de..014e1e2ff93d 100644 --- a/.github/workflows/contributor-check.yml +++ b/.github/workflows/contributor-check.yml @@ -2,6 +2,10 @@ name: Contributor Attribution Check on: workflow_call: + outputs: + review_status: + description: "JSON array of review status objects" + value: ${{ jobs.check-attribution.outputs.review_status }} permissions: contents: read @@ -10,12 +14,15 @@ jobs: check-attribution: runs-on: ubuntu-latest timeout-minutes: 10 + outputs: + review_status: ${{ steps.check-emails.outputs.review_status }} steps: - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 with: fetch-depth: 0 # Full history needed for git log - name: Check for unmapped contributor emails + id: check-emails run: | # Get the merge base between this PR and main MERGE_BASE=$(git merge-base origin/main HEAD) @@ -25,6 +32,7 @@ jobs: if [ -z "$NEW_EMAILS" ]; then echo "No new commits to check." + echo "review_status=[]" >> "$GITHUB_OUTPUT" exit 0 fi @@ -67,6 +75,16 @@ jobs: echo "" echo "To find the GitHub username for an email:" echo " gh api 'search/users?q=EMAIL+in:email' --jq '.items[0].login'" + + # Emit review_status for unmapped emails + DETAIL=$(echo -e "$MISSING" | sed '/^$/d; s/^ //') + HOW_TO_FIX=$'Add mappings to scripts/release.py AUTHOR_MAP:\n```\n"": "",\n```\nTo find the GitHub username for an email:\n```\ngh api \'search/users?q=EMAIL+in:email\' --jq \'.items[0].login\'\n```\n' + REVIEW_STATUS=$(jq -nc \ + --arg detail "$DETAIL" \ + --arg how_to_fix "$HOW_TO_FIX" \ + '[{"source":"contributor attribution","results":[{"kind":"action_required","title":"Unmapped contributor email(s)","summary":"New contributor email(s) are not in AUTHOR_MAP.","detail":$detail,"how_to_fix":$how_to_fix}]}]') + echo "review_status=$REVIEW_STATUS" >> "$GITHUB_OUTPUT" + exit 1 else echo "✅ All contributor emails are mapped." diff --git a/.github/workflows/history-check.yml b/.github/workflows/history-check.yml index a48dba8cb8af..668f0f795dd9 100644 --- a/.github/workflows/history-check.yml +++ b/.github/workflows/history-check.yml @@ -15,6 +15,10 @@ name: History Check on: workflow_call: + outputs: + review_status: + description: "JSON array of review_status objects for the synthesizer." + value: ${{ jobs.check-common-ancestor.outputs.review_status }} permissions: contents: read @@ -23,18 +27,23 @@ jobs: check-common-ancestor: runs-on: ubuntu-latest timeout-minutes: 10 + outputs: + review_status: ${{ steps.merge-base-check.outputs.review_status }} steps: - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 with: fetch-depth: 0 # full history both sides for merge-base - - name: Reject PRs with no common ancestor on main + - id: merge-base-check + name: Reject PRs with no common ancestor on main run: | # `git merge-base` exits non-zero AND prints nothing when the two # commits share no ancestor. We check both conditions explicitly # so the failure message is clear regardless of which signal fires # first. if ! BASE=$(git merge-base origin/main HEAD 2>/dev/null) || [ -z "$BASE" ]; then + STATUS='[{"source":"unrelated histories","results":[{"kind":"action_required","title":"Unrelated histories","summary":"This PR has no common ancestor with main.","detail":"","how_to_fix":"Rebase your changes onto current main:\n```\ngit fetch origin main\ngit checkout -b fix-branch origin/main\n# re-apply your changes (cherry-pick, copy files, etc.)\ngit push -f origin fix-branch\n```\n"}]}]' + echo "review_status=${STATUS}" >> "$GITHUB_OUTPUT" echo "" echo "::error::This PR has no common ancestor with main." echo "" @@ -56,3 +65,4 @@ jobs: exit 1 fi echo "::notice::Common ancestor with main: $BASE" + echo "review_status=[]" >> "$GITHUB_OUTPUT" diff --git a/.github/workflows/label-rerun.yml b/.github/workflows/label-rerun.yml new file mode 100644 index 000000000000..fbd8b3b89327 --- /dev/null +++ b/.github/workflows/label-rerun.yml @@ -0,0 +1,81 @@ +name: Label rerun + +# When the ``ci-reviewed`` label is added to a PR, rerun all failed jobs in +# the latest CI run. This re-evaluates ``review-labels`` (which now sees the +# label) and GitHub automatically reruns dependent jobs (``comment-live``, +# ``all-checks-pass``) — so the review comment gets updated too. +# +# If the CI run is still in progress when the label is added, we wait for it +# to finish before rerunning (``gh run rerun`` only works on completed runs). +# The wait can be long (20+ min for a full CI run), but it's better than +# silently failing and leaving the reviewer stuck. + +on: + pull_request: + types: [labeled] + +permissions: + actions: write + pull-requests: read + +concurrency: + group: label-rerun-${{ github.event.pull_request.number }} + cancel-in-progress: true + +jobs: + rerun-review-labels: + name: Rerun review-labels job + if: github.event.label.name == 'ci-reviewed' + runs-on: ubuntu-latest + timeout-minutes: 40 + steps: + - name: Wait for CI run to finish, then rerun failed jobs + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + REPO: ${{ github.repository }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + run: | + set -uo pipefail + + # Find the latest CI run for this PR's head SHA. + RUN_ID=$(gh run list \ + --repo "$REPO" \ + --commit "$HEAD_SHA" \ + --workflow ci.yml \ + --limit 1 \ + --json databaseId,status \ + --jq '.[0] | "\(.databaseId) \(.status)"' 2>/dev/null || true) + + if [ -z "$RUN_ID" ]; then + echo "No CI run found for this PR — nothing to rerun." + exit 0 + fi + + # Split "RUN_ID STATUS" into two vars. + RUN_ID="${RUN_ID%% *}" + STATUS="${RUN_ID##* }" + + echo "Latest CI run: $RUN_ID (status: $STATUS)" + + # If the run is still in progress, wait for it to finish. + # gh run rerun only works on completed runs — if we try while it's + # running, GitHub rejects with "cannot be rerun; This workflow is + # already running". + if [ "$STATUS" != "completed" ]; then + echo "Run is $STATUS — waiting for completion (this may take a while)..." + # gh run watch --exit-status exits non-zero if the run fails, + # which is expected (the label gate fails). Don't let that kill + # the workflow — we WANT to rerun failed jobs. + timeout 2100 gh run watch "$RUN_ID" --repo "$REPO" --interval 15 || true + + # Verify it's actually completed now. + STATUS=$(gh run view "$RUN_ID" --repo "$REPO" --json status --jq '.status' 2>/dev/null || echo "unknown") + if [ "$STATUS" != "completed" ]; then + echo "Run is still $STATUS after wait — giving up." + exit 0 + fi + fi + + echo "Run completed. Rerunning all failed jobs..." + gh run rerun "$RUN_ID" --repo "$REPO" --failed || true + echo "Done. GitHub will rerun review-labels and all dependent jobs." diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 28df38ad3e5d..670b6f2a44a2 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -2,11 +2,14 @@ name: Lint (ruff + ty) # Two things here: # 1. Advisory diff — ruff + ty diagnostics as a diff vs the target branch. -# Posts a Markdown summary and a PR comment. Exit zero always. +# Writes a Markdown summary to the run page. Exit zero always. # 2. Blocking ``ruff check .`` — enforces the explicit rules in # ``[tool.ruff.lint.select]`` (currently PLW1514). Failure blocks merge. -# Separate job so the advisory diff still runs and posts even when -# enforcement fails. +# Separate job so the advisory diff still runs even when enforcement +# fails. +# +# CI-sensitive file review was previously here as a ``ci-review`` job but +# has moved to ``review-labels.yml`` so it can be rerun independently. on: workflow_call: @@ -15,14 +18,9 @@ on: description: The event name from the calling orchestrator (pull_request or push). type: string required: true - ci_review: - description: Whether CI-sensitive files (eslint config, workflows, actions) changed and require a review label. - type: boolean - default: false permissions: contents: read - pull-requests: write # needed to post/update PR comments concurrency: group: lint-${{ github.ref }} @@ -162,115 +160,3 @@ jobs: - name: Run footgun checker run: python scripts/check-windows-footguns.py --all - - ci-review: - # Require explicit maintainer review when CI-sensitive files change: - # eslint config, workflow YAMLs, or composite actions. These files - # influence what code the js-autofix job executes and pushes to - # main, so a malicious PR could inject arbitrary code via a custom eslint - # rule's `fix` function. The label gate ensures a human reviews before - # merge. Mirrors the mcp-catalog-reviewed pattern in supply-chain-audit.yml. - name: CI-sensitive file review - if: inputs.event_name == 'pull_request' && inputs.ci_review - runs-on: ubuntu-latest - timeout-minutes: 2 - steps: - - name: Checkout code - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 - - name: Require ci-reviewed label - id: label-check - env: - # Read-only label lookup. Use the built-in GITHUB_TOKEN (present and - # read-only on forks) so the gate works on fork PRs; fall back to it - # when AUTOFIX_BOT_PAT is empty. `|| true` degrades an API blip to - # "label absent" rather than hard-failing the step. - GH_TOKEN: ${{ secrets.AUTOFIX_BOT_PAT || github.token }} - run: | - set -euo pipefail - PR="${{ github.event.pull_request.number }}" - LABELS=$(gh pr view "$PR" --json labels --jq '.labels[].name' || true) - if echo "$LABELS" | grep -Fxq 'ci-reviewed'; then - echo "reviewed=true" >> "$GITHUB_OUTPUT" - echo "ci-reviewed label present." - exit 0 - fi - echo "reviewed=false" >> "$GITHUB_OUTPUT" - - # On failure: find the bot's previous comment and edit it, or create - # a new one if none exists. Using an HTML comment marker so we can - # locate it reliably across runs without parsing the body text. - # Skipped on fork PRs — GITHUB_TOKEN is read-only there, so the API - # call would fail. The label gate still holds via the step below. - - name: Post or update review warning - if: steps.label-check.outputs.reviewed != 'true' && github.event.pull_request.head.repo.fork != true - env: - GH_TOKEN: ${{ secrets.AUTOFIX_BOT_PAT || github.token }} - run: | - set -euo pipefail - PR="${{ github.event.pull_request.number }}" - MARKER="" - BODY="${MARKER} - ## ⚠️ CI-sensitive file review required - - This PR changes CI-sensitive files (eslint config, workflow YAMLs, - or composite actions). These files influence what code the - js-autofix job executes and pushes to main. - - A maintainer should verify: - - no new eslint rules with custom \`fix\` functions that write outside linted paths, - - no workflow changes that widen permissions or remove guards, - - no composite action changes that alter what gets executed. - - After review, add the \`ci-reviewed\` label and re-run this check." - - # Find an existing comment with our marker. - COMMENT_ID=$(gh api \ - "repos/${{ github.repository }}/issues/${PR}/comments" \ - --paginate --jq ".[] | select(.body | contains(\"${MARKER}\")) | .id" \ - | head -1 || true) - - if [ -n "$COMMENT_ID" ]; then - gh api --method PATCH \ - "repos/${{ github.repository }}/issues/comments/${COMMENT_ID}" \ - -f body="$BODY" - else - gh pr comment "$PR" --body "$BODY" - fi - - # Fail the job when the label is missing — always runs (including - # fork PRs) so the security gate holds even when the comment step - # was skipped above. - - name: Fail on missing label - if: steps.label-check.outputs.reviewed != 'true' - run: | - echo "::error::CI-sensitive changes require the ci-reviewed label." - exit 1 - - # On success: if a previous warning comment exists, edit it to show - # the review passed so the PR doesn't have a stale ⚠️ sitting around. - # Skipped on fork PRs — no comment was ever posted to update. - - name: Update previous warning to passed - if: steps.label-check.outputs.reviewed == 'true' && github.event.pull_request.head.repo.fork != true - env: - GH_TOKEN: ${{ secrets.AUTOFIX_BOT_PAT || github.token }} - run: | - set -euo pipefail - PR="${{ github.event.pull_request.number }}" - MARKER="" - - # Find an existing comment with our marker. - COMMENT_ID=$(gh api \ - "repos/${{ github.repository }}/issues/${PR}/comments" \ - --paginate --jq ".[] | select(.body | contains(\"${MARKER}\")) | .id" \ - | head -1 || true) - - if [ -n "$COMMENT_ID" ]; then - BODY="${MARKER} - ## ✅ CI-sensitive file review passed - - The \`ci-reviewed\` label is present on this PR." - - gh api --method PATCH \ - "repos/${{ github.repository }}/issues/comments/${COMMENT_ID}" \ - -f body="$BODY" - fi diff --git a/.github/workflows/lockfile-diff.yml b/.github/workflows/lockfile-diff.yml index 2dcf66ea7c55..9d8d59f6da7e 100644 --- a/.github/workflows/lockfile-diff.yml +++ b/.github/workflows/lockfile-diff.yml @@ -7,22 +7,25 @@ name: Lockfile diff # the ``packages`` map at the merge base and at HEAD and set-diffs the # {install path: version} maps instead. # -# The comment is upserted: the script embeds a hidden HTML marker and the -# workflow PATCHes the existing comment when one is found, so a PR gets -# exactly one lockfile-diff comment that tracks the latest push instead -# of a stack of stale ones. When a later push reverts all lockfile -# changes, the comment is updated to say so (deleting it would be more -# surprising than telling the reviewer it's resolved). +# The semantic diff is exposed as a workflow_call output ``review_status`` +# (a JSON array in the unified status format) and an artifact +# (``lockfile-diff`` containing the markdown fragment) for the step +# summary. # -# Never blocking — this is review signal, not enforcement. Exit is 0 even -# when commenting fails (fork PRs get a read-only GITHUB_TOKEN). +# Never blocking — this is review signal, not enforcement. on: workflow_call: + outputs: + changed: + description: Whether package-lock.json changed relative to the target branch. + value: ${{ jobs.diff.outputs.changed }} + review_status: + description: JSON array of review status objects for the unified PR comment. + value: ${{ jobs.diff.outputs.review_status }} permissions: contents: read - pull-requests: write # post/update the diff comment concurrency: group: lockfile-diff-${{ github.event.pull_request.number || github.ref }} @@ -33,6 +36,9 @@ jobs: name: package-lock.json semantic diff runs-on: ubuntu-latest timeout-minutes: 5 + outputs: + changed: ${{ steps.diff.outputs.changed }} + review_status: ${{ steps.emit-status.outputs.review_status }} steps: - name: Checkout code uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 @@ -54,45 +60,36 @@ jobs: --output /tmp/lockfile-diff.md if [ -s /tmp/lockfile-diff.md ]; then echo "changed=true" >> "$GITHUB_OUTPUT" - cat /tmp/lockfile-diff.md >> "$GITHUB_STEP_SUMMARY" + { + echo "## package-lock.json semantic diff" + echo "" + cat /tmp/lockfile-diff.md + } >> "$GITHUB_STEP_SUMMARY" else echo "changed=false" >> "$GITHUB_OUTPUT" + : > /tmp/lockfile-diff.md fi - - name: Post or update PR comment - env: - GH_TOKEN: ${{ secrets.AUTOFIX_BOT_PAT }} - REPO: ${{ github.repository }} - PR: ${{ github.event.pull_request.number }} - CHANGED: ${{ steps.diff.outputs.changed }} + - name: Emit review_status + id: emit-status run: | set -euo pipefail - MARKER='' + CHANGED="${{ steps.diff.outputs.changed }}" + STATUS="[]" - # Find our previous comment (paginated — busy PRs exceed one page). - EXISTING=$(gh api --paginate "repos/${REPO}/issues/${PR}/comments" \ - --jq ".[] | select(.body | startswith(\"$MARKER\")) | .id" \ - | head -1 || true) - - if [ "$CHANGED" != "true" ]; then - if [ -n "$EXISTING" ]; then - # A previous push changed the lockfile but the latest one - # doesn't — update the comment rather than leave stale info. - printf '%s\n✅ package-lock.json changes from an earlier push have been reverted — locked versions now match the target branch.\n' "$MARKER" > /tmp/lockfile-diff.md - else - echo "No lockfile changes and no existing comment — nothing to do." - exit 0 - fi - fi - - if [ -n "$EXISTING" ]; then - echo "Updating existing comment ${EXISTING}" - gh api --method PATCH "repos/${REPO}/issues/comments/${EXISTING}" \ - -F body=@/tmp/lockfile-diff.md > /dev/null \ - || echo "::warning::Could not update PR comment (expected for fork PRs — GITHUB_TOKEN is read-only)" + if [ "$CHANGED" = "true" ]; then + CONTENT=$(cat /tmp/lockfile-diff.md | python3 -c "import sys,json; print(json.dumps(sys.stdin.read()))") + STATUS="[{\"source\":\"lockfile-diff\",\"results\":[{\"kind\":\"action_required\",\"title\":\"package-lock.json\",\"summary\":\"Locked npm dependency versions changed.\",\"detail\":${CONTENT},\"how_to_fix\":\"Add the \`ci-reviewed\` label after verifying the version changes are expected.\"}]}" else - echo "Creating new comment" - gh api "repos/${REPO}/issues/${PR}/comments" \ - -F body=@/tmp/lockfile-diff.md > /dev/null \ - || echo "::warning::Could not post PR comment (expected for fork PRs — GITHUB_TOKEN is read-only)" + STATUS="[]" fi + + echo "review_status=${STATUS}" >> "$GITHUB_OUTPUT" + + - name: Upload diff artifact + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7 + with: + name: lockfile-diff + path: /tmp/lockfile-diff.md + retention-days: 1 + overwrite: true diff --git a/.github/workflows/osv-scanner.yml b/.github/workflows/osv-scanner.yml index e5a983b1bca2..37fed82fbbf9 100644 --- a/.github/workflows/osv-scanner.yml +++ b/.github/workflows/osv-scanner.yml @@ -14,14 +14,14 @@ name: OSV-Scanner # code patterns in PR diffs) by covering the orthogonal "currently-pinned # dep became known-vulnerable" case. # -# Steps below are inlined from Google's officially-recommended reusable -# workflow (google/osv-scanner-action/.github/workflows/osv-scanner-reusable.yml), -# rather than called via `uses:` so we can set a `timeout-minutes` in the -# degenerate case where this job hangs. - +# Uses Google's officially-recommended reusable workflow, pinned by SHA. # Findings land in the repo's Security tab (Code Scanning > OSV-Scanner). # fail-on-vuln is disabled so the job does not block merges on pre-existing # vulnerabilities in pinned deps that we may need to patch deliberately. +# +# The reusable workflow can't emit custom outputs, so a wrapper job +# downloads the SARIF result and summarizes the vulnerability count into +# a review_status for the unified PR comment. on: workflow_call: @@ -40,62 +40,85 @@ permissions: jobs: scan: name: Scan lockfiles - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 - with: - persist-credentials: false + uses: google/osv-scanner-action/.github/workflows/osv-scanner-reusable.yml@9a498708959aeaef5ef730655706c5a1df1edbc2 # v2.3.8 + with: + # Scan explicit lockfiles rather than recursing, so we only look at + # the three sources of truth and skip vendored / test / worktree dirs. + scan-args: |- + --lockfile=uv.lock + --lockfile=package-lock.json + --lockfile=website/package-lock.json + fail-on-vuln: false - - name: 'Run scanner' - uses: google/osv-scanner-action/osv-scanner-action@9a498708959aeaef5ef730655706c5a1df1edbc2 # v2.3.8 + emit-status: + name: Emit review status + runs-on: ubuntu-latest + needs: scan + if: always() + outputs: + review_status: ${{ steps.emit.outputs.review_status }} + steps: + - name: Checkout code + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + + - name: Download SARIF result + uses: actions/download-artifact@d3f86a106a0bac45b974a628896c90dbdf5c8093 # v4 with: - # Scan explicit lockfiles rather than recursing, so we only look at - # the three sources of truth and skip vendored / test / worktree dirs. - scan-args: |- - --output=results.json - --format=json - --lockfile=uv.lock - --lockfile=package-lock.json - --lockfile=website/package-lock.json + name: osv-results + path: /tmp/osv-results continue-on-error: true - - name: 'Run osv-scanner-reporter' - uses: google/osv-scanner-action/osv-reporter-action@9a498708959aeaef5ef730655706c5a1df1edbc2 # v2.3.8 - with: - scan-args: |- - --output=results.sarif - --new=results.json - --gh-annotations=false - --fail-on-vuln=false - - # Upload the results as artifacts (optional). Commenting out will disable uploads of run results in SARIF - # format to the repository Actions tab. - - name: 'Upload artifact' - id: 'upload_artifact' - if: ${{ !cancelled() }} - uses: actions/upload-artifact@bbbca2ddaa5d8feaa63e36b76fdaad77386f024f # v7.0.0 - with: - name: OSV Scanner SARIF file - path: results.sarif - retention-days: 5 - - # Upload the results to GitHub's code scanning dashboard. - - name: 'Upload to code-scanning' - if: ${{ !cancelled() }} - uses: github/codeql-action/upload-sarif@cdefb33c0f6224e58673d9004f47f7cb3e328b89 # v4.31.10 - with: - sarif_file: results.sarif - - - name: 'Print Code Scanning URL' - if: ${{ !cancelled() }} + - name: Emit review_status + id: emit run: | - echo "View the OSV-Scanner results in the 'Security' tab, using the following link:" - echo "${{ github.server_url }}/${{ github.repository }}/security/code-scanning?query=is%3Aopen+branch%3A${GITHUB_REF_NAME}+tool%3Aosv-scanner" - env: - GITHUB_REF_NAME: ${{ github.ref_name }} + set -euo pipefail + STATUS="[]" - - name: 'Error troubleshooter' - if: ${{ always() && steps.upload_artifact.outcome == 'failure' }} - run: | - echo "::error::Artifact upload failed. This is most likely caused by a error during scanning earlier in the workflow." - exit 1 + if [ -f /tmp/osv-results/osv-results.sarif ]; then + # Count vulnerabilities from the SARIF file + VULN_COUNT=$(python3 -c " + import json, sys + try: + with open('/tmp/osv-results/osv-results.sarif') as f: + data = json.load(f) + count = 0 + vulns = [] + for run in data.get('runs', []): + for result in run.get('results', []): + count += 1 + rule_id = result.get('ruleId', 'unknown') + message = result.get('message', {}).get('text', '') + loc = result.get('locations', [{}])[0].get('physicalLocation', {}).get('artifactLocation', {}).get('uri', '') + vulns.append(f'- {rule_id} in {loc}: {message}') + print(count) + if vulns: + print('\n'.join(vulns[:20]), file=sys.stderr) + except Exception: + print(0) + ") + + VULN_DETAIL="" + if [ "$VULN_COUNT" -gt 0 ] 2>/dev/null; then + VULN_PLURAL=$([ "$VULN_COUNT" -eq 1 ] && echo "y" || echo "ies") + VULN_DETAIL=$(python3 -c " + import json, sys + try: + with open('/tmp/osv-results/osv-results.sarif') as f: + data = json.load(f) + vulns = [] + for run in data.get('runs', []): + for result in run.get('results', []): + rule_id = result.get('ruleId', 'unknown') + loc = result.get('locations', [{}])[0].get('physicalLocation', {}).get('artifactLocation', {}).get('uri', '') + vulns.append(f'- {rule_id} in {loc}') + print(json.dumps('\n'.join(vulns[:20]))) + except Exception: + print(json.dumps('')) + ") + STATUS="[{\"source\":\"osv scan\",\"results\":[{\"kind\":\"warning\",\"title\":\"OSV vulnerability scan\",\"summary\":\"${VULN_COUNT} known vulnerabilit${VULN_PLURAL} found in pinned dependencies.\",\"detail\":${VULN_DETAIL},\"how_to_fix\":\"Review the findings in the [Security tab](../../security/code-scanning). Update the affected dependencies if a patched version is available.\"}]}]" + else + STATUS="[]" + fi + fi + + echo "review_status=${STATUS}" >> "$GITHUB_OUTPUT" diff --git a/.github/workflows/review-labels.yml b/.github/workflows/review-labels.yml new file mode 100644 index 000000000000..f0167b486742 --- /dev/null +++ b/.github/workflows/review-labels.yml @@ -0,0 +1,92 @@ +name: Review labels + +# Require explicit maintainer review when CI-sensitive files or the MCP +# catalog change. Previously this was split across two jobs in two +# workflows: ``ci-review`` in lint.yml (gated on ``ci_review``) and +# ``mcp-catalog-review`` in supply-chain-audit.yml (gated on +# ``mcp_catalog``). Both checked for their own label. +# +# Now consolidated: a single ``ci-reviewed`` label covers both. The +# comment sections tell the reviewer exactly what to verify per area, +# so one label is enough — the human reads the comment, not the label +# name. +# +# Outputs: +# ci_reviewed — "true" / "false" / "" (empty when neither lane ran) +# review_status — JSON array of status objects consumed by the review +# comment assembler. See scripts/ci/emit_review_status.py. + +on: + workflow_call: + inputs: + ci_review: + description: Whether CI-sensitive files (eslint config, workflows, actions) changed. + type: boolean + default: false + mcp_catalog: + description: Whether the MCP catalog / installer changed. + type: boolean + default: false + outputs: + ci_reviewed: + description: Whether the ci-reviewed label is present. Empty when neither input was true. + value: ${{ jobs.check.outputs.ci_reviewed }} + review_status: + description: JSON array of status objects for the review comment assembler. + value: ${{ jobs.check.outputs.review_status }} + +permissions: + contents: read + pull-requests: read # read PR labels + +jobs: + check: + name: Review label gate + if: inputs.ci_review || inputs.mcp_catalog + runs-on: ubuntu-latest + timeout-minutes: 2 + outputs: + ci_reviewed: ${{ steps.label-check.outputs.ci_reviewed }} + review_status: ${{ steps.build-status.outputs.review_status }} + steps: + - name: Checkout code + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + + - name: Check ci-reviewed label + id: label-check + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + REPO: ${{ github.repository }} + run: | + set -euo pipefail + PR="${{ github.event.pull_request.number }}" + LABELS=$(gh pr view "$PR" --repo "$REPO" --json labels --jq '.labels[].name' || true) + + if echo "$LABELS" | grep -Fxq 'ci-reviewed'; then + echo "ci-reviewed label present." + echo "ci_reviewed=true" >> "$GITHUB_OUTPUT" + else + echo "ci-reviewed label missing." + echo "ci_reviewed=false" >> "$GITHUB_OUTPUT" + fi + + - name: Build review_status JSON + id: build-status + env: + CI_REVIEW: ${{ inputs.ci_review }} + MCP_CATALOG: ${{ inputs.mcp_catalog }} + LABEL_PRESENT: ${{ steps.label-check.outputs.ci_reviewed }} + run: | + set -euo pipefail + ARGS="" + if [ "$CI_REVIEW" = "true" ]; then ARGS="$ARGS --ci-review"; fi + if [ "$MCP_CATALOG" = "true" ]; then ARGS="$ARGS --mcp-catalog"; fi + if [ "$LABEL_PRESENT" = "true" ]; then ARGS="$ARGS --label-present"; fi + + python3 scripts/ci/emit_review_status.py $ARGS --output "$GITHUB_OUTPUT" + + - name: Fail on missing label + if: steps.label-check.outputs.ci_reviewed != 'true' + run: | + echo "::error::CI-sensitive changes require the ci-reviewed label. Add the label and re-run this check." + exit 1 diff --git a/.github/workflows/supply-chain-audit.yml b/.github/workflows/supply-chain-audit.yml index 648a1f7c6a63..e8a4508328f8 100644 --- a/.github/workflows/supply-chain-audit.yml +++ b/.github/workflows/supply-chain-audit.yml @@ -10,9 +10,17 @@ name: Supply Chain Audit # advisory-only workflow instead. # # Path-gating is handled centrally by the ``ci.yml`` orchestrator's -# ``detect`` job. The orchestrator passes ``scan`` / ``deps`` / -# ``mcp_catalog`` booleans as inputs; this workflow's jobs gate on those -# inputs instead of re-computing the diff. +# ``detect`` job. The orchestrator passes ``scan`` / ``deps`` booleans as +# inputs; this workflow's jobs gate on those inputs instead of re-computing +# the diff. MCP catalog review was previously here but has moved to +# ``review-labels.yml`` so it can be rerun independently. +# +# Outputs: +# review_status — JSON array of status objects consumed by the review +# comment assembler (scripts/ci/assemble_review_comment.py). +# Each job (``scan``, ``dep-bounds``) emits its own +# array; an ``aggregate`` job merges them into the +# workflow-level output. on: workflow_call: @@ -29,10 +37,10 @@ on: description: Whether pyproject.toml changed. type: boolean required: true - mcp_catalog: - description: Whether the MCP catalog / installer changed. - type: boolean - required: true + outputs: + review_status: + description: JSON array of review status objects for the review comment assembler. + value: ${{ jobs.aggregate.outputs.review_status }} permissions: pull-requests: write @@ -44,6 +52,8 @@ jobs: if: inputs.scan runs-on: ubuntu-latest timeout-minutes: 15 + outputs: + review_status: ${{ steps.emit-status.outputs.review_status }} steps: - name: Checkout uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 @@ -61,7 +71,7 @@ jobs: HEAD="${{ github.event.pull_request.head.sha }}" # Added lines only, excluding lockfiles. - # Three-dot diff (base...head) diffs from the merge base to HEAD, + # Three-point diff (base...head) diffs from the merge base to HEAD, # so only changes introduced by this PR are included — not changes # that landed on main after the PR branched off. DIFF=$(git diff "$BASE"..."$HEAD" -- . ':!uv.lock' ':!*.lock' ':!package-lock.json' ':!yarn.lock' || true) @@ -139,26 +149,41 @@ jobs: echo "found=false" >> "$GITHUB_OUTPUT" fi - - name: Post critical finding comment - if: steps.scan.outputs.found == 'true' + - name: Emit review_status + id: emit-status + if: always() env: - GH_TOKEN: ${{ secrets.AUTOFIX_BOT_PAT }} + FOUND: ${{ steps.scan.outputs.found }} run: | - BODY="## 🚨 CRITICAL Supply Chain Risk Detected + python3 - <<'PYEOF' + import json, os - This PR contains a pattern that has been used in real supply chain attacks. A maintainer must review the flagged code carefully before merging. + found = os.environ.get("FOUND", "") == "true" - $(cat /tmp/findings.md) + if found: + with open("/tmp/findings.md", encoding="utf-8") as f: + detail = f.read() + status = [{ + "source": "supply chain", + "results": [{ + "kind": "error", + "title": "Critical supply chain risk", + "summary": "Critical supply chain risk patterns detected in this PR.", + "detail": detail, + "how_to_fix": "Review the flagged code carefully. If intentional, add the `ci-reviewed` label." + }] + }] + else: + status = [] - --- - *Scanner only fires on high-signal indicators: .pth files, base64+exec/eval combos, subprocess with encoded commands, or install-hook files. Low-signal warnings were removed intentionally — if you're seeing this comment, the finding is worth inspecting.*" - - gh pr comment "${{ github.event.pull_request.number }}" --body "$BODY" || echo "::warning::Could not post PR comment (expected for fork PRs — GITHUB_TOKEN is read-only)" + with open(os.environ["GITHUB_OUTPUT"], "a", encoding="utf-8") as f: + f.write(f"review_status={json.dumps(status)}\n") + PYEOF - name: Fail on critical findings if: steps.scan.outputs.found == 'true' run: | - echo "::error::CRITICAL supply chain risk patterns detected in this PR. See the PR comment for details." + echo "::error::CRITICAL supply chain risk patterns detected in this PR. See the review comment for details." exit 1 dep-bounds: @@ -166,6 +191,8 @@ jobs: if: inputs.deps runs-on: ubuntu-latest timeout-minutes: 15 + outputs: + review_status: ${{ steps.emit-status.outputs.review_status }} steps: - name: Checkout uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 @@ -188,7 +215,7 @@ jobs: exit 0 fi - # Match PyPI dep specs that have >= but no < ceiling. + # Match PyPI dep specs that have >= and no < ceiling. # Pattern: "package>=version" without a following ",<" bound. # Excludes git+ URLs (which use commit SHAs) and comments. UNBOUNDED=$(echo "$ADDED" | grep -oE '"[a-zA-Z0-9_-]+(\[[^\]]*\])?>=[ 0-9.]+"' | grep -v ',<' || true) @@ -200,26 +227,36 @@ jobs: echo "found=false" >> "$GITHUB_OUTPUT" fi - - name: Post unbounded dep warning - if: steps.bounds.outputs.found == 'true' + - name: Emit review_status + id: emit-status + if: always() env: - GH_TOKEN: ${{ secrets.AUTOFIX_BOT_PAT }} + FOUND: ${{ steps.bounds.outputs.found }} run: | - BODY="## ⚠️ Unbounded PyPI Dependency Detected + python3 - <<'PYEOF' + import json, os - This PR adds PyPI dependencies without a \`=floor,=1.2.0,<2"`. See CONTRIBUTING.md dependency pinning policy.' + }] + }] + else: + status = [] - **Fix:** Add an upper bound, e.g. \`"package>=1.2.0,<2"\` - - --- - *See PR #2810 and CONTRIBUTING.md for the full policy rationale.*" - - gh pr comment "${{ github.event.pull_request.number }}" --body "$BODY" || echo "::warning::Could not post PR comment (expected for fork PRs)" + with open(os.environ["GITHUB_OUTPUT"], "a", encoding="utf-8") as f: + f.write(f"review_status={json.dumps(status)}\n") + PYEOF - name: Fail on unbounded deps if: steps.bounds.outputs.found == 'true' @@ -227,45 +264,36 @@ jobs: echo "::error::PyPI dependencies without upper bounds detected. Add > "$GITHUB_OUTPUT" exit 1 fi + review_status='[]' + echo "review_status=${review_status}" >> "$GITHUB_OUTPUT" diff --git a/scripts/ci/assemble_review_comment.py b/scripts/ci/assemble_review_comment.py new file mode 100644 index 000000000000..0f701f13e338 --- /dev/null +++ b/scripts/ci/assemble_review_comment.py @@ -0,0 +1,424 @@ +#!/usr/bin/env python3 +"""Assemble the unified CI review comment for a pull request. + +Every CI job that wants to appear in the review comment emits a +``review_status`` output: a JSON array of objects, each with a ``source`` +(the workflow name, used for dedup) and a ``results`` array of typed +result objects:: + + [ + { + "source": "review-label-gate", + "results": [ + {"kind": "action_required", "title": "...", "summary": "...", + "how_to_fix": "..."}, + {"kind": "info", "title": "...", "summary": "..."} + ] + }, + { + "source": "ci-timings", + "results": [ + {"kind": "warning", "title": "CI timings", "summary": "...", + "detail": "...", "link": "..."} + ] + } + ] + +Each result object has: + + kind: "error" | "action_required" | "warning" | "info" + title: section heading + summary: one-line description + detail: markdown detail (optional) + how_to_fix: markdown checklist (optional) + link: URL (optional) + link_label: label for the link (optional, default "View logs") + +The assembler flattens all results into a flat list of ReviewItems, +grouped by severity in the comment. Jobs that failed (from the +``needs`` context) but didn't emit any status get synthesized ❌ Error +items. Jobs that DID emit a status are excluded from the synthesized +error list — their own output is the authority for their classification. + +Exits 0 always — comment posting is best-effort (fork PRs are read-only). +""" + +from __future__ import annotations + +import argparse +import json +import sys +from dataclasses import dataclass +from pathlib import Path + +# Hidden marker the comment system uses to find-and-edit its +# previous comment instead of stacking new ones on each run. +MARKER = "" + +# Severity ordering for display. +_SEVERITY_ORDER = ["error", "action_required", "warning", "info"] + +# Severities that trigger the "blocking issues" layout (vs. the +# "looks good!" banner). +_BLOCKING_SEVERITIES = ("error", "action_required", "warning") + +_SEVERITY_GROUP_HEADER = { + "error": "## ❌ Job failures", + "action_required": "## ⚠️ Action required", + "warning": "## ⚠️ Warnings", + "info": "## ℹ️ Details", +} + + +@dataclass +class ReviewItem: + """A single piece of review information with a severity tag.""" + + severity: str # "error" | "action_required" | "warning" | "info" + title: str # short section title, e.g. "package-lock.json" + summary: str # one-line summary + detail: str = "" # optional markdown detail (tables, bullet lists, etc.) + link: str = "" # optional URL emitted by the job (e.g. report URL) + link_label: str = "View report" # label for the emitted link + how_to_fix: str = "" # optional markdown checklist for action_required items + source: str = "" # workflow that declared this status (for dedup) + job_url: str = "" # auto-attached per-job log link (from the live poller) + + +# --------------------------------------------------------------------------- +# Collectors — each returns a list of ReviewItems (possibly empty) +# --------------------------------------------------------------------------- + + +def collect_from_statuses(review_statuses_json: str) -> tuple[list[ReviewItem], set[str]]: + """Parse the nested review_status JSON into flat ReviewItems. + + The input is a JSON array of ``{source, results: [...]}`` objects. + Each entry in ``results`` becomes one ReviewItem, tagged with the + parent's ``source``. + + Returns ``(items, sources)`` where ``sources`` is the set of source + values — used by :func:`collect_failed_jobs` to exclude jobs that + already declared their own status (so a failing job that emitted an + ``action_required`` status doesn't also show as a synthesized ❌ Error). + """ + if not review_statuses_json: + return [], set() + try: + data = json.loads(review_statuses_json) + except (json.JSONDecodeError, TypeError): + return [], set() + if not isinstance(data, list): + return [], set() + + items: list[ReviewItem] = [] + sources: set[str] = set() + + for entry in data: + if not isinstance(entry, dict): + continue + source = entry.get("source", "") + if source: + sources.add(source) + for r in entry.get("results", []): + if not isinstance(r, dict): + continue + kind = r.get("kind", "info") + if kind not in _SEVERITY_ORDER: + kind = "info" + items.append(ReviewItem( + severity=kind, + title=r.get("title", "Unknown"), + summary=r.get("summary", ""), + detail=r.get("detail", ""), + link=r.get("link", ""), + link_label=r.get("link_label", "View logs"), + how_to_fix=r.get("how_to_fix", ""), + source=source, + )) + + return items, sources + + +def collect_failed_jobs( + needs_json: str, + run_url: str, + exclude_sources: set[str] | None = None, + job_urls: dict[str, str] | None = None, +) -> list[ReviewItem]: + """Build error items for failed CI jobs from the ``needs`` context. + + ``needs_json`` is the JSON string emitted by ``all-checks-pass`` — a + ``{job_name: result}`` dict where result is ``success`` / ``failure`` + / ``skipped``. Only ``failure`` entries become error items. + + ``exclude_sources`` is a set of ``source`` values from status objects + declared by workflow_call jobs. Job names containing any of these + source strings are excluded — their failure is already covered by their + own status output. + + ``job_urls`` is an optional ``{job_name: html_url}`` dict from the + live poller. When a job's name is in this dict, the ❌ Error link + points directly to that job's logs page instead of the whole run. + Falls back to ``run_url`` when no per-job URL is available. + """ + if not needs_json: + return [] + try: + needs = json.loads(needs_json) + except (json.JSONDecodeError, TypeError): + return [] + + # Pre-normalize exclude sources once: lowercase + hyphens→spaces, so + # "review-label-gate" matches "Review label gate / Review label gate". + norm_sources = { + src.lower().replace("-", " ") for src in (exclude_sources or set()) + } + + items: list[ReviewItem] = [] + for name, result in sorted(needs.items()): + if result != "failure": + continue + if norm_sources: + norm = name.lower().replace("-", " ") + if any(src in norm for src in norm_sources): + continue + job_url = (job_urls or {}).get(name, run_url) + items.append(ReviewItem( + severity="error", + title=name, + summary=f"Job **{name}** failed.", + job_url=job_url, + )) + return items + + +# --------------------------------------------------------------------------- +# Rendering +# --------------------------------------------------------------------------- + + +def _render_item(item: ReviewItem) -> str: + """Render a single ReviewItem as a markdown block. + + The group header (``## ❌ Job failures`` etc.) carries the severity + emoji, so items don't repeat it. Links are shown inline next to the + title. Layout per item:: + + ### {title} · [View report](url) · [View job](url) + + {summary} + + {detail} + + **How to fix:** + + {how_to_fix} + """ + title = f"### {item.title}" + # Build inline links next to the title. + links: list[str] = [] + if item.link: + links.append(f"[{item.link_label}]({item.link})") + if item.job_url: + links.append(f"[View job]({item.job_url})") + if links: + title += " · " + " · ".join(links) + + parts = [title, "", item.summary] + + if item.detail: + parts += ["", item.detail] + if item.how_to_fix: + parts += ["", "**How to fix:**", "", item.how_to_fix] + + return "\n".join(parts) + + +def _render_group(header: str, items: list[ReviewItem]) -> str: + """Render a severity group: ``##`` header + items separated by ``---``.""" + blocks = [_render_item(i) for i in items] + return f"{header}\n\n" + "\n\n---\n\n".join(blocks) + + +def _render_info_details(items: list[ReviewItem]) -> str: + """Render each info item as its own collapsible ``
`` block.""" + blocks = [] + for item in items: + inner = _render_item(item) + blocks.append( + f"
\n{item.title}\n\n{inner}\n\n
" + ) + return "\n\n".join(blocks) + + +def _render_pending_items(pending_jobs: list[str]) -> str: + """Render the dimmed ```` items for jobs still running.""" + job_list = ", ".join(f"`{j}`" for j in sorted(pending_jobs)) + return f"\n\n---\n\nStill running {len(pending_jobs)} job{'s' if len(pending_jobs) != 1 else ''}: {job_list}\n" + + +def render_comment(items: list[ReviewItem], pending_jobs: list[str] | None = None, commit_info: str = "") -> str: + """Render the full comment body from a list of review items. + + Items are grouped by severity under ``##`` group headers, separated + by ``---``. Errors and action_required items are always visible. + Warnings are shown only when present. Info items are in a collapsible + ``
`` block. If ``pending_jobs`` is non-empty, a dimmed + ```` footer is appended listing jobs still running. + + When there are no errors, action_required, or warnings (only info + items, or nothing at all), a "looks good!" banner is shown at the top, + and info items (if any) follow in a collapsible ``
`` block. + """ + pending = pending_jobs or [] + + # Group by severity + by_severity: dict[str, list[ReviewItem]] = {s: [] for s in _SEVERITY_ORDER} + for item in items: + by_severity.setdefault(item.severity, []).append(item) + + info = by_severity.get("info", []) + has_blocking = any(by_severity.get(s) for s in _BLOCKING_SEVERITIES) + + body = f"{MARKER}\n# ૮ >ﻌ< ა ci review\n\n" + + if commit_info: + body += f"{commit_info}\n\n" + + if not items and not pending: + return f"{body}looks good to me!" + + sections: list[str] = [] + + for sev in _BLOCKING_SEVERITIES: + group = by_severity.get(sev, []) + if group: + sections.append(_render_group(_SEVERITY_GROUP_HEADER[sev], group)) + + # Info: collapsible
+ if info: + sections.append(_render_info_details(info)) + + if pending: + body += _render_pending_items(pending) + + if sections: + body += "\n\n---\n\n".join(sections) + + return body + + +# --------------------------------------------------------------------------- +# Assembly +# --------------------------------------------------------------------------- + + +def _attach_job_urls(items: list[ReviewItem], job_urls: dict[str, str], run_url: str) -> None: + """Fill in per-job log links for all items. + + Uses the same case-insensitive, hyphen-normalized matching as + :func:`collect_failed_jobs`: the item's ``source`` is matched against + job names in ``job_urls``. Sets ``job_url`` on the item — this is + separate from ``link`` (the job-emitted URL, e.g. a report artifact), + so both can appear in the rendered comment. + """ + if not job_urls and not run_url: + return + # Pre-normalize job_url keys once. + norm_urls: dict[str, str] = {} + for name, url in job_urls.items(): + norm_urls[name.lower().replace("-", " ")] = url + + for item in items: + if item.job_url: + continue + src = item.source.lower().replace("-", " ") + # Try exact match first, then substring match. + if src in norm_urls: + item.job_url = norm_urls[src] + continue + for norm_name, url in norm_urls.items(): + if src and src in norm_name: + item.job_url = url + break + # If no per-job URL found, fall back to run_url for items with a source. + if not item.job_url and item.source and run_url: + item.job_url = run_url + + +def assemble( + needs_json: str = "", + run_url: str = "", + job_urls: dict[str, str] | None = None, + review_statuses_json: str = "", + pending_jobs: list[str] | None = None, + commit_info: str = "", +) -> str: + """Assemble the full comment body from all available inputs.""" + items: list[ReviewItem] = [] + + # 1. Structured statuses from workflow_call jobs (review-labels, etc.) + status_items, sources = collect_from_statuses(review_statuses_json) + items.extend(status_items) + + # 2. Synthesized error items for failed jobs not covered by statuses + items.extend(collect_failed_jobs(needs_json, run_url, exclude_sources=sources, job_urls=job_urls)) + + # 3. Attach per-job log links to all items (not just synthesized errors) + _attach_job_urls(items, job_urls or {}, run_url) + + return render_comment(items, pending_jobs, commit_info) + + +# --------------------------------------------------------------------------- +# CLI +# --------------------------------------------------------------------------- + + +def main() -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument( + "--needs-json", + default="", + help="JSON string of {job_name: result} from the all-checks-pass job.", + ) + parser.add_argument( + "--run-url", + default="", + help="URL to the CI run summary page (for failed job links).", + ) + parser.add_argument( + "--review-statuses-json", + default="", + help="JSON array of {source, results: [...]} objects from workflow_call jobs.", + ) + parser.add_argument( + "--pending-jobs", + default="", + help="Comma-separated list of job names still running (shown in a dimmed footer).", + ) + parser.add_argument( + "--output", + type=Path, + required=True, + help="Output file for the assembled comment body.", + ) + args = parser.parse_args() + + pending = [j.strip() for j in args.pending_jobs.split(",") if j.strip()] if args.pending_jobs else None + + body = assemble( + needs_json=args.needs_json, + run_url=args.run_url, + review_statuses_json=args.review_statuses_json, + pending_jobs=pending, + ) + + args.output.write_text(body) + print(f"Wrote {len(body)} chars to {args.output}") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/scripts/ci/emit_review_status.py b/scripts/ci/emit_review_status.py new file mode 100644 index 000000000000..dcf58a7a0e19 --- /dev/null +++ b/scripts/ci/emit_review_status.py @@ -0,0 +1,143 @@ +#!/usr/bin/env python3 +"""Emit review_status JSON for the review-labels workflow. + +Builds a JSON array with one entry:: + + [ + { + "source": "review-label-gate", + "results": [ + {"kind": "action_required", "title": "...", "summary": "...", + "how_to_fix": "..."}, + {"kind": "info", "title": "...", "summary": "..."} + ] + } + ] + +The ``source`` field is the workflow name that declared the status; the +assembler uses it to exclude the corresponding job from the synthesized +❌ Error list (the job already has its own status section). + +The array can contain 0, 1, or 2 results — one per lane that ran +(``ci_review``, ``mcp_catalog``). When the ``ci-reviewed`` label is +present, the kind is ``info``; when missing, it's ``action_required`` +with the verification checklist. +""" + +from __future__ import annotations + +import argparse +import json +import sys + +# The source identifier used for error-synthesis exclusion. This must +# match (as a normalized substring) the job name as it appears in the +# GitHub Actions API. The ci.yml job key is ``review-labels`` with +# ``name: Review label gate``, and the reusable workflow's job is also +# ``name: Review label gate``, so the API shows the job as +# "Review label gate / Review label gate". Normalizing "review-label-gate" +# (lowercase, hyphens→spaces) gives "review label gate", which is a +# substring of "review label gate / review label gate". +SOURCE = "review-label-gate" + + +def build_results( + ci_review: bool, + mcp_catalog: bool, + label_present: bool, +) -> list[dict]: + """Build the list of result objects for this source.""" + results: list[dict] = [] + + if ci_review: + if label_present: + results.append({ + "kind": "info", + "title": "CI-sensitive file review", + "summary": "`ci-reviewed` label is present.", + }) + else: + results.append({ + "kind": "action_required", + "title": "CI-sensitive file review", + "summary": ( + "This PR changes CI-sensitive files (eslint config, " + "workflow YAMLs, or composite actions). These influence " + "what the js-autofix job executes and pushes to main." + ), + "how_to_fix": ( + "Add the `ci-reviewed` label after verifying:\n" + "- no new eslint rules with custom `fix` functions that write outside linted paths,\n" + "- no workflow changes that widen permissions or remove guards,\n" + "- no composite action changes that alter what gets executed." + ), + }) + + if mcp_catalog: + if label_present: + results.append({ + "kind": "info", + "title": "MCP catalog security review", + "summary": "`ci-reviewed` label is present.", + }) + else: + results.append({ + "kind": "action_required", + "title": "MCP catalog security review", + "summary": ( + "This PR changes the bundled MCP catalog or MCP catalog " + "installer code. MCP entries can define local commands " + "that users later install into `mcp_servers`, so this " + "needs explicit maintainer review before merge." + ), + "how_to_fix": ( + "Add the `ci-reviewed` label after verifying:\n" + "- any new/changed `optional-mcps/**/manifest.yaml` command and args are expected,\n" + "- stdio transports do not use shell+egress/exfiltration payloads,\n" + "- git install refs are pinned and bootstrap commands are minimal,\n" + "- requested env vars/secrets match the upstream MCP's documented needs." + ), + }) + + return results + + +def build_statuses( + ci_review: bool, + mcp_catalog: bool, + label_present: bool, +) -> list[dict]: + """Build the full review_status array (one entry with a results list).""" + results = build_results(ci_review, mcp_catalog, label_present) + if not results: + return [] + return [{"source": SOURCE, "results": results}] + + +def main() -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--ci-review", action="store_true", + help="Whether CI-sensitive files changed.") + parser.add_argument("--mcp-catalog", action="store_true", + help="Whether the MCP catalog / installer changed.") + parser.add_argument("--label-present", action="store_true", + help="Whether the ci-reviewed label is present.") + parser.add_argument("--output", default="-", + help="Output file ('-' for stdout, or a GITHUB_OUTPUT path).") + args = parser.parse_args() + + statuses = build_statuses(args.ci_review, args.mcp_catalog, args.label_present) + json_str = json.dumps(statuses) + + if args.output == "-": + print(json_str) + else: + # GITHUB_OUTPUT format: key=value\n + with open(args.output, "a", encoding="utf-8") as f: + f.write(f"review_status={json_str}\n") + + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/scripts/ci/live_comment.py b/scripts/ci/live_comment.py new file mode 100644 index 000000000000..e4ca5c9e2de5 --- /dev/null +++ b/scripts/ci/live_comment.py @@ -0,0 +1,526 @@ +#!/usr/bin/env python3 +"""Live-updating CI review comment. + +Polls the GitHub Actions API for job statuses in the current run, assembles +the review comment from whatever results are available, and upserts it as a +PR comment. Repeats every ``--interval`` seconds until all jobs are +completed (or ``--timeout`` is reached), so the comment updates in real time +as each job finishes. + +The comment is identified by the ```` marker +— the same one ``assemble_review_comment.py`` uses — so it replaces any +previous comment from an earlier run. + +Architecture: + + - :func:`classify_jobs` (pure, testable) — takes a list of raw API job + dicts and returns ``(completed, pending, job_urls)`` where ``completed`` + is a ``{name: result}`` dict (for :func:`assemble_review_comment.assemble`) + and ``pending`` is a list of job names still running. + + - :func:`find_comment_id` / :func:`upsert_comment` — thin API wrappers. + + - :func:`_fetch_timings_statuses` — downloads the ci-timings artifact + (if available) and parses the ``review_status=`` line from it, merging + the status objects into the review statuses array. + + - :func:`run` — the polling loop. Calls the API, classifies, assembles, + upserts, sleeps, repeats. Exits when all jobs are completed. + +The orchestrator job names (detect, all-checks-pass, comment-live, etc.) +are excluded from the comment — they're infrastructure, not review signal. +""" + +from __future__ import annotations + +import argparse +import json +import os +import subprocess +import sys +import time +import urllib.error +import urllib.request +from pathlib import Path + +API_BASE = "https://api.github.com" + +# Job names that are infrastructure (this script, the gate, the detector) +# and should never appear in the review comment. +_INFRA_JOBS = frozenset({ + "detect", + "all-checks-pass", + "comment-pending", + "comment-results", + "comment-live", + "CI review comment (pending)", + "CI review comment (results)", + "CI review comment (live)", + "All required checks pass", + "Detect affected areas", +}) + +# Map GitHub API conclusion values to our result strings. +_CONCLUSION_MAP = { + "success": "success", + "failure": "failure", + "skipped": "skipped", + "cancelled": "skipped", + "neutral": "skipped", + "timed_out": "failure", + "action_required": "skipped", +} + + +def classify_jobs(api_jobs: list[dict]) -> tuple[dict[str, str], list[str], dict[str, str]]: + """Classify raw API job dicts into completed + pending + job_urls. + + Returns ``(completed, pending, job_urls)``: + + - ``completed``: ``{job_name: result}`` where result is + ``"success"`` / ``"failure"`` / ``"skipped"``. Only non-infra jobs + that have finished. + - ``pending``: list of job names still running (in_progress / queued + / waiting). Excludes infra jobs. + - ``job_urls``: ``{job_name: html_url}`` — direct links to each + job's logs page, for the assembler to use in ❌ Error links. + + The API returns orchestrator-level jobs and sub-workflow jobs + (workflow_call) in separate runs — :func:`collect_run_jobs` merges + them. Each sub-workflow job has a ``_workflow_name`` prefix so the + display name is ``"Workflow / job"``. + """ + completed: dict[str, str] = {} + pending: list[str] = [] + job_urls: dict[str, str] = {} + + for job in api_jobs: + name = job.get("name", "unknown") + if job.get("_workflow_name"): + name = f"{job['_workflow_name']} / {name}" + if name in _INFRA_JOBS: + continue + status = job.get("status", "") + conclusion = job.get("conclusion", "") + html_url = job.get("html_url", "") + + if html_url: + job_urls[name] = html_url + + if status in ("in_progress", "queued", "waiting"): + pending.append(name) + elif status == "completed": + result = _CONCLUSION_MAP.get(conclusion, "skipped") + completed[name] = result + # else: unknown status → skip + + return completed, pending, job_urls + + +# --------------------------------------------------------------------------- +# API helpers +# --------------------------------------------------------------------------- + + +def _api_request(url: str, token: str) -> dict: + """Authenticated GitHub API GET (single page).""" + req = urllib.request.Request(url, headers={ + "Authorization": f"Bearer {token}", + "Accept": "application/vnd.github+json", + "X-GitHub-Api-Version": "2022-11-28", + "User-Agent": "ci-live-comment", + }) + with urllib.request.urlopen(req) as resp: + data: dict = json.loads(resp.read()) + return data + + +def _api_get_paginated(url: str, token: str, list_key: str | None = None) -> list: + """Authenticated GitHub API GET with pagination.""" + results: list = [] + while url: + req = urllib.request.Request(url, headers={ + "Authorization": f"Bearer {token}", + "Accept": "application/vnd.github+json", + "X-GitHub-Api-Version": "2022-11-28", + "User-Agent": "ci-live-comment", + }) + with urllib.request.urlopen(req) as resp: + data = json.loads(resp.read()) + link_header = resp.headers.get("Link", "") + + if list_key: + results.extend(data.get(list_key, [])) + elif isinstance(data, list): + results.extend(data) + else: + return data + + next_url = None + for part in link_header.split(","): + part = part.strip() + if 'rel="next"' in part: + next_url = part[part.find("<") + 1:part.find(">")] + break + url = next_url + + return results + + +def collect_run_jobs(token: str, repo: str, run_id: str) -> list[dict]: + """Collect all jobs in the orchestrator run + sub-workflow runs. + + Returns a flat list of job dicts (same shape as the API returns, plus + ``_workflow_name`` on sub-workflow jobs). + """ + owner, repo_name = repo.split("/") + run_info = _api_request(f"{API_BASE}/repos/{owner}/{repo_name}/actions/runs/{run_id}", token) + created_at = run_info.get("created_at", "") + head_sha = run_info.get("head_sha", "") + + # Orchestrator jobs + orch_jobs = _api_get_paginated( + f"{API_BASE}/repos/{owner}/{repo_name}/actions/runs/{run_id}/jobs", + token, list_key="jobs", + ) + + # Sub-workflow runs (workflow_call) + sub_runs = _api_get_paginated( + f"{API_BASE}/repos/{owner}/{repo_name}/actions/runs?head_sha={head_sha}&event=workflow_call&per_page=100", + token, list_key="workflow_runs", + ) + sub_runs = [r for r in sub_runs if r.get("created_at", "") >= created_at] + + all_jobs: list[dict] = [] + # Orchestrator jobs: skip workflow-call placeholder steps (they're + # sub-workflow triggers, not review signal), but KEEP in_progress / + # queued jobs so the poller knows they're still running. + for job in orch_jobs: + steps = job.get("steps") or [] + if any(s.get("name", "").startswith("Run ./.github/") for s in steps): + continue + all_jobs.append(job) + + # Sub-workflow jobs (workflow_call). + # These runs may not exist yet on the first few polls — that's fine, + # classify_jobs() will just show 0 pending for them. + for sr in sub_runs: + sr_id = sr["id"] + sr_name = sr.get("name", "") + sr_jobs = _api_get_paginated( + f"{API_BASE}/repos/{owner}/{repo_name}/actions/runs/{sr_id}/jobs", + token, list_key="jobs", + ) + for j in sr_jobs: + j["_workflow_name"] = sr_name + all_jobs.append(j) + + return all_jobs + + +def find_comment_id(token: str, repo: str, pr_number: str) -> int | None: + """Find our existing review comment by marker prefix.""" + owner, repo_name = repo.split("/") + comments = _api_get_paginated( + f"{API_BASE}/repos/{owner}/{repo_name}/issues/{pr_number}/comments", + token, + ) + for c in comments: + body = c.get("body", "") if isinstance(c, dict) else "" + if body.startswith(""): + return c.get("id") if isinstance(c, dict) else None + return None + + +def upsert_comment( + token: str, repo: str, pr_number: str, body: str, comment_id: int | None = None +) -> int | None: + """Create or update the review comment. Returns the comment ID.""" + owner, repo_name = repo.split("/") + if comment_id is None: + comment_id = find_comment_id(token, repo, pr_number) + + if comment_id: + url = f"{API_BASE}/repos/{owner}/{repo_name}/issues/comments/{comment_id}" + method = "PATCH" + else: + url = f"{API_BASE}/repos/{owner}/{repo_name}/issues/{pr_number}/comments" + method = "POST" + + data = json.dumps({"body": body}).encode("utf-8") + req = urllib.request.Request(url, data=data, method=method, headers={ + "Authorization": f"Bearer {token}", + "Accept": "application/vnd.github+json", + "X-GitHub-Api-Version": "2022-11-28", + "Content-Type": "application/json", + "User-Agent": "ci-live-comment", + }) + try: + with urllib.request.urlopen(req) as resp: + result = json.loads(resp.read()) + return result.get("id") + except urllib.error.HTTPError as e: + print(f" API error {e.code}: {e.reason}", file=sys.stderr) + return None + + +# --------------------------------------------------------------------------- +# Artifact fetching (ci-timings review_status) +# --------------------------------------------------------------------------- + + +def _fetch_artifact_statuses( + token: str, repo: str, run_id: str, artifact_name: str, +) -> list[dict]: + """Download a workflow artifact and extract review_status entries. + + The ci-timings job writes a ``review-status.json`` file containing + ``review_status=`` (GITHUB_OUTPUT format) into its artifact. + This function downloads the artifact, parses the line, and returns + the parsed status array. Returns ``[]`` if the artifact doesn't exist + yet or can't be parsed. + """ + try: + result = subprocess.run( + ["gh", "run", "download", run_id, "--repo", repo, + "--name", artifact_name, "--dir", "/tmp/artifact-dl"], + capture_output=True, timeout=30, + ) + if result.returncode != 0: + return [] + except Exception: + return [] + + status_file = Path("/tmp/artifact-dl/review-status.json") + if not status_file.exists(): + return [] + + try: + content = status_file.read_text(encoding="utf-8").strip() + # GITHUB_OUTPUT format: review_status= + if content.startswith("review_status="): + content = content[len("review_status="):] + statuses = json.loads(content) + if isinstance(statuses, list): + return statuses + except (json.JSONDecodeError, OSError): + pass + + return [] + + +# --------------------------------------------------------------------------- +# Comment assembly +# --------------------------------------------------------------------------- + + +def _import_assembler(): + """Import assemble_review_comment.py from the same directory.""" + here = Path(__file__).resolve().parent + sys.path.insert(0, str(here)) + import assemble_review_comment as asm + return asm + + +def build_comment_body( + asm_mod, + completed: dict[str, str], + pending: list[str], + run_url: str, + job_urls: dict[str, str], + review_statuses_json: str, + commit_info: str = "", +) -> str: + """Assemble the comment body from current job states + static inputs.""" + needs_json = json.dumps(completed) if completed else "" + + return asm_mod.assemble( + needs_json=needs_json, + run_url=run_url, + job_urls=job_urls, + review_statuses_json=review_statuses_json, + pending_jobs=pending if pending else None, + commit_info=commit_info, + ) + + +def _merge_statuses( + base_statuses: list[dict], extra_statuses: list[dict] +) -> str: + """Merge two status arrays into one JSON string.""" + merged = list(base_statuses) + list(extra_statuses) + return json.dumps(merged) if merged else "" + + +# --------------------------------------------------------------------------- +# Polling loop +# --------------------------------------------------------------------------- + + +def run( + token: str, + repo: str, + run_id: str, + pr_number: str, + run_url: str, + review_statuses_json: str = "", + commit_info: str = "", + interval: int = 15, + timeout: int = 1800, + dry_run: bool = False, +) -> int: + """Poll for job statuses and update the PR comment until all done. + + Returns 0 always — comment posting is best-effort. + """ + asm = _import_assembler() + start = time.time() + last_body = "" + + # Parse the base statuses once (from review-labels, lockfile-diff, etc.) + try: + base_statuses = json.loads(review_statuses_json) if review_statuses_json else [] + except (json.JSONDecodeError, TypeError): + base_statuses = [] + print(f" Loaded {len(base_statuses)} base review status entries") + + while True: + elapsed = time.time() - start + if elapsed > timeout: + print(f"Timeout ({timeout}s) reached — stopping poll.", file=sys.stderr) + break + + try: + jobs = collect_run_jobs(token, repo, run_id) + except Exception as e: + print(f" API error collecting jobs: {e}", file=sys.stderr) + time.sleep(interval) + continue + + completed, pending, job_urls = classify_jobs(jobs) + total = len(completed) + len(pending) + print(f" [{elapsed:.0f}s] {len(completed)} completed, {len(pending)} pending " + f"({total} total jobs)") + + # Try to fetch ci-timings artifact statuses (may not exist yet). + artifact_statuses = _fetch_artifact_statuses( + token, repo, run_id, "ci-timings-report", + ) + if artifact_statuses: + print(f" Found ci-timings artifact with {len(artifact_statuses)} status entries") + + merged_json = _merge_statuses(base_statuses, artifact_statuses) + + body = build_comment_body( + asm, completed, pending, run_url, job_urls, + merged_json, + commit_info, + ) + + if body != last_body: + if dry_run: + print("--- DRY RUN — comment body ---") + print(body) + print("--- END ---") + else: + cid = upsert_comment(token, repo, pr_number, body) + if cid: + print(f" Updated comment {cid}") + else: + print(" Failed to update comment (will retry)", file=sys.stderr) + last_body = body + else: + print(" No change since last poll.") + + if not pending: + # Check if any dependency failed. If so, exit non-zero so the + # run shows as failed — this lets ``gh run rerun --failed`` + # (e.g. from label-rerun.yml) pick up and rerun the failed jobs. + failed_deps = [name for name, result in completed.items() if result == "failure"] + if failed_deps: + print(f" All jobs done, but {len(failed_deps)} failed: {', '.join(failed_deps)}") + print(" Exiting with error so the run can be rerun via --failed.") + return 1 + print(" All jobs completed — done.") + break + + time.sleep(interval) + + return 0 + + +def main() -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--interval", type=int, default=15, + help="Seconds between polls (default: 15).") + parser.add_argument("--timeout", type=int, default=1800, + help="Max seconds to poll before giving up (default: 1800).") + parser.add_argument("--review-statuses-file", type=Path, default=None, + help="Path to a JSON file with merged review statuses from workflow_call jobs.") + parser.add_argument("--dry-run", action="store_true", + help="Print comment body instead of posting to PR.") + args = parser.parse_args() + + token = os.environ.get("GITHUB_TOKEN", "") + repo = os.environ.get("GITHUB_REPOSITORY", "") + run_id = os.environ.get("GITHUB_RUN_ID", "") + pr_number = os.environ.get("PR_NUMBER", "") + run_url = os.environ.get("RUN_URL", "") + + if not args.dry_run: + if not token: + print("GITHUB_TOKEN is required", file=sys.stderr) + return 1 + if not repo: + print("GITHUB_REPOSITORY is required", file=sys.stderr) + return 1 + if not run_id: + print("GITHUB_RUN_ID is required", file=sys.stderr) + return 1 + if not pr_number: + print("PR_NUMBER is required", file=sys.stderr) + return 1 + + # Read merged review statuses from file (prepared by the ci.yml step). + review_statuses_json = "" + if args.review_statuses_file: + try: + review_statuses_json = args.review_statuses_file.read_text(encoding="utf-8") + except OSError as e: + print(f"Warning: could not read review statuses file: {e}", file=sys.stderr) + + # Build commit info line from env vars (set by ci.yml). + commit_sha = os.environ.get("COMMIT_SHA", "") + commit_msg = os.environ.get("COMMIT_MESSAGE", "") + commit_url = os.environ.get("COMMIT_URL", "") + commit_info = "" + if commit_sha: + short_sha = commit_sha[:7] + if commit_msg: + # Truncate commit message to first line, max 60 chars. + first_line = commit_msg.split("\n")[0][:60] + if commit_url: + commit_info = f"running on [{short_sha}]({commit_url}) — {first_line}" + else: + commit_info = f"running on {short_sha} — {first_line}" + elif commit_url: + commit_info = f"running on [{short_sha}]({commit_url})" + else: + commit_info = f"running on {short_sha}" + + return run( + token=token, + repo=repo, + run_id=run_id, + pr_number=pr_number, + run_url=run_url, + review_statuses_json=review_statuses_json, + commit_info=commit_info, + interval=args.interval, + timeout=args.timeout, + dry_run=args.dry_run, + ) + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/scripts/ci/lockfile_diff.py b/scripts/ci/lockfile_diff.py index 0a0e03c691ed..46712ad3760e 100644 --- a/scripts/ci/lockfile_diff.py +++ b/scripts/ci/lockfile_diff.py @@ -15,11 +15,11 @@ Usage (from a checkout that still has the base ref available): --output diff.md [--repo-root .] Reads every ``package-lock.json`` tracked at either ref (top-level and -nested — the repo has several), diffs each, and writes a Markdown report -to ``--output``. Exits 0 always; an empty report file means "no version -changes" (the caller uses that to decide whether to post/update the PR -comment). The report embeds ``COMMENT_MARKER`` so the workflow can find -and update its own previous comment instead of stacking new ones. +nested — the repo has several), diffs each, and writes a Markdown fragment +to ``--output``. Exits 0 always; an empty output file means "no version +changes" (the caller uses that to decide whether to include the section). +The fragment is consumed by ``scripts/ci/assemble_review_comment.py``, +which wraps it in a section with a header and action note. """ from __future__ import annotations @@ -29,9 +29,6 @@ import json import subprocess import sys -# Hidden marker used to locate the bot's previous comment for in-place update. -COMMENT_MARKER = "" - def parse_lockfile(text: str) -> dict[str, str]: """Reduce lockfile JSON to ``{install path: version}``. @@ -79,21 +76,24 @@ def _display_name(path: str) -> str: def render_markdown(diffs: dict[str, dict[str, list]]) -> str: - """Render per-lockfile diffs as a Markdown PR comment body. + """Render per-lockfile diffs as a Markdown fragment. ``diffs`` maps lockfile repo-path → the output of :func:`diff_locks`. Lockfiles with no version changes are omitted. Returns ``""`` when - nothing changed anywhere (caller skips commenting entirely). + nothing changed anywhere (caller skips the section entirely). + + The output is a fragment — per-lockfile ``####`` subsections with + tables — not a standalone comment. The ``assemble_review_comment`` + script wraps this in a section with its own header and action note, + so no top-level header or comment marker is emitted here. """ sections = [] - total = 0 for lockfile, d in sorted(diffs.items()): added, removed, updated = d["added"], d["removed"], d["updated"] n = len(added) + len(removed) + len(updated) if n == 0: continue - total += n - lines = [f"### `{lockfile}`", ""] + lines = [f"#### `{lockfile}`", ""] lines.append("| Package | Before | After |") lines.append("| --- | --- | --- |") for path, old, new in updated: @@ -107,13 +107,7 @@ def render_markdown(diffs: dict[str, dict[str, list]]) -> str: if not sections: return "" - header = ( - f"{COMMENT_MARKER}\n" - f"## ⚠️ `package-lock.json` changes ({total} package" - f"{'s' if total != 1 else ''})\n\n" - "This PR changes locked npm dependency versions." - ) - return header + "\n" + "\n\n".join(sections) + "\n" + return "\n\n".join(sections) + "\n" def _git_show(ref: str, path: str, repo_root: str) -> str | None: diff --git a/scripts/ci/timings_report.py b/scripts/ci/timings_report.py index 0e19e539c63a..f28bd9d703f8 100644 --- a/scripts/ci/timings_report.py +++ b/scripts/ci/timings_report.py @@ -895,6 +895,86 @@ def generate_summary(timings: dict, baseline: dict | None = None) -> str: return "\n".join(lines) +# --------------------------------------------------------------------------- +# Review status JSON for the unified PR comment +# --------------------------------------------------------------------------- + +# Wall-time regressions above this fraction of baseline are "warning" severity. +_TIMINGS_WARN_PCT = 0.25 + + +def generate_review_status( + timings: dict, baseline: dict | None, report_url: str | None = None +) -> list[dict]: + """Produce a review_status JSON array for the CI timings review section. + + Returns a list with one ``{source, results: [...]}`` entry. The + result kind is ``"info"`` or ``"warning"`` (timings is never error — + it's an observability job). *summary* is a single short line suitable + for the PR comment. *detail* has the per-job deltas as a markdown + fragment. + """ + stats = compute_stats(timings, baseline) + + if baseline is None: + severity = "info" + summary = f"Wall time {fmt_dur(stats['wall'])} (no baseline yet)." + else: + wall = stats["wall"] + bl_wall = stats["bl_wall"] or 0 + if bl_wall > 0: + pct = (wall - bl_wall) / bl_wall * 100 + wall_str = f"Wall time {fmt_dur(wall)} vs {fmt_dur(bl_wall)} ({pct:+.1f}%)." + if pct > _TIMINGS_WARN_PCT * 100: + severity = "warning" + else: + severity = "info" + else: + wall_str = f"Wall time {fmt_dur(wall)}." + severity = "info" + + if stats["slower"]: + wall_str += f" {stats['slower']} job(s) slower," + if stats["faster"]: + wall_str += f" {stats['faster']} faster," + if stats["unchanged"]: + wall_str += f" {stats['unchanged']} unchanged." + summary = wall_str + + # Per-job delta detail (top 5 by absolute change) + detail_lines: list[str] = [] + if baseline: + bl_map = {j["name"]: j for j in baseline.get("jobs", [])} + deltas: list[tuple[float, str, str]] = [] + for j in timings.get("jobs", []): + if is_skipped(j): + continue + bl = bl_map.get(j["name"]) + if not bl or is_skipped(bl): + continue + cur = j.get("duration_s") or 0 + bl_d = bl.get("duration_s") or 0 + diff = cur - bl_d + if abs(diff) < 1.0: + continue + deltas.append((abs(diff), j["name"], f"{diff:+.1f}s")) + deltas.sort(reverse=True) + for _, name, delta_str in deltas[:5]: + detail_lines.append(f"- {name}: {delta_str}") + + result: dict = { + "kind": severity, + "title": "CI timings", + "summary": summary, + "detail": "\n".join(detail_lines), + } + if report_url: + result["link"] = report_url + result["link_label"] = "View report" + + return [{"source": "ci timing", "results": [result]}] + + # --------------------------------------------------------------------------- # Main # --------------------------------------------------------------------------- @@ -916,6 +996,8 @@ def main(): help="JSON output path (default: ci-timings.json)") parser.add_argument("--summary-out", default="ci-timings-summary.md", help="Markdown summary output path (default: ci-timings-summary.md)") + parser.add_argument("--review-status-out", default="", + help="If set, write a review-status JSON for the unified PR comment.") args = parser.parse_args() # Collect or load timings @@ -974,6 +1056,17 @@ def main(): f.write(summary) print(f"Wrote summary to {args.summary_out}") + # Write review status for the unified PR comment. + # The output goes to GITHUB_OUTPUT (or a file with the same key=value + # format) so the ci-timings job can expose it as a workflow_call output. + if args.review_status_out: + report_url = os.environ.get("CI_TIMINGS_REPORT_URL", "") + statuses = generate_review_status(timings, baseline, report_url) + json_str = json.dumps(statuses) + with open(args.review_status_out, "a", encoding="utf-8") as f: + f.write(f"review_status={json_str}\n") + print(f"Wrote review status to {args.review_status_out}") + if __name__ == "__main__": main() diff --git a/tests/ci/test_assemble_review_comment.py b/tests/ci/test_assemble_review_comment.py new file mode 100644 index 000000000000..373fb6a3a47f --- /dev/null +++ b/tests/ci/test_assemble_review_comment.py @@ -0,0 +1,643 @@ +"""Tests for scripts/ci/assemble_review_comment.py. + +The assembler collects status from every CI sub-workflow into ReviewItems +classified by severity (error / action_required / warning / info), then +renders them into a single PR comment body. + +Status data comes from two sources: + 1. --review-statuses-json: JSON array of {source, results: [...]} objects + from workflow_call jobs. Each result has kind/title/summary/detail/ + how_to_fix/link. The assembler flattens all results into ReviewItems. + 2. --needs-json: {job_name: result} from all-checks-pass. Failed jobs not + claimed by any status become synthesized ❌ Error items. + +Layout rules tested here: + - group headers: ## ❌ Job failures, ## ⚠️ Action required, ## ⚠️ Warnings + - each item is a ### section under its group header + - errors + action_required always visible + - warnings shown only when present + - info in a collapsible
block + - sections separated by --- + - how_to_fix rendered at bottom of action_required items + - empty → clean banner + - jobs with declared statuses excluded from failed-jobs list + - per-job URLs used for failed job links when available +""" + +from __future__ import annotations + +import importlib.util +import json +import sys +from pathlib import Path + +import pytest + +_PATH = Path(__file__).resolve().parents[2] / "scripts" / "ci" / "assemble_review_comment.py" +_spec = importlib.util.spec_from_file_location("assemble_review_comment", _PATH) +if _spec is None or _spec.loader is None: + raise ImportError("Failed to load assemble_review_comment.py") +_mod = importlib.util.module_from_spec(_spec) +sys.modules["assemble_review_comment"] = _mod +_spec.loader.exec_module(_mod) + +MARKER = _mod.MARKER +ReviewItem = _mod.ReviewItem + + +def _status(source: str, results: list[dict]) -> str: + """Helper: build a review_statuses JSON string with one source entry.""" + return json.dumps([{"source": source, "results": results}]) + + +# ─── collect_from_statuses ────────────────────────────────────────── + + +def test_statuses_empty_json(): + items, sources = _mod.collect_from_statuses("") + assert items == [] + assert sources == set() + + +def test_statuses_bad_json(): + items, sources = _mod.collect_from_statuses("not json") + assert items == [] + assert sources == set() + + +def test_statuses_action_required(): + statuses = _status("review-label-gate", [{ + "kind": "action_required", + "title": "CI-sensitive file review", + "summary": "Changes detected.", + "how_to_fix": "Add the label.", + }]) + items, sources = _mod.collect_from_statuses(statuses) + assert len(items) == 1 + assert items[0].severity == "action_required" + assert items[0].title == "CI-sensitive file review" + assert items[0].how_to_fix == "Add the label." + assert items[0].source == "review-label-gate" + assert sources == {"review-label-gate"} + + +def test_statuses_info(): + statuses = _status("review-label-gate", [{ + "kind": "info", + "title": "CI-sensitive file review", + "summary": "Label present.", + }]) + items, sources = _mod.collect_from_statuses(statuses) + assert len(items) == 1 + assert items[0].severity == "info" + assert sources == {"review-label-gate"} + + +def test_statuses_multiple_results_same_source(): + """One source can emit multiple results of different kinds.""" + statuses = _status("review-label-gate", [ + {"kind": "action_required", "title": "CI review", "summary": "Missing label."}, + {"kind": "action_required", "title": "MCP review", "summary": "Missing label."}, + ]) + items, sources = _mod.collect_from_statuses(statuses) + assert len(items) == 2 + assert sources == {"review-label-gate"} + + +def test_statuses_mixed_kinds_same_source(): + """One source can emit both a warning and an info.""" + statuses = _status("ci-timings", [ + {"kind": "warning", "title": "CI timings", "summary": "Slower."}, + {"kind": "info", "title": "Baseline", "summary": "OK."}, + ]) + items, sources = _mod.collect_from_statuses(statuses) + assert len(items) == 2 + assert items[0].severity == "warning" + assert items[1].severity == "info" + assert sources == {"ci-timings"} + + +def test_statuses_multiple_sources(): + statuses = json.dumps([ + {"source": "review-label-gate", "results": [ + {"kind": "action_required", "title": "CI review", "summary": "Missing."}, + ]}, + {"source": "lockfile-diff", "results": [ + {"kind": "info", "title": "package-lock.json", "summary": "No changes."}, + ]}, + ]) + items, sources = _mod.collect_from_statuses(statuses) + assert len(items) == 2 + assert sources == {"review-label-gate", "lockfile-diff"} + + +def test_statuses_unknown_kind_becomes_info(): + statuses = _status("some-job", [{ + "kind": "bogus", + "title": "X", + "summary": "Y", + }]) + items, _ = _mod.collect_from_statuses(statuses) + assert items[0].severity == "info" + + +def test_statuses_no_source(): + """Status without a source — still rendered, just not excluded from errors.""" + statuses = json.dumps([{ + "results": [{"kind": "info", "title": "X", "summary": "Y"}], + }]) + items, sources = _mod.collect_from_statuses(statuses) + assert len(items) == 1 + assert sources == set() + + +def test_statuses_passes_through_optional_fields(): + statuses = _status("ci-timings", [{ + "kind": "warning", + "title": "CI timings", + "summary": "Slower.", + "detail": "- job: +5s", + "link": "https://report", + "link_label": "View report", + "how_to_fix": "Optimize.", + }]) + items, _ = _mod.collect_from_statuses(statuses) + assert items[0].detail == "- job: +5s" + assert items[0].link == "https://report" + assert items[0].link_label == "View report" + assert items[0].how_to_fix == "Optimize." + + +# ─── collect_failed_jobs ───────────────────────────────────────────── + + +def test_failed_jobs_empty_needs(): + assert _mod.collect_failed_jobs("", "https://run") == [] + + +def test_failed_jobs_no_failures(): + needs = json.dumps({"tests": "success", "lint": "skipped"}) + assert _mod.collect_failed_jobs(needs, "https://run") == [] + + +def test_failed_jobs_collects_only_failures(): + needs = json.dumps({"tests": "success", "lint": "failure", "js-tests": "failure"}) + items = _mod.collect_failed_jobs(needs, "https://run/123") + assert len(items) == 2 + assert all(i.severity == "error" for i in items) + # sorted by name + names = [i.title for i in items] + assert names == ["js-tests", "lint"] + assert all(i.job_url == "https://run/123" for i in items) + + +def test_failed_jobs_bad_json(): + assert _mod.collect_failed_jobs("not json", "https://run") == [] + + +def test_failed_jobs_excluded_by_source(): + """Jobs whose name contains a declared source are excluded.""" + needs = json.dumps({ + "Review label gate / Review label gate": "failure", + "tests": "failure", + }) + items = _mod.collect_failed_jobs(needs, "https://run", exclude_sources={"review-label-gate"}) + assert len(items) == 1 + assert items[0].title == "tests" + + +def test_failed_jobs_no_exclusion_without_sources(): + """Without exclude_sources, all failures are shown.""" + needs = json.dumps({"review-label-gate": "failure", "tests": "failure"}) + items = _mod.collect_failed_jobs(needs, "https://run") + assert len(items) == 2 + + +def test_failed_jobs_per_job_url(): + """When job_urls is provided, the link points to the specific job.""" + needs = json.dumps({"tests": "failure", "lint": "failure"}) + job_urls = {"tests": "https://run/1/job/2", "lint": "https://run/1/job/3"} + items = _mod.collect_failed_jobs(needs, "https://fallback", job_urls=job_urls) + assert len(items) == 2 + urls = {i.title: i.job_url for i in items} + assert urls["tests"] == "https://run/1/job/2" + assert urls["lint"] == "https://run/1/job/3" + + +def test_failed_jobs_fallback_to_run_url(): + """Jobs not in job_urls fall back to run_url.""" + needs = json.dumps({"tests": "failure", "lint": "failure"}) + job_urls = {"tests": "https://run/1/job/2"} + items = _mod.collect_failed_jobs(needs, "https://fallback", job_urls=job_urls) + urls = {i.title: i.job_url for i in items} + assert urls["tests"] == "https://run/1/job/2" + assert urls["lint"] == "https://fallback" + + +# ─── render_comment ─────────────────────────────────────────────────── + + +def test_render_empty_shows_clean_banner(): + """Completely clean — dog kaomoji + 'looks good' banner, no sections.""" + body = _mod.render_comment([]) + assert body.startswith(MARKER) + assert "૮ >ﻌ< ა" in body + assert "looks good to me!" in body + assert "##" not in body # no section headers + + +def test_render_info_only_shows_details(): + """Info items only — header + collapsible details, no blocking sections.""" + items = [ + ReviewItem(severity="info", title="lockfile", summary="No changes."), + ReviewItem(severity="info", title="timings", summary="OK."), + ] + body = _mod.render_comment(items) + assert "૮ >ﻌ< ა" in body + assert "
" in body + assert "
" in body + assert "No changes." in body + assert "OK." in body + # No blocking sections + assert "## ❌" not in body + assert "## ⚠️" not in body + + +def test_render_info_only_with_pending_shows_details_plus_footer(): + items = [ReviewItem(severity="info", title="lockfile", summary="No changes.")] + body = _mod.render_comment(items, pending_jobs=["ci-timings"]) + assert "૮ >ﻌ< ა" in body + assert "Still running" in body + assert "
" in body + assert "Still running" in body + assert "`ci-timings`" in body + + +def test_render_group_header_for_errors(): + """Errors appear under a '## ❌ Job failures' group header.""" + items = [ + ReviewItem(severity="error", title="tests", summary="Job **tests** failed.", link="https://run"), + ReviewItem(severity="error", title="lint", summary="Job **lint** failed.", link="https://run"), + ] + body = _mod.render_comment(items) + assert "## ❌ Job failures" in body + assert "### tests" in body + assert "### lint" in body + assert body.index("## ❌ Job failures") < body.index("### tests") + + +def test_render_group_header_for_action_required(): + items = [ + ReviewItem(severity="action_required", title="CI review", summary="Need label."), + ] + body = _mod.render_comment(items) + assert "## ⚠️ Action required" in body + assert "### CI review" in body + + +def test_render_group_header_for_warnings(): + items = [ + ReviewItem(severity="warning", title="CI timings", summary="Slower."), + ] + body = _mod.render_comment(items) + assert "## ⚠️ Warnings" in body + assert "### CI timings" in body + assert "
" not in body + + items2 = [ReviewItem(severity="info", title="x", summary="y")] + body2 = _mod.render_comment(items2) + assert "## ⚠️ Warnings" not in body2 + + +def test_render_no_duplicated_severity_in_item_body(): + """Items don't repeat the severity label — the group header carries it.""" + items = [ReviewItem(severity="error", title="tests", summary="Job failed.", link="https://run")] + body = _mod.render_comment(items) + assert "### tests" in body + assert "Job failed." in body + assert "**❌ Error**" not in body + + +def test_render_how_to_fix_at_bottom(): + items = [ + ReviewItem(severity="action_required", title="CI review", summary="Need label.", + how_to_fix="Add the `ci-reviewed` label."), + ] + body = _mod.render_comment(items) + assert "**How to fix:**" in body + assert "Add the `ci-reviewed` label." in body + assert body.index("Need label.") < body.index("How to fix") + + +def test_render_sections_separated_by_hr(): + items = [ + ReviewItem(severity="error", title="tests", summary="failed."), + ReviewItem(severity="action_required", title="CI review", summary="need label."), + ] + body = _mod.render_comment(items) + assert "\n\n---\n\n" in body + + +def test_render_errors_always_visible(): + items = [ + ReviewItem(severity="error", title="tests", summary="Job **tests** failed.", job_url="https://run"), + ReviewItem(severity="info", title="lockfile", summary="No changes."), + ] + body = _mod.render_comment(items) + assert "## ❌ Job failures" in body + assert "### tests" in body + assert "Job **tests** failed." in body + assert "[View job](https://run)" in body + assert "
" in body + assert "No changes." in body + + +def test_render_info_in_collapsible_details(): + """Each info item is its own
block.""" + items = [ + ReviewItem(severity="info", title="lockfile", summary="No changes."), + ReviewItem(severity="info", title="timings", summary="OK."), + ] + body = _mod.render_comment(items) + assert body.count("
") == 2 + assert body.count("
") == 2 + assert "lockfile" in body + assert "timings" in body + assert "No changes." in body + assert "OK." in body + + +def test_render_order_errors_then_action_then_warn_then_info(): + items = [ + ReviewItem(severity="info", title="i", summary="info"), + ReviewItem(severity="warning", title="w", summary="warn"), + ReviewItem(severity="action_required", title="a", summary="action"), + ReviewItem(severity="error", title="e", summary="error"), + ] + body = _mod.render_comment(items) + error_pos = body.index("## ❌ Job failures") + action_pos = body.index("## ⚠️ Action required") + warn_pos = body.index("## ⚠️ Warnings") + info_pos = body.index("
") + assert error_pos < action_pos < warn_pos < info_pos + + +# ─── render_comment (pending jobs) ──────────────────────────────────── + + +def test_render_pending_only_shows_header_with_clock(): + """Pending jobs only — header has 'still waiting', footer lists jobs, no sections.""" + body = _mod.render_comment([], pending_jobs=["ci-timings"]) + assert body.startswith(MARKER) + assert "૮ >ﻌ< ა" in body + assert "Still running" in body + assert "`ci-timings`" in body + assert "##" not in body + + +def test_render_pending_notif(): + items = [ReviewItem(severity="info", title="lockfile", summary="No changes.")] + body = _mod.render_comment(items, pending_jobs=["ci-timings"]) + assert "૮ >ﻌ< ა" in body + assert "Still running 1 job: `ci-timings`" in body + + +def test_render_pending_multiple_jobs_sorted(): + body = _mod.render_comment([], pending_jobs=["docker", "ci-timings"]) + assert "`ci-timings`" in body + assert "`docker`" in body + assert body.index("`ci-timings`") < body.index("`docker`") + + +def test_render_no_pending_no_footer(): + items = [ReviewItem(severity="info", title="x", summary="y")] + body = _mod.render_comment(items) + assert "Still running" not in body + + +# ─── assemble (integration) ────────────────────────────────────────── + + +def test_assemble_all_skipped_clean_banner(): + body = _mod.assemble() + assert body.startswith(MARKER) + assert "૮ >ﻌ< ა" in body + assert "looks good to me!" in body + assert "##" not in body + + +def test_assemble_failed_job_shown(): + needs = json.dumps({"tests": "failure", "lint": "success"}) + body = _mod.assemble(needs_json=needs, run_url="https://run/1") + assert "## ❌ Job failures" in body + assert "### tests" in body + assert "[View job](https://run/1)" in body + + +def test_assemble_with_review_statuses(): + """Statuses from review-labels render directly + exclude gate from errors.""" + statuses = _status("review-label-gate", [{ + "kind": "action_required", + "title": "CI-sensitive file review", + "summary": "Changes detected.", + "how_to_fix": "Add the label.", + }]) + needs = json.dumps({ + "Review label gate / Review label gate": "failure", + "tests": "success", + }) + body = _mod.assemble( + needs_json=needs, + run_url="https://run", + review_statuses_json=statuses, + ) + assert "## ⚠️ Action required" in body + assert "### CI-sensitive file review" in body + assert "Add the label." in body + assert "## ❌ Job failures" not in body + + +def test_assemble_pending_jobs(): + body = _mod.assemble(pending_jobs=["ci-timings"]) + assert "Still running" in body + assert "`ci-timings`" in body + + +def test_assemble_with_items_and_pending(): + needs = json.dumps({"tests": "failure"}) + body = _mod.assemble(needs_json=needs, run_url="https://run", pending_jobs=["ci-timings"]) + assert "## ❌ Job failures" in body + assert "### tests" in body + assert "Still running" in body + assert "`ci-timings`" in body + + +def test_assemble_with_timings_status(): + """Timings status from the nested format renders as info or warning.""" + statuses = _status("ci-timings", [{ + "kind": "info", + "title": "CI timings", + "summary": "Wall time 3m (no baseline yet).", + "detail": "", + "link": "https://report", + }]) + body = _mod.assemble(review_statuses_json=statuses) + assert "
" in body + assert "### CI timings" in body + assert "Wall time 3m" in body + assert "## ❌" not in body + assert "## ⚠️" not in body + + +def test_assemble_with_lockfile_status(): + """Lockfile no-changes status renders as info in the details block.""" + statuses = _status("lockfile-diff", [{ + "kind": "info", + "title": "package-lock.json", + "summary": "No lockfile changes — locked versions match the target branch.", + }]) + body = _mod.assemble(review_statuses_json=statuses) + assert "
" in body + assert "### package-lock.json" in body + assert "No lockfile changes" in body + + +def test_assemble_with_lockfile_changed_status(): + """Lockfile changed status renders as action_required with ci-reviewed how_to_fix.""" + statuses = _status("lockfile-diff", [{ + "kind": "action_required", + "title": "package-lock.json", + "summary": "Locked npm dependency versions changed.", + "detail": "#### `package-lock.json`\n\n| col | | |", + "how_to_fix": "Add the `ci-reviewed` label after verifying the version changes are expected.", + }]) + body = _mod.assemble(review_statuses_json=statuses) + assert "## ⚠️ Action required" in body + assert "### package-lock.json" in body + assert "Locked npm dependency versions changed." in body + assert "Add the `ci-reviewed` label" in body + assert "
" not in body # action_required is not in the collapsible block + + +# ─── _attach_job_urls ──────────────────────────────────────────────── + + +def test_attach_job_urls_fills_missing_links(): + """Items without a link get one from job_urls via source matching.""" + items = [ + ReviewItem(severity="info", title="Supply chain scan", + summary="No risks.", source="supply chain"), + ReviewItem(severity="warning", title="CI timings", + summary="Slower.", source="ci timings", + link="https://report"), # already has a link + ] + job_urls = { + "Supply Chain Audit / Scan PR for critical supply chain risks": "https://run/1/job/2", + } + _mod._attach_job_urls(items, job_urls, "https://fallback") + # First item gets the per-job URL as job_url (link untouched) + assert items[0].job_url == "https://run/1/job/2" + assert items[0].link == "" # no emitted link + # Second item keeps its existing link, job_url is set separately + assert items[1].link == "https://report" + assert items[1].job_url == "https://fallback" # fell back to run_url + + +def test_attach_job_urls_fallback_to_run_url(): + """Items with a source but no matching job URL fall back to run_url.""" + items = [ + ReviewItem(severity="info", title="X", summary="Y", source="some-job"), + ] + _mod._attach_job_urls(items, {}, "https://fallback") + assert items[0].job_url == "https://fallback" + + +def test_attach_job_urls_no_source_no_link(): + """Items without a source don't get a link.""" + items = [ + ReviewItem(severity="info", title="X", summary="Y"), # no source + ] + _mod._attach_job_urls(items, {"job": "https://run/1"}, "https://fallback") + assert items[0].job_url == "" + + +def test_assemble_attaches_links_to_all_items(): + """Integration: assemble() attaches job URLs to status items, not just errors.""" + statuses = _status("supply chain", [{ + "kind": "info", + "title": "Supply chain scan", + "summary": "No risks.", + }]) + job_urls = { + "Supply Chain Audit / Scan PR for critical supply chain risks": "https://run/1/job/2", + } + body = _mod.assemble( + review_statuses_json=statuses, + job_urls=job_urls, + run_url="https://fallback", + ) + assert "[View job](https://run/1/job/2)" in body + + +def test_render_commit_info_below_header(): + """Commit info is rendered below the header, above the content.""" + body = _mod.render_comment( + [ReviewItem(severity="error", title="tests", summary="failed.")], + commit_info="running on [abc1234](https://commit-url) — fix: thing", + ) + assert "# ૮ >ﻌ< ა ci review" in body + assert "running on [abc1234](https://commit-url)" in body + assert "fix: thing" in body + # Commit info appears before the content + assert body.index("abc1234") < body.index("## ❌") + + +def test_render_no_commit_info_when_empty(): + """No commit_info → no extra line below header.""" + body = _mod.render_comment([]) + assert "running on" not in body + + +def test_assemble_passes_commit_info(): + """assemble() passes commit_info through to render_comment.""" + body = _mod.assemble(commit_info="running on abc1234") + assert "running on abc1234" in body + assert "looks good to me!" in body + + +def test_render_both_emitted_link_and_job_url(): + """An item with both an emitted link and a job_url shows both.""" + item = ReviewItem( + severity="warning", + title="CI timings", + summary="Slower.", + link="https://artifact/report.html", + link_label="View report", + source="ci timings", + job_url="https://github.com/run/1/job/5", + ) + body = _mod.render_comment([item]) + assert "[View report](https://artifact/report.html)" in body + assert "[View job](https://github.com/run/1/job/5)" in body + # Both links on the same line, separated by · + assert " · " in body + + +def test_assemble_both_links_for_ci_timings(): + """Integration: ci-timings has a report URL (link) AND gets a job_url.""" + statuses = _status("ci timings", [{ + "kind": "warning", + "title": "CI timings", + "summary": "Wall time 5m vs 3m (+66%).", + "detail": "- tests: +120s", + "link": "https://artifact/report.html", + "link_label": "View report", + }]) + job_urls = {"CI timings": "https://github.com/run/1/job/5"} + body = _mod.assemble( + review_statuses_json=statuses, + job_urls=job_urls, + run_url="https://fallback", + ) + assert "[View report](https://artifact/report.html)" in body + assert "[View job](https://github.com/run/1/job/5)" in body diff --git a/tests/ci/test_live_comment.py b/tests/ci/test_live_comment.py new file mode 100644 index 000000000000..ffc952921310 --- /dev/null +++ b/tests/ci/test_live_comment.py @@ -0,0 +1,162 @@ +"""Tests for scripts/ci/live_comment.py — classify_jobs(). + +The poller's core logic is a pure function: take raw GitHub API job dicts +and split them into (completed, pending). The API wrapper + polling loop +are tested via E2E in CI, not here. +""" + +from __future__ import annotations + +import importlib.util +import sys +from pathlib import Path + +_PATH = Path(__file__).resolve().parents[2] / "scripts" / "ci" / "live_comment.py" +_spec = importlib.util.spec_from_file_location("live_comment", _PATH) +if _spec is None or _spec.loader is None: + raise ImportError("Failed to load live_comment.py") +_mod = importlib.util.module_from_spec(_spec) +sys.modules["live_comment"] = _mod +_spec.loader.exec_module(_mod) + + +def _job(name: str, status: str, conclusion: str | None = None, workflow: str = "") -> dict: + """Build a raw API job dict.""" + j = {"name": name, "status": status, "conclusion": conclusion} + if workflow: + j["_workflow_name"] = workflow + return j + + +def test_classify_empty(): + completed, pending, job_urls = _mod.classify_jobs([]) + assert completed == {} + assert pending == [] + assert job_urls == {} + + +def test_classify_success(): + jobs = [_job("Python tests", "completed", "success")] + completed, pending, job_urls = _mod.classify_jobs(jobs) + assert completed == {"Python tests": "success"} + assert pending == [] + + +def test_classify_failure(): + jobs = [_job("Python tests", "completed", "failure")] + completed, pending, job_urls = _mod.classify_jobs(jobs) + assert completed == {"Python tests": "failure"} + assert pending == [] + + +def test_classify_skipped(): + jobs = [_job("Python tests", "completed", "skipped")] + completed, pending, job_urls = _mod.classify_jobs(jobs) + assert completed == {"Python tests": "skipped"} + assert pending == [] + + +def test_classify_in_progress(): + jobs = [_job("Python tests", "in_progress", None)] + completed, pending, job_urls = _mod.classify_jobs(jobs) + assert completed == {} + assert pending == ["Python tests"] + + +def test_classify_queued(): + jobs = [_job("Python tests", "queued", None)] + completed, pending, job_urls = _mod.classify_jobs(jobs) + assert completed == {} + assert pending == ["Python tests"] + + +def test_classify_waiting(): + jobs = [_job("Python tests", "waiting", None)] + completed, pending, job_urls = _mod.classify_jobs(jobs) + assert completed == {} + assert pending == ["Python tests"] + + +def test_classify_mixed(): + jobs = [ + _job("Python tests", "completed", "success"), + _job("Python lints", "completed", "failure"), + _job("JS & TS checks", "in_progress", None), + _job("Desktop E2E", "queued", None), + ] + completed, pending, job_urls = _mod.classify_jobs(jobs) + assert completed == {"Python tests": "success", "Python lints": "failure"} + assert set(pending) == {"JS & TS checks", "Desktop E2E"} + + +def test_classify_infra_jobs_excluded(): + """Infra jobs (detect, all-checks-pass, comment-live) are never shown.""" + jobs = [ + _job("detect", "completed", "success"), + _job("Detect affected areas", "completed", "success"), + _job("all-checks-pass", "completed", "success"), + _job("All required checks pass", "completed", "success"), + _job("comment-live", "in_progress", None), + _job("CI review comment (live)", "in_progress", None), + _job("Python tests", "completed", "success"), + ] + completed, pending, job_urls = _mod.classify_jobs(jobs) + assert completed == {"Python tests": "success"} + assert pending == [] + + +def test_classify_sub_workflow_jobs_prefixed(): + """Sub-workflow jobs get 'Workflow / job' display names.""" + jobs = [ + _job("test", "completed", "success", workflow="Tests"), + _job("check", "in_progress", None, workflow="JS Tests"), + ] + completed, pending, job_urls = _mod.classify_jobs(jobs) + assert "Tests / test" in completed + assert completed["Tests / test"] == "success" + assert "JS Tests / check" in pending + + +def test_classify_captures_html_url(): + """The poller captures html_url per job for per-job log links.""" + jobs = [ + {**_job("Python tests", "completed", "failure"), + "html_url": "https://github.com/repo/actions/runs/1/job/2"}, + _job("Python lints", "completed", "success"), + ] + completed, pending, job_urls = _mod.classify_jobs(jobs) + assert job_urls["Python tests"] == "https://github.com/repo/actions/runs/1/job/2" + # Jobs without html_url are simply absent from the dict + assert "Python lints" not in job_urls + + +def test_classify_cancelled_treated_as_skipped(): + jobs = [_job("Python tests", "completed", "cancelled")] + completed, pending, job_urls = _mod.classify_jobs(jobs) + assert completed == {"Python tests": "skipped"} + + +def test_classify_timed_out_treated_as_failure(): + jobs = [_job("Python tests", "completed", "timed_out")] + completed, pending, job_urls = _mod.classify_jobs(jobs) + assert completed == {"Python tests": "failure"} + + +def test_classify_neutral_treated_as_skipped(): + jobs = [_job("Python tests", "completed", "neutral")] + completed, pending, job_urls = _mod.classify_jobs(jobs) + assert completed == {"Python tests": "skipped"} + + +def test_classify_action_required_treated_as_skipped(): + jobs = [_job("Python tests", "completed", "action_required")] + completed, pending, job_urls = _mod.classify_jobs(jobs) + assert completed == {"Python tests": "skipped"} + + +def test_classify_unknown_status_skipped(): + """Unknown status values are silently ignored, not crashed on.""" + jobs = [_job("weird-job", "unknown_status", None)] + completed, pending, job_urls = _mod.classify_jobs(jobs) + assert completed == {} + assert pending == [] diff --git a/tests/ci/test_lockfile_diff.py b/tests/ci/test_lockfile_diff.py index 0554ed473331..46b3ff01d745 100644 --- a/tests/ci/test_lockfile_diff.py +++ b/tests/ci/test_lockfile_diff.py @@ -89,15 +89,14 @@ def test_nested_dedup_is_distinct_entry(): assert d["updated"] == [("node_modules/foo/node_modules/react", "17.0.2", "17.0.3")] -def test_render_markdown_contains_marker_and_versions(): +def test_render_markdown_contains_versions_and_nested_display(): d = _mod.diff_locks( _mod.parse_lockfile(BASE), _mod.parse_lockfile(_lock({"node_modules/react": {"version": "19.0.0"}})), ) md = _mod.render_markdown({"apps/desktop/package-lock.json": d}) - assert md.startswith(_mod.COMMENT_MARKER) # workflow finds its comment by prefix - assert "⚠️" in md - assert "`apps/desktop/package-lock.json`" in md + # Fragment starts directly with the per-lockfile subsection header. + assert md.startswith("#### `apps/desktop/package-lock.json`") assert "`18.2.0`" in md and "`19.0.0`" in md # nested display name keeps the parent chain visible assert "nested under foo" in md diff --git a/tests/ci/test_timings_report.py b/tests/ci/test_timings_report.py new file mode 100644 index 000000000000..dee48c09f309 --- /dev/null +++ b/tests/ci/test_timings_report.py @@ -0,0 +1,143 @@ +"""Tests for scripts/ci/timings_report.py — generate_review_status(). + +The review status is a JSON array in the unified nested format consumed +by the review comment assembler. It classifies the CI timings result as +info/warning (never error — timings is an observability job, not a gate) +and provides a one-line summary plus optional per-job delta detail. +""" + +from __future__ import annotations + +import importlib.util +from datetime import datetime, timezone +from pathlib import Path + +_PATH = Path(__file__).resolve().parents[2] / "scripts" / "ci" / "timings_report.py" +_spec = importlib.util.spec_from_file_location("timings_report", _PATH) +if _spec is None or _spec.loader is None: + raise ImportError("Failed to load timings_report.py") +_mod = importlib.util.module_from_spec(_spec) +_spec.loader.exec_module(_mod) + +_T0 = datetime(2025, 1, 1, 0, 0, 0, tzinfo=timezone.utc) + + +def _ts(seconds: float) -> str: + """ISO timestamp `seconds` after T0.""" + dt = _T0.timestamp() + seconds + return datetime.fromtimestamp(dt, tz=timezone.utc).isoformat().replace("+00:00", "Z") + + +def _job(name: str, dur_s: float, start_s: float = 0.0, conclusion: str = "success") -> dict: + """Build a normalized job dict with realistic timestamps for wall-time math.""" + return { + "name": name, + "duration_s": dur_s, + "conclusion": conclusion, + "started_at": _ts(start_s), + "completed_at": _ts(start_s + dur_s), + "wait_s": 0.0, + } + + +def _timings(jobs: list[dict]) -> dict: + return {"run_id": "123", "head_sha": "abc", "created_at": "", "jobs": jobs} + + +def _result(statuses: list[dict]) -> dict: + """Extract the single result dict from the nested format.""" + assert len(statuses) == 1 + assert statuses[0]["source"] == "ci timing" + results = statuses[0]["results"] + assert len(results) == 1 + return results[0] + + +def test_no_baseline_is_info(): + t = _timings([_job("tests", 60.0)]) + result = _result(_mod.generate_review_status(t, None)) + assert result["kind"] == "info" + assert "no baseline" in result["summary"].lower() + assert "link" not in result # no report_url → no link field + + +def test_no_regression_is_info(): + cur = _timings([_job("tests", 60.0)]) + bl = _timings([_job("tests", 60.0)]) + result = _result(_mod.generate_review_status(cur, bl)) + assert result["kind"] == "info" + assert "+0.0%" in result["summary"] + + +def test_small_regression_is_info(): + cur = _timings([_job("tests", 65.0)]) + bl = _timings([_job("tests", 60.0)]) + result = _result(_mod.generate_review_status(cur, bl)) + # +8.3% — well under the 25% warning threshold + assert result["kind"] == "info" + + +def test_large_regression_is_warning(): + cur = _timings([_job("tests", 80.0)]) + bl = _timings([_job("tests", 60.0)]) + result = _result(_mod.generate_review_status(cur, bl)) + # +33% — above the 25% threshold + assert result["kind"] == "warning" + assert "+33" in result["summary"] + + +def test_improvement_is_info(): + cur = _timings([_job("tests", 40.0)]) + bl = _timings([_job("tests", 60.0)]) + result = _result(_mod.generate_review_status(cur, bl)) + assert result["kind"] == "info" + assert "-33" in result["summary"] + + +def test_detail_shows_top_deltas(): + cur = _timings([_job("slow-job", 120.0), _job("fast-job", 30.0, start_s=120.0)]) + bl = _timings([_job("slow-job", 60.0), _job("fast-job", 60.0, start_s=60.0)]) + result = _result(_mod.generate_review_status(cur, bl)) + assert "slow-job" in result["detail"] + assert "fast-job" in result["detail"] + # Sorted by abs delta — slow-job (+60) before fast-job (-30) + assert result["detail"].index("slow-job") < result["detail"].index("fast-job") + + +def test_skipped_jobs_excluded_from_detail(): + cur = _timings([_job("skipped-job", 0.0, conclusion="skipped"), _job("tests", 60.0)]) + bl = _timings([_job("skipped-job", 0.0, conclusion="skipped"), _job("tests", 60.0)]) + result = _result(_mod.generate_review_status(cur, bl)) + assert "skipped-job" not in result["detail"] + + +def test_report_url_passed_through(): + t = _timings([_job("tests", 60.0)]) + result = _result(_mod.generate_review_status(t, None, report_url="https://artifact/123")) + assert result["link"] == "https://artifact/123" + assert result["link_label"] == "View report" + + +def test_never_error_severity(): + """Timings is observability — even huge regressions are warnings, not errors.""" + cur = _timings([_job("tests", 600.0)]) + bl = _timings([_job("tests", 60.0)]) + result = _result(_mod.generate_review_status(cur, bl)) + assert result["kind"] == "warning" + assert result["kind"] != "error" + + +def test_nested_format_structure(): + """The return value is a list with one {source, results: [...]} entry.""" + t = _timings([_job("tests", 60.0)]) + statuses = _mod.generate_review_status(t, None) + assert isinstance(statuses, list) + assert len(statuses) == 1 + assert statuses[0]["source"] == "ci timing" + assert isinstance(statuses[0]["results"], list) + assert len(statuses[0]["results"]) == 1 + r = statuses[0]["results"][0] + assert r["kind"] == "info" + assert r["title"] == "CI timings" + assert "summary" in r + assert "detail" in r