fix(ci): don't report all-good before jobs start
The live comment poller inferred completion from the job list. An empty job list looks the same as a finished run: GitHub has not spawned the jobs yet, so nothing is pending, and the poller posted a final "all good!" comment and exited. The run status is now the authoritative signal. collect_run_jobs() returns whether the CI run and every watched sibling run report status=completed, and the loop exits only when no job is pending AND all runs are complete. While a run is still queued or in progress with no visible jobs, the comment shows "waiting for jobs to start" instead of a final banner.
This commit is contained in:
parent
cd4317b449
commit
8359e760be
|
|
@ -258,7 +258,12 @@ def _render_pending_items(pending_jobs: list[str]) -> str:
|
|||
return f"\n\n---\n\n<sub>Still running {len(pending_jobs)} job{'s' if len(pending_jobs) != 1 else ''}: {job_list}</sub>\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 ``<details>`` 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}<sub>waiting for jobs to start…</sub>"
|
||||
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\n<sub>waiting for more jobs to start…</sub>\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)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
||||
|
|
|
|||
|
|
@ -181,6 +181,35 @@ def test_render_pending_notif():
|
|||
assert "<sub>Still running 1 job: `ci-timings`</sub>" 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
|
||||
|
||||
|
||||
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -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([{}])
|
||||
|
|
|
|||
Loading…
Reference in New Issue