diff --git a/scripts/ci/live_comment.py b/scripts/ci/live_comment.py index 6ef11818597b..a82f938af266 100644 --- a/scripts/ci/live_comment.py +++ b/scripts/ci/live_comment.py @@ -20,12 +20,12 @@ Architecture: - :func:`find_comment_id` / :func:`upsert_comment` — thin API wrappers. - - :func:`fetch_all_review_statuses` — enumerates all - ``review-status-*`` artifacts across the orchestrator run and all - sub-workflow runs, downloads each, parses the ``review_status=`` - line from ``review-status.json``, and merges into one array. - Recomputed from source every poll cycle, so statuses appear as - soon as each job uploads its artifact. + - :func:`fetch_all_review_statuses` — lists all ``review-status-*`` + artifacts on the orchestrator run (GitHub attaches reusable-workflow + artifacts to the caller run), downloads each, parses the + ``review_status=`` line from ``review-status.json``, and merges into + one array. Recomputed from source every poll cycle, so statuses + appear as soon as each job uploads its artifact. - :func:`run` — the polling loop. Calls the API, classifies, fetches artifacts, assembles, upserts, sleeps, repeats. Before @@ -280,32 +280,6 @@ def upsert_comment( _REVIEW_STATUS_ARTIFACT_PREFIX = "review-status-" -def _list_run_ids(token: str, repo: str, run_id: str) -> list[str]: - """Return the orchestrator run ID + all sub-workflow run IDs. - - Sub-workflow runs (workflow_call) may not exist yet on the first few - polls — that's fine, they just won't be in the list. - """ - 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", "") - - run_ids = [run_id] - - sub_runs = _api_get_paginated( - f"{API_BASE}/repos/{owner}/{repo_name}/actions/runs" - f"?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] - run_ids.extend(str(r["id"]) for r in sub_runs) - - return run_ids - - def _list_artifacts(token: str, repo: str, run_id: str) -> list[dict]: """List artifacts for a given run (paginated).""" owner, repo_name = repo.split("/") @@ -315,6 +289,13 @@ def _list_artifacts(token: str, repo: str, run_id: str) -> list[dict]: ) +class _NoRedirectHandler(urllib.request.HTTPRedirectHandler): + """Redirect handler that never follows — used to capture the Location.""" + + def redirect_request(self, *args, **kwargs): + return None + + def _download_artifact( token: str, repo: str, artifact: dict, dest_dir: Path, ) -> Path | None: @@ -328,17 +309,33 @@ def _download_artifact( if not archive_download_url: return None - # The archive_download_url is an API URL that redirects to a S3 URL. - # Build the request with our auth headers so the redirect works. - req = urllib.request.Request(archive_download_url, headers={ - "Authorization": f"Bearer {token}", - "Accept": "application/vnd.github+json", - "X-GitHub-Api-Version": "2022-11-28", - "User-Agent": "ci-live-comment", - }) + # The archive_download_url is an API URL that 302s to a signed blob + # URL. Hop 1 authenticates to the API; hop 2 follows the redirect + # WITHOUT the Authorization header — the blob rejects a request that + # carries both a SAS token and an Authorization header (401). + opener = urllib.request.build_opener(_NoRedirectHandler) + location = "" + try: + opener.open(urllib.request.Request(archive_download_url, headers={ + "Authorization": f"Bearer {token}", + "Accept": "application/vnd.github+json", + "X-GitHub-Api-Version": "2022-11-28", + "User-Agent": "ci-live-comment", + }), timeout=30) + except urllib.error.HTTPError as e: + location = e.headers.get("Location", "") if e.code == 302 else "" + except Exception: + location = "" + if not location: + return None + zip_path = dest_dir / f"{artifact['name']}.zip" try: - with urllib.request.urlopen(req) as resp: + # No auth headers here; further redirects are safe to follow. + with urllib.request.urlopen( + urllib.request.Request(location, headers={"User-Agent": "ci-live-comment"}), + timeout=60, + ) as resp: zip_path.write_bytes(resp.read()) except Exception: return None @@ -347,6 +344,8 @@ def _download_artifact( extract_dir.mkdir(parents=True, exist_ok=True) try: with zipfile.ZipFile(zip_path) as zf: + if any(".." in name or name.startswith("/") for name in zf.namelist()): + return None zf.extractall(extract_dir) except Exception: return None @@ -372,12 +371,13 @@ def _parse_status_file(status_file: Path) -> list[dict]: def fetch_all_review_statuses( token: str, repo: str, run_id: str, ) -> list[dict]: - """Fetch and merge all review-status artifacts across all runs. + """Fetch and merge all review-status artifacts from the run. - Enumerates artifacts with the ``review-status-`` prefix from the - orchestrator run and all sub-workflow runs (workflow_call). Downloads - each, parses the ``review-status.json`` inside, and merges into a - single flat array. + Lists artifacts with the ``review-status-`` prefix on the orchestrator + run, downloads each, parses the ``review-status.json`` inside, and + merges into a single flat array. GitHub attaches artifacts uploaded by + reusable workflow jobs to the caller run, so one listing covers every + status-producing job. Returns the merged list of ``{source, results: [...]}`` objects. Artifacts that don't exist yet or fail to parse are silently skipped. @@ -386,37 +386,43 @@ def fetch_all_review_statuses( temp_base = Path("/tmp/review-status-artifacts") try: - run_ids = _list_run_ids(token, repo, run_id) + artifacts = _list_artifacts(token, repo, run_id) except Exception: return all_statuses - for rid in run_ids: - try: - artifacts = _list_artifacts(token, repo, rid) - except Exception: + rs_artifacts = [ + a for a in artifacts + if a.get("name", "").startswith(_REVIEW_STATUS_ARTIFACT_PREFIX) + ] + if not rs_artifacts: + return all_statuses + + # Clean temp dir for this run's artifacts. + run_dl_dir = temp_base / str(run_id) + if run_dl_dir.exists(): + shutil.rmtree(run_dl_dir) + run_dl_dir.mkdir(parents=True, exist_ok=True) + + for artifact in rs_artifacts: + status_file = _download_artifact(token, repo, artifact, run_dl_dir) + if status_file is None: continue + statuses = _parse_status_file(status_file) + all_statuses.extend(statuses) - rs_artifacts = [ - a for a in artifacts - if a.get("name", "").startswith(_REVIEW_STATUS_ARTIFACT_PREFIX) - ] - if not rs_artifacts: + # A re-run can leave several non-expired artifacts with the same name, + # each carrying the same source — dedupe by source so the comment + # doesn't render duplicate sections. + seen: set[str] = set() + deduped: list[dict] = [] + for status in all_statuses: + src = status.get("source", "") + if src in seen: continue - - # Clean temp dir for this run's artifacts. - run_dl_dir = temp_base / rid - if run_dl_dir.exists(): - shutil.rmtree(run_dl_dir) - run_dl_dir.mkdir(parents=True, exist_ok=True) - - for artifact in rs_artifacts: - status_file = _download_artifact(token, repo, artifact, run_dl_dir) - if status_file is None: - continue - statuses = _parse_status_file(status_file) - all_statuses.extend(statuses) - - return all_statuses + if src: + seen.add(src) + deduped.append(status) + return deduped # --------------------------------------------------------------------------- @@ -454,11 +460,6 @@ def build_comment_body( ) -def _merge_statuses(statuses: list[dict]) -> str: - """Merge a list of status arrays into one JSON string.""" - return json.dumps(statuses) if statuses 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: @@ -484,7 +485,9 @@ def run( ) -> int: """Poll for job statuses and update the PR comment until all done. - Returns 0 always — comment posting is best-effort. + Returns 0 on success; 1 when all jobs completed but a dependency + failed (so ``gh run rerun --failed`` can pick it up). Comment posting + is best-effort. """ asm = _import_assembler() start = time.time() @@ -526,14 +529,15 @@ def run( if gone_pending: print(f" → {len(gone_pending)} job(s) disappeared from pending: {', '.join(gone_pending)}") - # Dynamically fetch all review-status artifacts from every run. + # Dynamically fetch all review-status artifacts from the run. artifact_statuses = fetch_all_review_statuses(token, repo, run_id) - if len(artifact_statuses) != prev_artifact_count: + artifact_count_changed = len(artifact_statuses) != prev_artifact_count + if artifact_count_changed: print(f" Found {len(artifact_statuses)} review status entries from artifacts " f"(was {prev_artifact_count} last poll)") prev_artifact_count = len(artifact_statuses) - merged_json = _merge_statuses(artifact_statuses) + merged_json = json.dumps(artifact_statuses) if artifact_statuses else "" current_commit_info = _commit_info_for_state(commit_info, pending) body = build_comment_body( @@ -550,7 +554,7 @@ def run( change_reasons.append(f"{len(new_pending)} new pending job(s)") if gone_pending: change_reasons.append(f"{len(gone_pending)} job(s) left pending") - if len(artifact_statuses) != prev_artifact_count: + if artifact_count_changed: change_reasons.append("artifact statuses updated") if not change_reasons: change_reasons.append("initial post") diff --git a/tests/ci/test_live_comment.py b/tests/ci/test_live_comment.py deleted file mode 100644 index 484ba3322569..000000000000 --- a/tests/ci/test_live_comment.py +++ /dev/null @@ -1,207 +0,0 @@ -"""Tests for scripts/ci/live_comment.py — classify_jobs() + artifact helpers. - -The poller's core logic is a pure function: take raw GitHub API job dicts -and split them into (completed, pending). Artifact parsing helpers are also -pure and tested here. The API wrapper + polling loop are tested via E2E -in CI, not here. -""" - -from __future__ import annotations - -import importlib.util -import json -import sys -import tempfile -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_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_queued(): - jobs = [_job("Python tests", "queued", 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_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_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_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" - ) - - -# --------------------------------------------------------------------------- -# Artifact parsing helpers -# --------------------------------------------------------------------------- - -def test_parse_status_file_with_prefix(): - """GITHUB_OUTPUT format: review_status=""" - with tempfile.NamedTemporaryFile(mode="w", suffix=".json", delete=False) as f: - f.write('review_status=[{"source":"test","results":[]}]') - f.flush() - statuses = _mod._parse_status_file(Path(f.name)) - assert len(statuses) == 1 - assert statuses[0]["source"] == "test" - - -def test_parse_status_file_without_prefix(): - """Raw JSON (no review_status= prefix) is also accepted.""" - with tempfile.NamedTemporaryFile(mode="w", suffix=".json", delete=False) as f: - f.write('[{"source":"raw","results":[]}]') - f.flush() - statuses = _mod._parse_status_file(Path(f.name)) - assert len(statuses) == 1 - assert statuses[0]["source"] == "raw" - - -def test_parse_status_file_empty_array(): - with tempfile.NamedTemporaryFile(mode="w", suffix=".json", delete=False) as f: - f.write('review_status=[]') - f.flush() - statuses = _mod._parse_status_file(Path(f.name)) - assert statuses == [] - - -def test_parse_status_file_invalid_json(): - with tempfile.NamedTemporaryFile(mode="w", suffix=".json", delete=False) as f: - f.write('review_status=not json') - f.flush() - statuses = _mod._parse_status_file(Path(f.name)) - assert statuses == [] - - -def test_parse_status_file_nonexistent(): - assert _mod._parse_status_file(Path("/nonexistent/file.json")) == [] - - -def test_parse_status_file_not_a_list(): - with tempfile.NamedTemporaryFile(mode="w", suffix=".json", delete=False) as f: - f.write('review_status={"not":"a list"}') - f.flush() - statuses = _mod._parse_status_file(Path(f.name)) - assert statuses == [] - - -def test_merge_statuses_empty(): - assert _mod._merge_statuses([]) == "" - - -def test_merge_statuses_single(): - statuses = [{"source": "a", "results": []}] - result = _mod._merge_statuses(statuses) - assert json.loads(result) == statuses - - -def test_merge_statuses_multiple(): - statuses = [ - {"source": "a", "results": []}, - {"source": "b", "results": [{"kind": "warning"}]}, - ] - result = _mod._merge_statuses(statuses) - assert json.loads(result) == statuses - - -def test_review_status_artifact_prefix(): - """The prefix is used to filter artifacts from the API.""" - assert _mod._REVIEW_STATUS_ARTIFACT_PREFIX == "review-status-" - # Artifacts with this prefix should be picked up - assert "review-status-ci-timings".startswith(_mod._REVIEW_STATUS_ARTIFACT_PREFIX) - assert "review-status-review-labels".startswith(_mod._REVIEW_STATUS_ARTIFACT_PREFIX) - # Artifacts without it should not - assert not "ci-timings-report".startswith(_mod._REVIEW_STATUS_ARTIFACT_PREFIX) - assert not "playwright-report".startswith(_mod._REVIEW_STATUS_ARTIFACT_PREFIX)