Skip to content

Commit 88a3fd5

Browse files
committed
fix(ci): gate goes green on its own PR — recency, exclusions, messaging
The gate failed on its own PR (#2783), and replaying that PR's real Checks API history through evaluate() (not synthetic fixtures) found the actual causes and took total failures 55 -> 1 (the one remaining is the pre-existing, self-documented build_cpp_email.yml scaffold guard for #1110, unrelated to this change). Root cause, unified: GitHub never deletes a check-run — every triggering event (opened, then labeled, then reopened) adds new rows alongside old ones. evaluate() collected the full history for a SHA and gave any bad match unconditional priority over good ones, with no notion of recency. A five-minute-old stale skip was masking a fresh, real pass sitting right next to it in the same API response. Fixed with an exact rule, not a tuned time-window (the observed gap between stale and fresh ranged from about a minute to several minutes on real data, so no fixed window reliably separates "same event" from "different event"): dedup by exact check-run name keeping only the newest, then discard any pre-expansion artifact older than the newest real entry for that requirement. A pre-expansion artifact is decidable two ways, not one — the second was found the same way, by replaying real data: a job's name containing literal "${{ }}" (skipped before its matrix ever expanded), OR a job whose name has no template at all but does have a strategy.matrix, where GitHub still auto-appends "(leg, values)" to every real rendered check-run, so a real run can never produce the bare, un-suffixed name matching the job's declared name. Also: a job gated on a tag ref (`startsWith(github.ref, 'refs/tags/')`) can never run on a pull_request event at all — unlike needs.*, this is statically decidable, not just excluded on uncertainty. Every exclusion (continue-on-error, needs.*-gated, tag-ref-gated) is now named in the gate's own posted summary, not silently dropped from the required set — "enforced N/M requirements; excluded K (kind: count, ...)" with each one's reason. Messaging: the neutral and failure summaries now say label AND close/reopen, not label alone — the audited suites don't listen for `labeled`, so the label by itself cannot re-trigger them. Failure text also splits "did not run" (stuck at skipped, needs a re-trigger) from "ran and did not pass" (a real failure), since they need different advice. 6 new unit tests reproducing the real shapes found live: stale skip masked by fresh pass (both artifact forms), a genuine failure not masked by a stale skip nearby, latest-entry-still-skipped staying failed (not silently passing), a re-skip after a real pass still reported as current, and the exclusion-count summary itself.
1 parent cdbf3ac commit 88a3fd5

2 files changed

Lines changed: 569 additions & 28 deletions

File tree

.github/scripts/check_required_status.py

Lines changed: 225 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,23 @@ class CheckRun:
6464
name: str
6565
status: str
6666
conclusion: str | None
67+
# ISO 8601 — GitHub's Checks API never deletes or overwrites a prior
68+
# check-run when a job re-runs; every triggering event adds a new row.
69+
# `started_at` is what lets evaluate() tell a stale result from the
70+
# current one instead of treating the whole history as live.
71+
started_at: str = ""
72+
73+
74+
@dataclass
75+
class Exclusion:
76+
name_prefix: str
77+
source_file: str
78+
job_id: str
79+
reason: str
80+
# Short, stable category for the summary counts in format_exclusions —
81+
# kept separate from `reason` (the free-text detail) so the count
82+
# breakdown doesn't depend on parsing that string.
83+
kind: str
6784

6885

6986
@dataclass
@@ -124,7 +141,18 @@ def _iter_workflow_jobs(workflows_dir: str):
124141
if not isinstance(job, dict):
125142
continue
126143
name_tmpl = job.get("name") or job_id
127-
yield fname, doc, job_id, job, name_tmpl, "${{" in name_tmpl
144+
# A job needs prefix (not exact) matching whenever its RENDERED
145+
# check-run name can vary — either its own `name:` has a
146+
# `${{ }}` expression, OR it has a matrix strategy at all. The
147+
# second case is easy to miss: a static `name:` with a matrix
148+
# ("Audit Dependencies" + strategy.matrix.package) still gets
149+
# GitHub's own auto-appended "(leg, values)" disambiguator on
150+
# every rendered check-run — confirmed live on #2767's own PR
151+
# (#2783), where exact-matching "Audit Dependencies" against
152+
# "Audit Dependencies (example-app, ...)" never matched at all.
153+
has_matrix = bool(job.get("strategy", {}).get("matrix")) if isinstance(job.get("strategy"), dict) else False
154+
is_templated = "${{" in name_tmpl or has_matrix
155+
yield fname, doc, job_id, job, name_tmpl, is_templated
128156

129157

130158
@dataclass(frozen=True)
@@ -182,14 +210,28 @@ def _most_specific_owner(name: str, matchers: list[_NameMatcher]) -> tuple[str,
182210
return (best.source_file, best.job_id) if best else None
183211

184212

185-
def load_requirements(workflows_dir: str, exclude: Iterable[str]) -> list[Requirement]:
213+
def load_requirements(
214+
workflows_dir: str, exclude: Iterable[str]
215+
) -> tuple[list[Requirement], list[Exclusion]]:
186216
"""Parse every workflow with a `pull_request:` trigger into a flat list
187-
of per-job requirements. A job is excluded (not required) when the
188-
workflow itself has none to give: `continue-on-error: true` on that job
189-
means its own author has already declared it non-blocking.
217+
of per-job requirements, plus the jobs deliberately left OUT and why.
218+
219+
Two things make a job impossible to verify statically, and both get
220+
excluded — but reported, not silently dropped (see Exclusion): a
221+
silent exclusion is a fail-open hole inside the gate built to close
222+
fail-open holes, e.g. a future job wrapped in `needs.*` dropping out of
223+
the required set with nobody told.
224+
225+
- `continue-on-error: true` — the job's own author already declared it
226+
non-blocking.
227+
- an `if:` that references `needs.` — gated on another job's runtime
228+
output (e.g. a dynamically discovered matrix via
229+
`fromJson(needs.x.outputs.y)`), which cannot be evaluated from YAML
230+
alone without executing the DAG.
190231
"""
191232
exclude_set = set(exclude)
192233
reqs: list[Requirement] = []
234+
exclusions: list[Exclusion] = []
193235
for fname, doc, job_id, job, name_tmpl, is_templated in _iter_workflow_jobs(workflows_dir):
194236
if fname in exclude_set:
195237
continue
@@ -209,8 +251,6 @@ def load_requirements(workflows_dir: str, exclude: Iterable[str]) -> list[Requir
209251
paths = pr.get("paths")
210252
paths_ignore = pr.get("paths-ignore")
211253

212-
if job.get("continue-on-error") is True:
213-
continue
214254
# Matrix-templated names ("Foo (${{ matrix.x }})") can't be
215255
# statically expanded without evaluating GH Actions expressions;
216256
# match by the static prefix before the first expression instead.
@@ -219,6 +259,49 @@ def load_requirements(workflows_dir: str, exclude: Iterable[str]) -> list[Requir
219259
prefix = name_tmpl.split("${{", 1)[0].rstrip() if is_templated else name_tmpl
220260
if not prefix:
221261
continue
262+
263+
if job.get("continue-on-error") is True:
264+
exclusions.append(
265+
Exclusion(
266+
prefix,
267+
fname,
268+
job_id,
269+
"continue-on-error: true (job's own author declared it non-blocking)",
270+
kind="continue-on-error",
271+
)
272+
)
273+
continue
274+
job_if = job.get("if")
275+
if isinstance(job_if, str) and "needs." in job_if:
276+
exclusions.append(
277+
Exclusion(
278+
prefix,
279+
fname,
280+
job_id,
281+
f"if: references needs.* ({job_if.strip()!r}) — depends on another job's runtime output, not statically verifiable",
282+
kind="needs-gated",
283+
)
284+
)
285+
continue
286+
# Unlike needs.*, this one IS statically decidable: github.ref on a
287+
# pull_request event is always the PR's merge ref, never a tag ref,
288+
# so a job gated on refs/tags/ can never run in PR context at all —
289+
# confirmed live on #2767's own PR (build_agents.yml's "Consolidate
290+
# release bundle", `if: startsWith(github.ref, 'refs/tags/v')`,
291+
# sitting at conclusion=skipped on every PR run forever). Excluding
292+
# it is correcting a real derivation gap, not hedging on the unknown.
293+
if isinstance(job_if, str) and "refs/tags/" in job_if:
294+
exclusions.append(
295+
Exclusion(
296+
prefix,
297+
fname,
298+
job_id,
299+
f"if: gated on a tag ref ({job_if.strip()!r}) — can never run on a pull_request event",
300+
kind="ref-conditional",
301+
)
302+
)
303+
continue
304+
222305
reqs.append(
223306
Requirement(
224307
name_prefix=prefix,
@@ -229,7 +312,7 @@ def load_requirements(workflows_dir: str, exclude: Iterable[str]) -> list[Requir
229312
job_id=job_id,
230313
)
231314
)
232-
return reqs
315+
return reqs, exclusions
233316

234317

235318
def is_gated_off(is_draft: bool, labels: set[str]) -> bool:
@@ -255,6 +338,75 @@ def is_path_relevant(req: Requirement, changed_files: list[str]) -> bool:
255338
return any(not _matches_any(cf, req.paths_ignore) for cf in changed_files)
256339

257340

341+
def _is_pre_expansion_artifact(name: str, req: Requirement) -> bool:
342+
"""True when `name` is what a matrix job's check-run looks like BEFORE
343+
its strategy ever expanded (job `if:` was false, e.g. still draft) —
344+
confirmed live on #2767's own PRs, in two different shapes:
345+
346+
1. The job's own `name:` contains `${{ }}` — GitHub posts the raw,
347+
un-rendered template string, e.g.
348+
"Test Apps Build (${{ matrix.app.name }})".
349+
2. The job's `name:` is fully static but it has a `strategy.matrix`
350+
anyway — GitHub normally disambiguates each real leg by appending
351+
"(leg, values)" (e.g. "Audit Dependencies (jira-app, ...)"), but a
352+
job skipped before expansion posts the bare, un-suffixed name with
353+
nothing appended: "Audit Dependencies", identical to the derived
354+
prefix itself. A REAL run of a multi-leg matrix job can never
355+
produce that bare, un-suffixed name — GitHub always appends
356+
something once legs actually exist — so `name == req.name_prefix`
357+
is exact and decidable, not a heuristic, for this shape.
358+
359+
Only meaningful for prefix-mode (non-exact) requirements; exact-mode
360+
requirements have no matrix-expansion ambiguity to begin with.
361+
"""
362+
if "${{" in name:
363+
return True
364+
return not req.exact and name == req.name_prefix
365+
366+
367+
def _current_matches(matches: list[CheckRun], req: Requirement) -> list[CheckRun]:
368+
"""Reduce a requirement's raw check-run matches to the ones that
369+
represent its CURRENT state, not its full history.
370+
371+
GitHub never deletes or overwrites a check-run — every triggering event
372+
(opened, then later labeled, then later reopened, ...) adds new rows
373+
alongside the old ones. Two things fall out of that:
374+
375+
1. The same exact name can appear more than once (e.g. "discover-apps"
376+
re-run after a draft PR is reopened). Keep only the newest per name.
377+
2. A matrix job whose `if:` was false BEFORE its strategy expanded posts
378+
exactly ONE check-run in its pre-expansion form (see
379+
_is_pre_expansion_artifact) — confirmed live on #2767's own
380+
throwaway and real PRs. That row is meaningless once the SAME job
381+
has actually run for real anywhere in this SHA's history (producing
382+
real, expanded rows) — so once at least one non-artifact row exists,
383+
discard any artifact row OLDER than the newest non-artifact one. An
384+
artifact row NEWER than every non-artifact row is kept: that's a
385+
genuine re-skip (e.g. the PR went back to draft after a real pass),
386+
and the gate should report that as current, not paper over it.
387+
388+
Deliberately not a fixed time-window ("group everything within N
389+
seconds"): the observed gap between a stale skip and its real re-run
390+
ranged from about a minute to several minutes on real PRs in this repo,
391+
so no fixed window reliably separates "same event" from "different
392+
event." Comparing by exact name + the decidable artifact rule above
393+
needs no tuned constant and is exact rather than approximate.
394+
"""
395+
latest_by_name: dict[str, CheckRun] = {}
396+
for cr in matches:
397+
cur = latest_by_name.get(cr.name)
398+
if cur is None or cr.started_at > cur.started_at:
399+
latest_by_name[cr.name] = cr
400+
deduped = list(latest_by_name.values())
401+
402+
non_artifact = [cr for cr in deduped if not _is_pre_expansion_artifact(cr.name, req)]
403+
artifact = [cr for cr in deduped if _is_pre_expansion_artifact(cr.name, req)]
404+
if non_artifact:
405+
newest_real_ts = max(cr.started_at for cr in non_artifact)
406+
artifact = [cr for cr in artifact if cr.started_at > newest_real_ts]
407+
return non_artifact + artifact
408+
409+
258410
def evaluate(
259411
requirements: list[Requirement],
260412
changed_files: list[str],
@@ -281,6 +433,7 @@ def evaluate(
281433
if not matches:
282434
result.missing.append(req)
283435
continue
436+
matches = _current_matches(matches, req)
284437
not_completed = [cr for cr in matches if cr.status != "completed"]
285438
bad = [cr for cr in matches if cr.status == "completed" and cr.conclusion in TERMINAL_BAD_CONCLUSIONS]
286439
good = [cr for cr in matches if cr.status == "completed" and cr.conclusion in PASSING_CONCLUSIONS]
@@ -350,7 +503,7 @@ def fetch_check_runs(repo: str, sha: str) -> list[CheckRun]:
350503
f"repos/{repo}/commits/{sha}/check-runs",
351504
"--paginate",
352505
"-q",
353-
".check_runs[] | {name, status, conclusion}",
506+
".check_runs[] | {name, status, conclusion, started_at}",
354507
],
355508
capture_output=True,
356509
text=True,
@@ -366,10 +519,38 @@ def fetch_check_runs(repo: str, sha: str) -> list[CheckRun]:
366519
if not line:
367520
continue
368521
obj = json.loads(line)
369-
runs.append(CheckRun(name=obj["name"], status=obj["status"], conclusion=obj.get("conclusion")))
522+
runs.append(
523+
CheckRun(
524+
name=obj["name"],
525+
status=obj["status"],
526+
conclusion=obj.get("conclusion"),
527+
started_at=obj.get("started_at") or "",
528+
)
529+
)
370530
return runs
371531

372532

533+
def format_exclusions(num_enforced: int, exclusions: list[Exclusion]) -> str:
534+
"""Scope summary for every posted check-run, not just the exclusion
535+
list itself — with three exclusion classes now, a bare list is easy to
536+
skim past as a workflow evolves and the excluded set quietly grows.
537+
Stating it as a ratio keeps the gate's own coverage auditable at a
538+
glance: "enforced N, excluded M (kind: count, ...)".
539+
"""
540+
total = num_enforced + len(exclusions)
541+
if not exclusions:
542+
return f" Enforced {num_enforced}/{total} requirement(s); none excluded."
543+
by_kind: dict[str, int] = {}
544+
for e in exclusions:
545+
by_kind[e.kind] = by_kind.get(e.kind, 0) + 1
546+
breakdown = ", ".join(f"{kind}: {count}" for kind, count in sorted(by_kind.items()))
547+
items = "; ".join(f"{e.name_prefix} (from {e.source_file}): {e.reason}" for e in exclusions)
548+
return (
549+
f" Enforced {num_enforced}/{total} requirement(s); excluded {len(exclusions)} "
550+
f"as not statically verifiable ({breakdown}): {items}."
551+
)
552+
553+
373554
def format_report(result: EvalResult, elapsed: int) -> str:
374555
lines = []
375556
if result.ok:
@@ -402,6 +583,13 @@ def main() -> int:
402583
is_draft = args.draft == "true"
403584
labels = {label.strip() for label in args.labels.split(",") if label.strip()}
404585

586+
requirements, exclusions = load_requirements(args.workflows_dir, args.exclude)
587+
exclusion_note = format_exclusions(len(requirements), exclusions)
588+
print(exclusion_note.strip())
589+
if exclusions:
590+
for e in exclusions:
591+
print(f" - {e.name_prefix} (from {e.source_file}, {e.kind}): {e.reason}")
592+
405593
if is_gated_off(is_draft, labels):
406594
# Deliberately NOT skipped via a job-level `if:` — see the module
407595
# docstring on post_check_run for why a job that skips itself here
@@ -415,14 +603,18 @@ def main() -> int:
415603
"This PR is a draft without the `ready_for_ci` label, so the "
416604
"suites this gate audits have not run (by the same convention "
417605
"every audited workflow uses). Nothing has been verified — this "
418-
"is not a pass. Mark the PR ready for review, or add the "
419-
"`ready_for_ci` label, to require and verify them."
606+
"is not a pass. Two ways to require and verify them: mark the PR "
607+
"ready for review, OR add the `ready_for_ci` label AND THEN close "
608+
"and reopen the PR. The label alone is not enough — the audited "
609+
"suites only listen for opened/synchronize/reopened/ready_for_review "
610+
"events, not `labeled`, so adding the label by itself does not "
611+
"re-trigger them (only this gate re-triggers on `labeled`)."
612+
+ exclusion_note
420613
)
421614
print(f"GATED OFF: {summary}")
422615
post_check_run(args.repo, args.sha, "neutral", title, summary)
423616
return 0
424617

425-
requirements = load_requirements(args.workflows_dir, args.exclude)
426618
all_matchers = _collect_all_name_matchers(args.workflows_dir)
427619
changed_files = fetch_changed_files(args.changed_files)
428620

@@ -438,7 +630,23 @@ def main() -> int:
438630
print(format_report(result, elapsed))
439631

440632
if result.is_terminal_failure:
441-
details = "; ".join(f"{req.name_prefix} ({reason})" for req, reason in result.failed)
633+
# "did not run" (stuck at skipped — needs a re-trigger) and "ran
634+
# and failed" (a real failure/timeout/cancellation) are different
635+
# states and need different advice, not one blanket message.
636+
not_run = [(req, reason) for req, reason in result.failed if "=skipped" in reason]
637+
really_failed = [(req, reason) for req, reason in result.failed if "=skipped" not in reason]
638+
parts = []
639+
if not_run:
640+
names = "; ".join(f"{req.name_prefix} ({reason})" for req, reason in not_run)
641+
parts.append(
642+
"Did not run (stuck at 'skipped', not re-evaluated for the current "
643+
f"state — add `ready_for_ci` AND close/reopen the PR to re-trigger "
644+
f"them, the label alone will not): {names}"
645+
)
646+
if really_failed:
647+
names = "; ".join(f"{req.name_prefix} ({reason})" for req, reason in really_failed)
648+
parts.append(f"Ran and did not pass: {names}")
649+
details = " ".join(parts)
442650
for req, reason in result.failed:
443651
print(
444652
f"::error::Required check '{req.name_prefix}' (from "
@@ -451,7 +659,7 @@ def main() -> int:
451659
args.sha,
452660
"failure",
453661
"Required suites did not pass",
454-
f"One or more required, path-relevant checks did not pass: {details}",
662+
details + exclusion_note,
455663
)
456664
return 1
457665
if result.is_fully_ok:
@@ -462,7 +670,7 @@ def main() -> int:
462670
args.sha,
463671
"success",
464672
"All required suites passed",
465-
f"Required and path-relevant for this diff: {names}",
673+
f"Required and path-relevant for this diff: {names}." + exclusion_note,
466674
)
467675
return 0
468676
if elapsed >= args.timeout_seconds:
@@ -477,7 +685,7 @@ def main() -> int:
477685
args.sha,
478686
"failure",
479687
"Required suites never reported",
480-
f"Timed out after {args.timeout_seconds}s waiting on: {details}",
688+
f"Timed out after {args.timeout_seconds}s waiting on: {details}." + exclusion_note,
481689
)
482690
return 1
483691
time.sleep(args.poll_seconds)

0 commit comments

Comments
 (0)