From 433673067e6454c5e3c01d6ac1ecfd638ca377a0 Mon Sep 17 00:00:00 2001 From: ethernet Date: Wed, 22 Jul 2026 19:19:53 -0400 Subject: [PATCH] ci: surface E2E screenshots in review comment (#69631) * ci: surface E2E screenshots in review comment * ci: mark completed review commits in past tense * ci: surface approved sensitive-file reviews * ci: link sensitive files to reviewed changes * ci: stage desktop E2E visual evidence Track screenshots newly introduced against main and package visual diffs for a trusted publisher. * fix(ci): pass E2E evidence output paths Supply the manifest and staging-directory arguments required by the screenshot status helper. * fix(ci): download the OSV SARIF artifact Match the artifact name and result filename emitted by the pinned upstream reusable workflow. --- .github/actions/detect-changes/action.yml | 3 + .github/workflows/ci.yml | 4 +- .github/workflows/e2e-desktop.yml | 69 +++++++--- .github/workflows/osv-scanner.yml | 5 +- .github/workflows/review-labels.yml | 13 +- scripts/ci/assemble_review_comment.py | 32 +++-- scripts/ci/classify_changes.py | 14 +- scripts/ci/e2e_screenshot_status.py | 155 ++++++++++++++++++++++ scripts/ci/emit_review_status.py | 70 ++++++++-- scripts/ci/live_comment.py | 10 +- scripts/ci/timings_report.py | 8 +- tests/ci/test_assemble_review_comment.py | 88 ++++++++---- tests/ci/test_classify_changes.py | 13 ++ tests/ci/test_e2e_screenshot_status.py | 57 ++++++++ tests/ci/test_emit_review_status.py | 60 +++++++++ tests/ci/test_live_comment.py | 12 ++ tests/ci/test_timings_report.py | 18 +-- 17 files changed, 545 insertions(+), 86 deletions(-) create mode 100644 scripts/ci/e2e_screenshot_status.py create mode 100644 tests/ci/test_e2e_screenshot_status.py create mode 100644 tests/ci/test_emit_review_status.py diff --git a/.github/actions/detect-changes/action.yml b/.github/actions/detect-changes/action.yml index 0794b5ed4f8e7..145d742d5b12b 100644 --- a/.github/actions/detect-changes/action.yml +++ b/.github/actions/detect-changes/action.yml @@ -39,6 +39,9 @@ outputs: ci_review: description: Require CI-sensitive file review label. value: ${{ steps.classify.outputs.ci_review }} + ci_review_files: + description: JSON list of CI-sensitive files changed by the pull request. + value: ${{ steps.classify.outputs.ci_review_files }} runs: using: composite diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6932fff39ccad..0aa540ed8beee 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -46,6 +46,7 @@ jobs: docker_meta: ${{ steps.classify.outputs.docker_meta }} mcp_catalog: ${{ steps.classify.outputs.mcp_catalog }} ci_review: ${{ steps.classify.outputs.ci_review }} + ci_review_files: ${{ steps.classify.outputs.ci_review_files }} event_name: ${{ github.event_name }} steps: - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 @@ -163,6 +164,7 @@ jobs: uses: ./.github/workflows/review-labels.yml with: ci_review: ${{ needs.detect.outputs.ci_review == 'true' }} + ci_review_files: ${{ needs.detect.outputs.ci_review_files }} mcp_catalog: ${{ needs.detect.outputs.mcp_catalog == 'true' }} supply_chain: ${{ needs.supply-chain.outputs.critical_findings == 'true' }} secrets: inherit @@ -186,7 +188,7 @@ jobs: # ───────────────────────────────────────────────────────────────────── comment-live: name: CI review comment (live) - needs: [detect, review-labels, lockfile-diff, supply-chain, osv-scanner, uv-lockfile, history-check, contributor-check] + needs: [detect, review-labels, lockfile-diff, supply-chain, osv-scanner, uv-lockfile, history-check, contributor-check, e2e-desktop] if: always() && github.event_name == 'pull_request' && github.event.pull_request.head.repo.fork != true runs-on: ubuntu-latest timeout-minutes: 40 diff --git a/.github/workflows/e2e-desktop.yml b/.github/workflows/e2e-desktop.yml index 364f23b73d0f7..55af27e24c2e3 100644 --- a/.github/workflows/e2e-desktop.yml +++ b/.github/workflows/e2e-desktop.yml @@ -2,6 +2,10 @@ name: E2E Desktop on: workflow_call: + outputs: + review_status: + description: Screenshot and visual-diff status for the CI review comment. + value: ${{ jobs.e2e.outputs.review_status }} permissions: contents: read @@ -15,6 +19,8 @@ jobs: name: Playwright E2E (Linux) runs-on: ubuntu-latest timeout-minutes: 20 + outputs: + review_status: ${{ steps.review-status.outputs.review_status }} steps: - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 @@ -146,6 +152,25 @@ jobs: overwrite: true if-no-files-found: ignore + - name: Build screenshot review status + id: review-status + if: always() + working-directory: apps/desktop + env: + RESULTS_URL: ${{ steps.upload-results.outputs.artifact-url }} + run: | + python3 ../../scripts/ci/e2e_screenshot_status.py \ + --results-dir test-results \ + --manifest-output /tmp/e2e-screenshot-manifest.json \ + --evidence-dir /tmp/e2e-evidence \ + --artifact-url "$RESULTS_URL" \ + --output /tmp/e2e-review-status.json + { + echo 'review_status<<__E2E_REVIEW_STATUS__' + cat /tmp/e2e-review-status.json + echo '__E2E_REVIEW_STATUS__' + } >> "$GITHUB_OUTPUT" + # ── Generate step summary with visual diff info ─────────────────── # Parse the JSON report + scan for diff images, then post a summary # to the GitHub Actions step output so reviewers can see what changed @@ -159,49 +184,50 @@ jobs: RESULTS_URL: ${{ steps.upload-results.outputs.artifact-url }} DIFFS_URL: ${{ steps.upload-diffs.outputs.artifact-url }} run: | - echo "## Desktop E2E — Visual Diff Report" >> "$GITHUB_STEP_SUMMARY" - echo "" >> "$GITHUB_STEP_SUMMARY" + { + echo "## Desktop E2E — Visual Diff Report" + echo "" # Count diff images (playwright writes *-diff.png on mismatch) DIFF_COUNT=$(find test-results -name '*-diff.png' 2>/dev/null | wc -l) ACTUAL_COUNT=$(find test-results -name '*-actual.png' 2>/dev/null | wc -l) if [ "$DIFF_COUNT" -eq 0 ]; then - echo "✅ All $ACTUAL_COUNT screenshot(s) matched their baselines (or no baselines existed yet)." >> "$GITHUB_STEP_SUMMARY" + echo "✅ All $ACTUAL_COUNT screenshot(s) matched their baselines (or no baselines existed yet)." else - echo "📸 **$DIFF_COUNT of $ACTUAL_COUNT screenshot(s) differ from baseline:**" >> "$GITHUB_STEP_SUMMARY" - echo "" >> "$GITHUB_STEP_SUMMARY" - echo "| Test | Diff | Actual | Expected |" >> "$GITHUB_STEP_SUMMARY" - echo "|------|------|--------|----------|" >> "$GITHUB_STEP_SUMMARY" + echo "📸 **$DIFF_COUNT of $ACTUAL_COUNT screenshot(s) differ from baseline:**" + echo "" + echo "| Test | Diff | Actual | Expected |" + echo "|------|------|--------|----------|" # List each diff image with a link to the artifact for diff in $(find test-results -name '*-diff.png' 2>/dev/null | sort); do - base=$(echo "$diff" | sed 's/-diff\.png$//') + base=${diff%-diff.png} test_name=$(basename "$base") - echo "| $test_name | [diff]($diff) | [actual](${base}-actual.png) | [expected](${base}-expected.png) |" >> "$GITHUB_STEP_SUMMARY" + echo "| $test_name | [diff]($diff) | [actual](${base}-actual.png) | [expected](${base}-expected.png) |" done fi - echo "" >> "$GITHUB_STEP_SUMMARY" - echo "📥 **Artifacts:**" >> "$GITHUB_STEP_SUMMARY" - echo "" >> "$GITHUB_STEP_SUMMARY" + echo "" + echo "📥 **Artifacts:**" + echo "" if [ -n "$RESULTS_URL" ]; then - echo "- [playwright-test-results]($RESULTS_URL) — all screenshots (actual + expected + diff) + traces" >> "$GITHUB_STEP_SUMMARY" + echo "- [playwright-test-results]($RESULTS_URL) — all screenshots (actual + expected + diff) + traces" fi if [ -n "$REPORT_URL" ]; then - echo "- [playwright-report]($REPORT_URL) — interactive HTML report" >> "$GITHUB_STEP_SUMMARY" + echo "- [playwright-report]($REPORT_URL) — interactive HTML report" fi if [ -n "$DIFFS_URL" ]; then - echo "- [visual-diffs]($DIFFS_URL) — just the diffed screenshots (small, fast to review)" >> "$GITHUB_STEP_SUMMARY" + echo "- [visual-diffs]($DIFFS_URL) — just the diffed screenshots (small, fast to review)" fi - echo "" >> "$GITHUB_STEP_SUMMARY" - echo "**To update baselines:** merge to main (baselines auto-update on main runs) or run \`npx playwright test --update-snapshots\` locally." >> "$GITHUB_STEP_SUMMARY" + echo "" + echo "**To update baselines:** merge to main (baselines auto-update on main runs) or run \`npx playwright test --update-snapshots\` locally." # Also parse the JSON report for pass/fail counts if [ -f playwright-report/results.json ]; then - echo "" >> "$GITHUB_STEP_SUMMARY" - echo "### Test Results" >> "$GITHUB_STEP_SUMMARY" - echo "" >> "$GITHUB_STEP_SUMMARY" + echo "" + echo "### Test Results" + echo "" node -e " const r = require('./playwright-report/results.json'); const stats = r.stats || {}; @@ -211,5 +237,6 @@ jobs: console.log('| ❌ Failed | ' + (stats.unexpected || 0) + ' |'); console.log('| ⏭️ Skipped | ' + (stats.skipped || 0) + ' |'); console.log('| 🔄 Flaky | ' + (stats.flaky || 0) + ' |'); - " >> "$GITHUB_STEP_SUMMARY" 2>/dev/null || true + " 2>/dev/null || true fi + } >> "$GITHUB_STEP_SUMMARY" diff --git a/.github/workflows/osv-scanner.yml b/.github/workflows/osv-scanner.yml index 37fed82fbbf97..455ede33dd562 100644 --- a/.github/workflows/osv-scanner.yml +++ b/.github/workflows/osv-scanner.yml @@ -48,6 +48,9 @@ jobs: --lockfile=uv.lock --lockfile=package-lock.json --lockfile=website/package-lock.json + # The upstream reusable workflow uploads this exact file under its + # fixed artifact name, which the wrapper downloads below. + results-file-name: osv-results.sarif fail-on-vuln: false emit-status: @@ -64,7 +67,7 @@ jobs: - name: Download SARIF result uses: actions/download-artifact@d3f86a106a0bac45b974a628896c90dbdf5c8093 # v4 with: - name: osv-results + name: OSV Scanner SARIF file path: /tmp/osv-results continue-on-error: true diff --git a/.github/workflows/review-labels.yml b/.github/workflows/review-labels.yml index 737348d1c559c..c8ea37dbbcaf0 100644 --- a/.github/workflows/review-labels.yml +++ b/.github/workflows/review-labels.yml @@ -23,6 +23,10 @@ on: description: Whether CI-sensitive files (eslint config, workflows, actions) changed. type: boolean default: false + ci_review_files: + description: JSON list of CI-sensitive files changed by the pull request. + type: string + default: '[]' mcp_catalog: description: Whether the MCP catalog / installer changed. type: boolean @@ -78,18 +82,25 @@ jobs: id: build-status env: CI_REVIEW: ${{ inputs.ci_review }} + CI_REVIEW_FILES: ${{ inputs.ci_review_files }} MCP_CATALOG: ${{ inputs.mcp_catalog }} SUPPLY_CHAIN: ${{ inputs.supply_chain }} LABEL_PRESENT: ${{ steps.label-check.outputs.ci_reviewed }} + REPO_URL: ${{ github.server_url }}/${{ github.repository }} + BASE_SHA: ${{ github.event.pull_request.base.sha }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} run: | set -euo pipefail args=() if [ "$CI_REVIEW" = "true" ]; then args+=(--ci-review); fi + args+=(--ci-review-files "$CI_REVIEW_FILES") if [ "$MCP_CATALOG" = "true" ]; then args+=(--mcp-catalog); fi if [ "$SUPPLY_CHAIN" = "true" ]; then args+=(--supply-chain); fi if [ "$LABEL_PRESENT" = "true" ]; then args+=(--label-present); fi - python3 scripts/ci/emit_review_status.py "${args[@]}" --output "$GITHUB_OUTPUT" + python3 scripts/ci/emit_review_status.py "${args[@]}" \ + --repo-url "$REPO_URL" --base-sha "$BASE_SHA" --head-sha "$HEAD_SHA" \ + --output "$GITHUB_OUTPUT" - name: Fail on missing label if: steps.label-check.outputs.ci_reviewed != 'true' diff --git a/scripts/ci/assemble_review_comment.py b/scripts/ci/assemble_review_comment.py index 0f701f13e3382..76f3488012add 100644 --- a/scripts/ci/assemble_review_comment.py +++ b/scripts/ci/assemble_review_comment.py @@ -26,7 +26,7 @@ result objects:: Each result object has: - kind: "error" | "action_required" | "warning" | "info" + kind: "error" | "action_required" | "warning" | "info" | "debug" title: section heading summary: one-line description detail: markdown detail (optional) @@ -56,7 +56,7 @@ from pathlib import Path MARKER = "" # Severity ordering for display. -_SEVERITY_ORDER = ["error", "action_required", "warning", "info"] +_SEVERITY_ORDER = ["error", "action_required", "warning", "info", "debug"] # Severities that trigger the "blocking issues" layout (vs. the # "looks good!" banner). @@ -74,7 +74,7 @@ _SEVERITY_GROUP_HEADER = { class ReviewItem: """A single piece of review information with a severity tag.""" - severity: str # "error" | "action_required" | "warning" | "info" + severity: str # "error" | "action_required" | "warning" | "info" | "debug" title: str # short section title, e.g. "package-lock.json" summary: str # one-line summary detail: str = "" # optional markdown detail (tables, bullet lists, etc.) @@ -241,15 +241,15 @@ def _render_group(header: str, items: list[ReviewItem]) -> str: 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.""" +def _render_debug_details(items: list[ReviewItem]) -> str: + """Render each debug 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) + return "### debug info\n\n" + "\n\n".join(blocks) def _render_pending_items(pending_jobs: list[str]) -> str: @@ -263,13 +263,13 @@ def render_comment(items: list[ReviewItem], pending_jobs: list[str] | None = Non 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 + Warnings are shown only when present. Info items are visible; debug 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. + When there are no errors, action_required, or warnings, an "all good!" + banner is shown at the top. Info items remain visible and debug items + follow in collapsible ``
`` blocks. """ pending = pending_jobs or [] @@ -279,6 +279,7 @@ def render_comment(items: list[ReviewItem], pending_jobs: list[str] | None = Non by_severity.setdefault(item.severity, []).append(item) info = by_severity.get("info", []) + debug = by_severity.get("debug", []) has_blocking = any(by_severity.get(s) for s in _BLOCKING_SEVERITIES) body = f"{MARKER}\n# ૮ >ﻌ< ა ci review\n\n" @@ -287,7 +288,7 @@ def render_comment(items: list[ReviewItem], pending_jobs: list[str] | None = Non body += f"{commit_info}\n\n" if not items and not pending: - return f"{body}looks good to me!" + return f"{body}all good!" sections: list[str] = [] @@ -296,9 +297,12 @@ def render_comment(items: list[ReviewItem], pending_jobs: list[str] | None = Non if group: sections.append(_render_group(_SEVERITY_GROUP_HEADER[sev], group)) - # Info: collapsible
if info: - sections.append(_render_info_details(info)) + sections.append(_render_group("## ℹ️ Info", info)) + + # Debug: collapsible
+ if debug: + sections.append(_render_debug_details(debug)) if pending: body += _render_pending_items(pending) diff --git a/scripts/ci/classify_changes.py b/scripts/ci/classify_changes.py index 64380aa5d3944..fd0b870281f04 100644 --- a/scripts/ci/classify_changes.py +++ b/scripts/ci/classify_changes.py @@ -32,6 +32,7 @@ must never skip one a change could break: from __future__ import annotations +import json import os import sys @@ -89,6 +90,11 @@ def _is_ci_review(p: str) -> bool: return os.path.basename(p).startswith("eslint.config.") +def ci_review_files(files: list[str]) -> list[str]: + """Return the CI-sensitive paths that need maintainer review.""" + return sorted({f.strip() for f in files if f.strip() and _is_ci_review(f.strip())}) + + def classify(files: list[str]) -> dict[str, bool]: """Map changed paths to ``{lane: should_run}``.""" files = [f.strip() for f in files if f.strip()] @@ -119,8 +125,12 @@ def classify(files: list[str]) -> dict[str, bool]: def main() -> int: - lanes = classify(sys.stdin.read().splitlines()) - out = "\n".join(f"{k}={str(v).lower()}" for k, v in lanes.items()) + files = sys.stdin.read().splitlines() + lanes = classify(files) + out = "\n".join([ + *(f"{key}={str(value).lower()}" for key, value in lanes.items()), + f"ci_review_files={json.dumps(ci_review_files(files))}", + ]) if dest := os.environ.get("GITHUB_OUTPUT"): with open(dest, "a", encoding="utf-8") as fh: fh.write(out + "\n") diff --git a/scripts/ci/e2e_screenshot_status.py b/scripts/ci/e2e_screenshot_status.py new file mode 100644 index 0000000000000..e40b415768321 --- /dev/null +++ b/scripts/ci/e2e_screenshot_status.py @@ -0,0 +1,155 @@ +#!/usr/bin/env python3 +"""Select Desktop E2E visual evidence and build its CI review status.""" + +from __future__ import annotations + +import argparse +import hashlib +import json +import shutil +from pathlib import Path + +SOURCE = "playwright e2e" +EVIDENCE_START = "" +EVIDENCE_END = "" + + +def _files(root: Path, pattern: str) -> list[Path]: + return sorted(path for path in root.rglob(pattern) if path.is_file()) if root.exists() else [] + + +def _is_explicit_screenshot(path: Path) -> bool: + """Exclude Playwright's automatic and visual-comparator PNG outputs.""" + return not ( + path.name.startswith(("test-finished-", "test-failed-")) + or path.name.endswith(("-actual.png", "-expected.png", "-diff.png")) + ) + + +def build_manifest(results_dir: Path) -> dict: + """Record stable screenshot names from one E2E run for main/PR comparison.""" + screenshots = [path for path in _files(results_dir, "*.png") if _is_explicit_screenshot(path)] + return {"version": 1, "screenshot_names": sorted({path.name for path in screenshots})} + + +def _base_screenshot_names(path: Path | None) -> set[str] | None: + """Return ``None`` when main evidence is unavailable (never guess newness).""" + if path is None or not path.is_file(): + return None + try: + data = json.loads(path.read_text(encoding="utf-8")) + except json.JSONDecodeError: + return None + names = data.get("screenshot_names", []) if isinstance(data, dict) else [] + if not isinstance(data, dict) or not isinstance(names, list): + return None + return {name for name in names if isinstance(name, str)} + + +def _stage_name(kind: str, path: Path, results_dir: Path) -> str: + relative = path.relative_to(results_dir).as_posix() + digest = hashlib.sha256(relative.encode("utf-8")).hexdigest()[:12] + return f"{kind}-{digest}-{path.name}" + + +def select_evidence(results_dir: Path, base_manifest: Path | None = None) -> dict: + """Select only screenshots new to main, plus every generated visual diff.""" + base_names = _base_screenshot_names(base_manifest) + screenshots = [] if base_names is None else [ + path for path in _files(results_dir, "*.png") + if _is_explicit_screenshot(path) and path.name not in base_names + ] + diffs: list[dict[str, Path]] = [] + for diff in _files(results_dir, "*-diff.png"): + stem = diff.with_name(diff.name.removesuffix("-diff.png")) + entry = {"diff": diff} + for kind in ("actual", "expected"): + candidate = stem.with_name(f"{stem.name}-{kind}.png") + if candidate.is_file(): + entry[kind] = candidate + diffs.append(entry) + return {"screenshots": screenshots, "diffs": diffs} + + +def stage_evidence(results_dir: Path, evidence_dir: Path, selection: dict) -> dict: + """Copy selected PNGs into a flat, path-safe evidence artifact.""" + evidence_dir.mkdir(parents=True, exist_ok=True) + staged: dict[Path, str] = {} + + def stage(kind: str, path: Path) -> str: + if path in staged: + return staged[path] + name = _stage_name(kind, path, results_dir) + shutil.copyfile(path, evidence_dir / name) + staged[path] = name + return name + + manifest = {"version": 1, "screenshots": [], "diffs": []} + for screenshot in selection["screenshots"]: + manifest["screenshots"].append({ + "name": screenshot.name, + "file": stage("screenshot", screenshot), + }) + for diff in selection["diffs"]: + entry = {"name": diff["diff"].name.removesuffix("-diff.png"), "diff": stage("diff", diff["diff"])} + for kind in ("actual", "expected"): + if kind in diff: + entry[kind] = stage(kind, diff[kind]) + manifest["diffs"].append(entry) + + (evidence_dir / "e2e-evidence.json").write_text( + json.dumps(manifest, sort_keys=True) + "\n", encoding="utf-8" + ) + return manifest + + +def build_status(selection: dict, artifact_url: str = "") -> list[dict]: + """Return the review status. The trusted publisher replaces its marker.""" + screenshots = selection["screenshots"] + diffs = selection["diffs"] + if not screenshots and not diffs: + return [] + + summary_parts = [] + if screenshots: + summary_parts.append( + f"{len(screenshots)} new screenshot{'s' if len(screenshots) != 1 else ''} vs main" + ) + if diffs: + summary_parts.append(f"{len(diffs)} visual diff{'s' if len(diffs) != 1 else ''}") + + result: dict[str, str] = { + "kind": "info", + "title": "Desktop E2E visual evidence", + "summary": "; ".join(summary_parts) + ".", + "detail": "\n".join((EVIDENCE_START, "inline evidence is publishing...", EVIDENCE_END)), + } + if artifact_url: + result["link"] = artifact_url + result["link_label"] = "View test artifacts" + return [{"source": SOURCE, "results": [result]}] + + +def main() -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--results-dir", type=Path, required=True) + parser.add_argument("--base-manifest", type=Path) + parser.add_argument("--manifest-output", type=Path, required=True) + parser.add_argument("--evidence-dir", type=Path, required=True) + parser.add_argument("--artifact-url", default="") + parser.add_argument("--output", type=Path, required=True) + args = parser.parse_args() + + args.manifest_output.write_text( + json.dumps(build_manifest(args.results_dir), sort_keys=True) + "\n", encoding="utf-8" + ) + selection = select_evidence(args.results_dir, args.base_manifest) + stage_evidence(args.results_dir, args.evidence_dir, selection) + args.output.write_text( + json.dumps(build_status(selection, args.artifact_url)) + "\n", encoding="utf-8" + ) + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/scripts/ci/emit_review_status.py b/scripts/ci/emit_review_status.py index f8c02938d8650..726d1f6b8651d 100644 --- a/scripts/ci/emit_review_status.py +++ b/scripts/ci/emit_review_status.py @@ -27,8 +27,10 @@ with the verification checklist. from __future__ import annotations import argparse +import hashlib import json import sys +from urllib.parse import quote # The source identifier used for error-synthesis exclusion. This must # match (as a normalized substring) the job name as it appears in the @@ -41,24 +43,57 @@ import sys SOURCE = "review-label-gate" +def _ci_review_detail( + files_json: str, repo_url: str, base_sha: str, head_sha: str, +) -> str: + """Render links to the changed CI-sensitive files that triggered review.""" + try: + files = json.loads(files_json) + except (json.JSONDecodeError, TypeError): + return "" + if not isinstance(files, list) or not repo_url or not base_sha or not head_sha: + return "" + + links = [] + for path in files: + if not isinstance(path, str) or not path: + continue + label = path.replace("\\", "\\\\").replace("[", "\\[").replace("]", "\\]") + path_hash = hashlib.sha256(path.encode()).hexdigest() + url = ( + f"{repo_url}/compare/{quote(base_sha, safe='')}...{quote(head_sha, safe='')}" + f"#diff-{path_hash}" + ) + links.append(f"- [`{label}`]({url})") + return "**Sensitive files changed:**\n" + "\n".join(links) if links else "" + + def build_results( ci_review: bool, mcp_catalog: bool, supply_chain: bool, label_present: bool, + ci_review_files: str = "[]", + repo_url: str = "", + base_sha: str = "", + head_sha: str = "", ) -> list[dict]: """Build the list of result objects for this source.""" results: list[dict] = [] if ci_review: + detail = _ci_review_detail(ci_review_files, repo_url, base_sha, head_sha) if label_present: - results.append({ + result = { "kind": "info", "title": "CI-sensitive file review", - "summary": "`ci-reviewed` label is present.", - }) + "summary": ( + "PR touches sensitive files, but the `ci-reviewed` label has been " + "added, approving them." + ), + } else: - results.append({ + result = { "kind": "action_required", "title": "CI-sensitive file review", "summary": ( @@ -72,12 +107,15 @@ def build_results( "- no workflow changes that widen permissions or remove guards,\n" "- no composite action changes that alter what gets executed." ), - }) + } + if detail: + result["detail"] = detail + results.append(result) if mcp_catalog: if label_present: results.append({ - "kind": "info", + "kind": "debug", "title": "MCP catalog security review", "summary": "`ci-reviewed` label is present.", }) @@ -119,9 +157,16 @@ def build_statuses( mcp_catalog: bool, supply_chain: bool, label_present: bool, + ci_review_files: str = "[]", + repo_url: str = "", + base_sha: str = "", + head_sha: str = "", ) -> list[dict]: """Build the full review_status array (one entry with a results list).""" - results = build_results(ci_review, mcp_catalog, supply_chain, label_present) + results = build_results( + ci_review, mcp_catalog, supply_chain, label_present, + ci_review_files, repo_url, base_sha, head_sha, + ) if not results: return [] return [{"source": SOURCE, "results": results}] @@ -131,18 +176,27 @@ 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("--ci-review-files", default="[]", + help="JSON list of CI-sensitive files changed.") parser.add_argument("--mcp-catalog", action="store_true", help="Whether the MCP catalog / installer changed.") parser.add_argument("--supply-chain", action="store_true", help="Whether the critical supply-chain scanner found a risk.") parser.add_argument("--label-present", action="store_true", help="Whether the ci-reviewed label is present.") + parser.add_argument("--repo-url", default="", + help="Repository URL used for changed-file links.") + parser.add_argument("--base-sha", default="", + help="Pull request base SHA used for changed-file links.") + parser.add_argument("--head-sha", default="", + help="Pull request head SHA used for changed-file links.") 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.supply_chain, args.label_present + args.ci_review, args.mcp_catalog, args.supply_chain, args.label_present, + args.ci_review_files, args.repo_url, args.base_sha, args.head_sha, ) json_str = json.dumps(statuses) diff --git a/scripts/ci/live_comment.py b/scripts/ci/live_comment.py index e4ca5c9e2de55..b015c1ba08db4 100644 --- a/scripts/ci/live_comment.py +++ b/scripts/ci/live_comment.py @@ -352,6 +352,13 @@ def _merge_statuses( return json.dumps(merged) if merged else "" +def _commit_info_for_state(commit_info: str, pending: list[str]) -> str: + """Use past tense in the final comment after every CI job completes.""" + if pending: + return commit_info + return commit_info.replace("running on ", "ran on ", 1) + + # --------------------------------------------------------------------------- # Polling loop # --------------------------------------------------------------------------- @@ -410,11 +417,12 @@ def run( print(f" Found ci-timings artifact with {len(artifact_statuses)} status entries") merged_json = _merge_statuses(base_statuses, artifact_statuses) + current_commit_info = _commit_info_for_state(commit_info, pending) body = build_comment_body( asm, completed, pending, run_url, job_urls, merged_json, - commit_info, + current_commit_info, ) if body != last_body: diff --git a/scripts/ci/timings_report.py b/scripts/ci/timings_report.py index f28bd9d703f87..0fba990b3fda0 100644 --- a/scripts/ci/timings_report.py +++ b/scripts/ci/timings_report.py @@ -909,7 +909,7 @@ def generate_review_status( """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 — + result kind is ``"debug"`` 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. @@ -917,7 +917,7 @@ def generate_review_status( stats = compute_stats(timings, baseline) if baseline is None: - severity = "info" + severity = "debug" summary = f"Wall time {fmt_dur(stats['wall'])} (no baseline yet)." else: wall = stats["wall"] @@ -928,10 +928,10 @@ def generate_review_status( if pct > _TIMINGS_WARN_PCT * 100: severity = "warning" else: - severity = "info" + severity = "debug" else: wall_str = f"Wall time {fmt_dur(wall)}." - severity = "info" + severity = "debug" if stats["slower"]: wall_str += f" {stats['slower']} job(s) slower," diff --git a/tests/ci/test_assemble_review_comment.py b/tests/ci/test_assemble_review_comment.py index 373fb6a3a47fd..8c51a6043ad11 100644 --- a/tests/ci/test_assemble_review_comment.py +++ b/tests/ci/test_assemble_review_comment.py @@ -1,7 +1,7 @@ """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 +classified by severity (error / action_required / warning / info / debug), then renders them into a single PR comment body. Status data comes from two sources: @@ -16,7 +16,7 @@ Layout rules tested here: - each item is a ### section under its group header - errors + action_required always visible - warnings shown only when present - - info in a collapsible
block + - info above the fold; debug in a collapsible
block - sections separated by --- - how_to_fix rendered at bottom of action_required items - empty → clean banner @@ -93,6 +93,16 @@ def test_statuses_info(): assert sources == {"review-label-gate"} +def test_statuses_debug(): + statuses = _status("ci-timings", [{ + "kind": "debug", + "title": "CI timings", + "summary": "No regression.", + }]) + items, _ = _mod.collect_from_statuses(statuses) + assert items[0].severity == "debug" + + def test_statuses_multiple_results_same_source(): """One source can emit multiple results of different kinds.""" statuses = _status("review-label-gate", [ @@ -238,24 +248,24 @@ def test_failed_jobs_fallback_to_run_url(): def test_render_empty_shows_clean_banner(): - """Completely clean — dog kaomoji + 'looks good' banner, no sections.""" + """Completely clean — dog kaomoji + 'all good' banner, no sections.""" body = _mod.render_comment([]) assert body.startswith(MARKER) assert "૮ >ﻌ< ა" in body - assert "looks good to me!" in body + assert "all good!" 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.""" +def test_render_info_only_is_visible_above_the_fold(): + """Info items are visible rather than hidden in debug details.""" 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 "## ℹ️ Info" in body + assert "
" not in body assert "No changes." in body assert "OK." in body # No blocking sections @@ -263,12 +273,12 @@ def test_render_info_only_shows_details(): assert "## ⚠️" not in body -def test_render_info_only_with_pending_shows_details_plus_footer(): +def test_render_info_only_with_pending_shows_info_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 "## ℹ️ Info" in body assert "Still running" in body assert "`ci-timings`" in body @@ -348,17 +358,19 @@ def test_render_errors_always_visible(): assert "### tests" in body assert "Job **tests** failed." in body assert "[View job](https://run)" in body - assert "
" in body + assert "## ℹ️ Info" in body assert "No changes." in body -def test_render_info_in_collapsible_details(): - """Each info item is its own
block.""" +def test_render_debug_in_collapsible_details(): + """Debug items have a small label and each has its own
block.""" items = [ - ReviewItem(severity="info", title="lockfile", summary="No changes."), - ReviewItem(severity="info", title="timings", summary="OK."), + ReviewItem(severity="debug", title="lockfile", summary="No changes."), + ReviewItem(severity="debug", title="timings", summary="OK."), ] body = _mod.render_comment(items) + assert "### debug info" in body + assert body.index("### debug info") < body.index("
") assert body.count("
") == 2 assert body.count("
") == 2 assert "lockfile" in body @@ -367,9 +379,10 @@ def test_render_info_in_collapsible_details(): assert "OK." in body -def test_render_order_errors_then_action_then_warn_then_info(): +def test_render_order_errors_then_action_then_warn_then_info_then_debug(): items = [ ReviewItem(severity="info", title="i", summary="info"), + ReviewItem(severity="debug", title="d", summary="debug"), ReviewItem(severity="warning", title="w", summary="warn"), ReviewItem(severity="action_required", title="a", summary="action"), ReviewItem(severity="error", title="e", summary="error"), @@ -378,8 +391,9 @@ def test_render_order_errors_then_action_then_warn_then_info(): 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 + info_pos = body.index("## ℹ️ Info") + debug_pos = body.index("
") + assert error_pos < action_pos < warn_pos < info_pos < debug_pos # ─── render_comment (pending jobs) ──────────────────────────────────── @@ -422,7 +436,7 @@ 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 "all good!" in body assert "##" not in body @@ -457,6 +471,32 @@ def test_assemble_with_review_statuses(): assert "## ❌ Job failures" not in body +def test_assemble_review_status_detail_renders_sensitive_file_links(): + statuses = _status("review-label-gate", [{ + "kind": "action_required", + "title": "CI-sensitive file review", + "summary": "Changes detected.", + "detail": "**Sensitive files:**\n- [`ci.yml`](https://example.test/ci.yml)", + }]) + body = _mod.assemble(review_statuses_json=statuses) + assert "**Sensitive files:**" in body + assert "[`ci.yml`](https://example.test/ci.yml)" in body + + +def test_assemble_info_keeps_screenshot_details_visible_below_its_summary(): + statuses = _status("playwright e2e", [{ + "kind": "info", + "title": "Desktop E2E screenshots", + "summary": "1 screenshot captured; 0 visual diffs.", + "detail": "
\n1 captured screenshot\n\n- [`proof.png`](https://example.test/artifact)\n\n
", + }]) + body = _mod.assemble(review_statuses_json=statuses) + assert "## ℹ️ Info" in body + assert "1 screenshot captured; 0 visual diffs." in body + assert "1 captured screenshot" in body + assert "[`proof.png`](https://example.test/artifact)" in body + + def test_assemble_pending_jobs(): body = _mod.assemble(pending_jobs=["ci-timings"]) assert "Still running" in body @@ -473,9 +513,9 @@ def test_assemble_with_items_and_pending(): def test_assemble_with_timings_status(): - """Timings status from the nested format renders as info or warning.""" + """Timings status from the nested format renders as debug or warning.""" statuses = _status("ci-timings", [{ - "kind": "info", + "kind": "debug", "title": "CI timings", "summary": "Wall time 3m (no baseline yet).", "detail": "", @@ -490,14 +530,14 @@ def test_assemble_with_timings_status(): def test_assemble_with_lockfile_status(): - """Lockfile no-changes status renders as info in the details block.""" + """Lockfile no-changes status renders as visible info.""" 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 "## ℹ️ Info" in body assert "### package-lock.json" in body assert "No lockfile changes" in body @@ -602,7 +642,7 @@ 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 + assert "all good!" in body def test_render_both_emitted_link_and_job_url(): diff --git a/tests/ci/test_classify_changes.py b/tests/ci/test_classify_changes.py index 14bf342d87eda..c9b0818fb4843 100644 --- a/tests/ci/test_classify_changes.py +++ b/tests/ci/test_classify_changes.py @@ -19,6 +19,7 @@ if _spec is None or _spec.loader is None: _mod = importlib.util.module_from_spec(_spec) _spec.loader.exec_module(_mod) classify = _mod.classify +ci_review_files = _mod.ci_review_files DEFAULT = { "python": True, @@ -130,3 +131,15 @@ CASES = { @pytest.mark.parametrize("files,expected", CASES.values(), ids=CASES.keys()) def test_classify(files, expected): assert classify(files) == expected + + +def test_ci_review_files_returns_only_sensitive_paths_sorted_and_unique(): + assert ci_review_files([ + "apps/desktop/src/app.tsx", + ".github/workflows/ci.yml", + "apps/desktop/eslint.config.mjs", + ".github/workflows/ci.yml", + ]) == [ + ".github/workflows/ci.yml", + "apps/desktop/eslint.config.mjs", + ] diff --git a/tests/ci/test_e2e_screenshot_status.py b/tests/ci/test_e2e_screenshot_status.py new file mode 100644 index 0000000000000..c864a766938c4 --- /dev/null +++ b/tests/ci/test_e2e_screenshot_status.py @@ -0,0 +1,57 @@ +"""Tests for scripts/ci/e2e_screenshot_status.py.""" + +from __future__ import annotations + +import importlib.util +import sys +from pathlib import Path + +_PATH = Path(__file__).resolve().parents[2] / "scripts" / "ci" / "e2e_screenshot_status.py" +_spec = importlib.util.spec_from_file_location("e2e_screenshot_status", _PATH) +if _spec is None or _spec.loader is None: + raise ImportError("Failed to load e2e_screenshot_status.py") +_mod = importlib.util.module_from_spec(_spec) +_spec.loader.exec_module(_mod) + + +def test_status_selects_only_new_explicit_screenshots_and_all_diffs(tmp_path): + for name in ( + "explicit-proof.png", + "test-finished-1.png", + "visual-actual.png", + "visual-expected.png", + "visual-diff.png", + ): + (tmp_path / name).write_bytes(b"png") + + base_manifest = tmp_path / "main-manifest.json" + base_manifest.write_text('{"screenshot_names":["already-on-main.png"]}', encoding="utf-8") + (tmp_path / "already-on-main.png").write_bytes(b"png") + + selection = _mod.select_evidence(tmp_path, base_manifest) + status = _mod.build_status(selection, "https://github.test/artifacts/1") + + result = status[0]["results"][0] + assert result["kind"] == "info" + assert result["summary"] == "1 new screenshot vs main; 1 visual diff." + assert _mod.EVIDENCE_START in result["detail"] + assert "already-on-main.png" not in result["detail"] + assert result["link"] == "https://github.test/artifacts/1" + + +def test_cli_output_ends_with_newline_for_github_output_delimiter(tmp_path, monkeypatch): + output = tmp_path / "review-status.json" + manifest = tmp_path / "main-manifest.json" + evidence_dir = tmp_path / "evidence" + monkeypatch.setattr(sys, "argv", [ + "e2e_screenshot_status.py", + "--results-dir", str(tmp_path), + "--manifest-output", str(manifest), + "--evidence-dir", str(evidence_dir), + "--output", str(output), + ]) + + assert _mod.main() == 0 + assert output.read_text(encoding="utf-8") == "[]\n" + assert manifest.read_text(encoding="utf-8") == '{"screenshot_names": [], "version": 1}\n' + assert evidence_dir.joinpath("e2e-evidence.json").is_file() \ No newline at end of file diff --git a/tests/ci/test_emit_review_status.py b/tests/ci/test_emit_review_status.py new file mode 100644 index 0000000000000..b1587a5d13c18 --- /dev/null +++ b/tests/ci/test_emit_review_status.py @@ -0,0 +1,60 @@ +"""Tests for scripts/ci/emit_review_status.py.""" + +from __future__ import annotations + +import importlib.util +import sys +from pathlib import Path + +_PATH = Path(__file__).resolve().parents[2] / "scripts" / "ci" / "emit_review_status.py" +_spec = importlib.util.spec_from_file_location("emit_review_status", _PATH) +if _spec is None or _spec.loader is None: + raise ImportError("Failed to load emit_review_status.py") +_mod = importlib.util.module_from_spec(_spec) +sys.modules["emit_review_status"] = _mod +_spec.loader.exec_module(_mod) + + +def test_ci_review_status_links_to_each_sensitive_file_change(): + results = _mod.build_results( + ci_review=True, + mcp_catalog=False, + supply_chain=False, + label_present=False, + ci_review_files='[".github/workflows/ci.yml", "apps/desktop/eslint.config.mjs"]', + repo_url="https://github.com/nousresearch/hermes-agent", + base_sha="base456", + head_sha="abc123", + ) + + assert results[0]["detail"] == ( + "**Sensitive files changed:**\n" + "- [`.github/workflows/ci.yml`](https://github.com/nousresearch/hermes-agent/compare/base456...abc123#diff-b803fcb7f17ed9235f1e5cb1fcd2f5d3b2838429d4368ae4c57ce4436577f03f)\n" + "- [`apps/desktop/eslint.config.mjs`](https://github.com/nousresearch/hermes-agent/compare/base456...abc123#diff-a45471520795db6e46840d1ba2a82c1f8a2841039bd60fb50624488c5f192438)" + ) + + +def test_approved_ci_review_is_visible_info(): + results = _mod.build_results( + ci_review=True, + mcp_catalog=False, + supply_chain=False, + label_present=True, + ci_review_files='[".github/workflows/ci.yml"]', + repo_url="https://github.com/nousresearch/hermes-agent", + base_sha="base456", + head_sha="abc123", + ) + + assert results == [{ + "kind": "info", + "title": "CI-sensitive file review", + "summary": ( + "PR touches sensitive files, but the `ci-reviewed` label has been " + "added, approving them." + ), + "detail": ( + "**Sensitive files changed:**\n" + "- [`.github/workflows/ci.yml`](https://github.com/nousresearch/hermes-agent/compare/base456...abc123#diff-b803fcb7f17ed9235f1e5cb1fcd2f5d3b2838429d4368ae4c57ce4436577f03f)" + ), + }] diff --git a/tests/ci/test_live_comment.py b/tests/ci/test_live_comment.py index ffc9529213100..c4d48027058c2 100644 --- a/tests/ci/test_live_comment.py +++ b/tests/ci/test_live_comment.py @@ -160,3 +160,15 @@ def test_classify_unknown_status_skipped(): completed, pending, job_urls = _mod.classify_jobs(jobs) assert completed == {} assert pending == [] + + +def test_commit_info_uses_present_tense_while_jobs_are_pending(): + info = "running on [abc1234](https://commit-url) — fix: thing" + assert _mod._commit_info_for_state(info, ["Python tests"]) == info + + +def test_commit_info_uses_past_tense_after_jobs_complete(): + info = "running on [abc1234](https://commit-url) — fix: thing" + assert _mod._commit_info_for_state(info, []) == ( + "ran on [abc1234](https://commit-url) — fix: thing" + ) diff --git a/tests/ci/test_timings_report.py b/tests/ci/test_timings_report.py index dee48c09f3097..b3e7585b37aa8 100644 --- a/tests/ci/test_timings_report.py +++ b/tests/ci/test_timings_report.py @@ -53,28 +53,28 @@ def _result(statuses: list[dict]) -> dict: return results[0] -def test_no_baseline_is_info(): +def test_no_baseline_is_debug(): t = _timings([_job("tests", 60.0)]) result = _result(_mod.generate_review_status(t, None)) - assert result["kind"] == "info" + assert result["kind"] == "debug" assert "no baseline" in result["summary"].lower() assert "link" not in result # no report_url → no link field -def test_no_regression_is_info(): +def test_no_regression_is_debug(): 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 result["kind"] == "debug" assert "+0.0%" in result["summary"] -def test_small_regression_is_info(): +def test_small_regression_is_debug(): 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" + assert result["kind"] == "debug" def test_large_regression_is_warning(): @@ -86,11 +86,11 @@ def test_large_regression_is_warning(): assert "+33" in result["summary"] -def test_improvement_is_info(): +def test_improvement_is_debug(): 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 result["kind"] == "debug" assert "-33" in result["summary"] @@ -137,7 +137,7 @@ def test_nested_format_structure(): assert isinstance(statuses[0]["results"], list) assert len(statuses[0]["results"]) == 1 r = statuses[0]["results"][0] - assert r["kind"] == "info" + assert r["kind"] == "debug" assert r["title"] == "CI timings" assert "summary" in r assert "detail" in r