diff --git a/scripts/ci/assemble_review_comment.py b/scripts/ci/assemble_review_comment.py index 312f3c0587678..5e8dc64ebdb7d 100644 --- a/scripts/ci/assemble_review_comment.py +++ b/scripts/ci/assemble_review_comment.py @@ -258,7 +258,12 @@ def _render_pending_items(pending_jobs: list[str]) -> str: 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: +def render_comment( + items: list[ReviewItem], + pending_jobs: list[str] | None = None, + commit_info: str = "", + waiting: bool = False, +) -> str: """Render the full comment body from a list of review items. Items are grouped by severity under ``##`` group headers, separated @@ -270,6 +275,12 @@ def render_comment(items: list[ReviewItem], pending_jobs: list[str] | None = Non 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. + + ``waiting`` means a workflow run is still queued or in progress even + though no individual job is visibly pending — GitHub has not spawned + the jobs yet. The comment must not look final in that state, so the + "all good!" banner is replaced by a waiting note and a dimmed footer + marks the comment as still live. """ pending = pending_jobs or [] @@ -288,6 +299,8 @@ def render_comment(items: list[ReviewItem], pending_jobs: list[str] | None = Non body += f"{commit_info}\n\n" if not items and not pending: + if waiting: + return f"{body}waiting for jobs to start…" return f"{body}all good!" sections: list[str] = [] @@ -306,6 +319,8 @@ def render_comment(items: list[ReviewItem], pending_jobs: list[str] | None = Non if pending: body += _render_pending_items(pending) + elif waiting: + body += "\n\n---\n\nwaiting for more jobs to start…\n" if sections: body += "\n\n---\n\n".join(sections) @@ -358,6 +373,7 @@ def assemble( review_statuses_json: str = "", pending_jobs: list[str] | None = None, commit_info: str = "", + waiting: bool = False, ) -> str: """Assemble the full comment body from all available inputs.""" items: list[ReviewItem] = [] @@ -372,7 +388,7 @@ def assemble( # 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) + return render_comment(items, pending_jobs, commit_info, waiting=waiting) # --------------------------------------------------------------------------- diff --git a/scripts/ci/live_comment.py b/scripts/ci/live_comment.py index 883999e97c854..d94508d6d49a0 100644 --- a/scripts/ci/live_comment.py +++ b/scripts/ci/live_comment.py @@ -212,13 +212,30 @@ def select_watched_runs( return list(newest.values()) +def runs_all_completed(runs: list[dict]) -> bool: + """True only when every run in the list reports ``status: completed``. + + The job list alone cannot answer "is CI done": a run that GitHub just + created has no jobs yet, and a mid-run poll can catch the moment where + every visible job finished but a downstream sub-workflow has not + spawned its jobs. Both look identical to "all done" at the job level. + The run's own ``status`` is the authoritative signal, so the poller + must not exit while any relevant run is still ``queued`` or + ``in_progress``. An empty list is not done — it means the poller has + no run information at all. + """ + return bool(runs) and all(str(r.get("status", "")) == "completed" for r in runs) + + def collect_run_jobs( token: str, repo: str, run_id: str, watch_workflows: list[str] | None = None, -) -> list[dict]: +) -> tuple[list[dict], bool]: """Collect all jobs in the CI run + any watched sibling runs. - Returns a flat list of job dicts (same shape as the API returns, plus - ``_workflow_name`` on jobs from a watched run). + Returns ``(jobs, runs_completed)``: a flat list of job dicts (same + shape as the API returns, plus ``_workflow_name`` on jobs from a + watched run), and whether the CI run and every selected watched run + report ``status: completed`` (see :func:`runs_all_completed`). Reusable-workflow (``workflow_call``) jobs need no special handling: GitHub flattens them into the caller run's job list, already named @@ -247,7 +264,7 @@ def collect_run_jobs( all_jobs.append(job) if not watch_workflows or not head_sha: - return all_jobs + return all_jobs, runs_all_completed([run_info]) # Watched sibling runs for the same commit. A run can be absent on the # first polls. Then classify_jobs() shows nothing for it. @@ -255,7 +272,9 @@ def collect_run_jobs( f"{API_BASE}/repos/{owner}/{repo_name}/actions/runs?head_sha={head_sha}&per_page=100", token, list_key="workflow_runs", ) + relevant_runs = [run_info] for watched in select_watched_runs(sibling_runs, watch_workflows, exclude_run_id=run_id): + relevant_runs.append(watched) watched_jobs = _api_get_paginated( f"{API_BASE}/repos/{owner}/{repo_name}/actions/runs/{watched['id']}/jobs", token, list_key="jobs", @@ -264,7 +283,7 @@ def collect_run_jobs( job["_workflow_name"] = watched.get("name", "") all_jobs.append(job) - return all_jobs + return all_jobs, runs_all_completed(relevant_runs) def find_comment_id(token: str, repo: str, pr_number: str) -> int | None: @@ -490,6 +509,7 @@ def build_comment_body( job_urls: dict[str, str], review_statuses_json: str, commit_info: str = "", + waiting: bool = False, ) -> str: """Assemble the comment body from current job states + static inputs.""" needs_json = json.dumps(completed) if completed else "" @@ -501,10 +521,11 @@ def build_comment_body( review_statuses_json=review_statuses_json, pending_jobs=pending if pending else None, commit_info=commit_info, + waiting=waiting, ) -def _commit_info_for_state(commit_info: str, pending: list[str]) -> str: +def _commit_info_for_state(commit_info: str, pending: bool) -> str: """Use past tense in the final comment after every CI job completes.""" if pending: return commit_info @@ -549,7 +570,7 @@ def run( break try: - jobs = collect_run_jobs(token, repo, run_id, watch_workflows) + jobs, runs_completed = collect_run_jobs(token, repo, run_id, watch_workflows) except Exception as e: print(f" API error collecting jobs: {e}", file=sys.stderr) time.sleep(interval) @@ -583,12 +604,17 @@ def run( prev_artifact_count = len(artifact_statuses) merged_json = json.dumps(artifact_statuses) if artifact_statuses else "" - current_commit_info = _commit_info_for_state(commit_info, pending) + # The run status is authoritative for "done": an empty job list on + # a run that is still queued/in_progress means GitHub has not + # spawned the jobs yet, not that everything passed. + all_done = not pending and runs_completed + current_commit_info = _commit_info_for_state(commit_info, pending=not all_done) body = build_comment_body( asm, completed, pending, run_url, job_urls, merged_json, current_commit_info, + waiting=not runs_completed, ) if body != last_body: @@ -626,13 +652,14 @@ def run( prev_completed = completed prev_pending = pending - if not pending and not quiet_grace_used: + if all_done and not quiet_grace_used: quiet_grace_used = True - print(" No visible jobs pending — waiting 10s for downstream jobs to appear.") + print(" No jobs pending and runs report completed — " + "waiting 10s for downstream jobs to appear.") time.sleep(10) continue - if not pending: + if all_done: failed = [name for name, result in completed.items() if result == "failure"] if failed: print(f" All jobs done, {len(failed)} failed: {', '.join(failed)}") @@ -640,6 +667,10 @@ def run( print(" All jobs completed — done.") break + if not pending: + print(" No visible jobs pending, but a run is still queued or " + "in progress — waiting for its jobs to appear.") + quiet_grace_used = False time.sleep(interval) diff --git a/tests/ci/test_assemble_review_comment.py b/tests/ci/test_assemble_review_comment.py index dfceb698a4fa0..b7e60e0a07634 100644 --- a/tests/ci/test_assemble_review_comment.py +++ b/tests/ci/test_assemble_review_comment.py @@ -181,6 +181,35 @@ def test_render_pending_notif(): assert "Still running 1 job: `ci-timings`" in body +# ─── render_comment (waiting for jobs to start) ─────────────────────── + + +def test_waiting_with_no_items_shows_waiting_not_all_good(): + """A run with no jobs yet must not render the final 'all good!' banner.""" + body = _mod.render_comment([], waiting=True) + assert "all good" not in body + assert "waiting for jobs to start" in body + + +def test_waiting_with_items_but_no_pending_keeps_a_live_footer(): + """Between job waves: results exist, nothing pending, run not done.""" + items = [ReviewItem(severity="info", title="lockfile", summary="No changes.")] + body = _mod.render_comment(items, waiting=True) + assert "waiting for more jobs to start" in body + assert "### lockfile" in body + + +def test_not_waiting_and_no_items_still_renders_all_good(): + body = _mod.render_comment([]) + assert "all good!" in body + + +def test_assemble_passes_waiting_through(): + body = _mod.assemble(waiting=True) + assert "waiting for jobs to start" in body + assert "all good" not in body + + diff --git a/tests/ci/test_live_comment.py b/tests/ci/test_live_comment.py index 25d3f9c85dc6c..b4c1329eb05b8 100644 --- a/tests/ci/test_live_comment.py +++ b/tests/ci/test_live_comment.py @@ -113,3 +113,47 @@ def test_workflow_watch_list_names_a_workflow_that_exists(): known.add(doc["name"]) assert set(watched) <= known, f"unknown workflow names: {set(watched) - known}" + + +def test_poller_never_watches_its_own_workflow(): + """The poller's own run must never gate completion. + + ``runs_all_completed`` waits until every relevant run is completed. + The poller's run is in progress for as long as it polls, so watching + itself would make the loop wait for itself and only ever exit on + timeout. + """ + yaml = pytest.importorskip("yaml") + root = Path(__file__).resolve().parents[2] + doc = yaml.safe_load( + (root / ".github/workflows/ci-review-comment.yml").read_text(encoding="utf-8") + ) + own_name = doc["name"] + step = next( + s for s in doc["jobs"]["comment"]["steps"] + if "WATCH_WORKFLOWS" in (s.get("env") or {}) + ) + watched = _mod.parse_watch_workflows(step["env"]["WATCH_WORKFLOWS"]) + assert own_name not in watched + + +# ─── runs_all_completed ─────────────────────────────────────────────── + + +def test_runs_all_completed_true_only_when_every_run_finished(): + done = {"status": "completed"} + running = {"status": "in_progress"} + queued = {"status": "queued"} + assert _mod.runs_all_completed([done]) + assert _mod.runs_all_completed([done, done]) + assert not _mod.runs_all_completed([done, running]) + assert not _mod.runs_all_completed([queued]) + + +def test_runs_all_completed_empty_list_is_not_done(): + """No run info at all must not read as 'everything passed'.""" + assert not _mod.runs_all_completed([]) + + +def test_runs_all_completed_missing_status_is_not_done(): + assert not _mod.runs_all_completed([{}])