diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index a19e6b606..adf545218 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -17922,6 +17922,44 @@ today's instance and leaves the instrument exactly as blind.** > is a different set of runs the PR page does not surface. > > **Scored 2026-09-03 -> P2.** Value **6/10** · Difficulty **3/10** · _quick win_. Partly shipped, and more has landed since filing than the first pass credited. The visibility half is in -- .github/workflows/failure-signal.yml:71 recovers the pull request number from a merge_group ref and :92 applies ci-red, so an ejection is recorded, though nothing in this tree reads that label (grep for ci-red returns the writing workflow plus prose at CLAUDE.md:302 and docs/METHOD.md:365). The windows-2025 hang limb is no longer undiagnosed either: .github/workflows/ci.yml:852 now passes --max-worker-restart=0, landed 2026-09-02 in 042ef7ff5, and the comment from :826 to :850 reads the mechanism out of the pinned pytest-xdist and records four hangs of 25 to 46 minutes with the inner watchdogs armed, which is this row's process-level deadlock below pytest. What is left is two tests -- tests/test_api_request_timeout.py:99 still runs a route against a 0.1 second deadline, and tests/test_sqlserver_store.py:4039 is unchanged and runs at ci.yml:1772 under a wrapper that retries only a native crash (ci.yml:1769), never exit 1; the hostile-disposition test the row names was already a ratio assertion before filing (tests/test_multipart.py:142). Difficulty falls to 3 because the hard limb landed and the seams exist -- pyproject.toml:195 already ships pytest-rerunfailures and tests/test_load_failover_sqlserver.py:71 marks a sibling flaky -- with the SQL Server arm provable only on the gated CI leg. +> +> **WORKED 2026-09-03. Two limbs closed, one was already closed by somebody else, and the run +> census in this item is wrong by one -- there were FOUR failing `merge_group` CI runs, not three.** +> Read from the Actions API with `--paginate`; an unpaginated `per_page=100` query returns 100 of +> `total_count` 190 and silently drops half the population, which is the truncation trap #1417's +> notes already record. The corrected census, each attributed from its own job log: +> +> | run | id | failing job | failing test | +> |---|---|---|---| +> | 11:15:01Z | 33249679477 | test (windows-2025) | hung 55 min, no FAILED line | +> | 11:37:13Z | 33250553925 | test (windows-2025) | `test_api_request_timeout.py::test_a_fast_handler_is_untouched` -- `assert 503 == 200` | +> | 12:42:51Z | 33253197221 | test (windows-2025) + sql server 2022 | `test_multipart.py::test_hostile_disposition_header_parses_in_linear_time`; `test_cipher_invocations_upsert_is_atomic_and_additive` | +> | 13:01:54Z | 33253973185 | test (windows-2025) + sql server 2025 | `test_dr_activation`, `test_connscale_smoke` x2; `test_cipher_invocations_upsert_is_atomic_and_additive` | +> +> **So "three DIFFERENT reasons with no overlap" is not what the logs say.** The SQL Server test +> failed in BOTH of the last two runs, identically. The no-overlap claim held only across the three +> runs the original pass sampled. +> +> **Closed here.** (1) `tests/test_api_request_timeout.py` -- the 0.1s deadline is gone from the arms +> that assert a prompt handler is UNTOUCHED, which is the arm that actually ejected #669 at 11:37. +> The arms asserting the bound FIRES keep 0.1s, because there the margin runs the forgiving way. +> While in the file, `test_a_disabled_deadline_lets_a_slow_handler_finish` asked the FAST route, so +> it could not fail for its stated reason; it asks the slow route now. (2) `scripts/ci/report_ci_red.py` +> reads the `ci-red` label back and names the run, marking `merge_group` runs as invisible on the PR +> page -- the half that was missing since PR #716 wrote the label. +> +> **Already closed by #738, not by this item.** `test_cipher_invocations_upsert_is_atomic_and_additive` +> failed `StoreAcquireTimeout` after 30s because the aioodbc pool shared the event loop's default +> executor (8 threads on a 4-vCPU runner) against a `pool_size` of 40, so 20 concurrent upserts +> deadlocked on the executor queue. `cc5e09e1e` (2026-09-02) gave the store its own executor and names +> this exact test and error in its message. Nothing was rebuilt here. +> +> **STILL OPEN, and it is the one the earlier pass waved through.** +> `test_multipart.py::test_hostile_disposition_header_parses_in_linear_time` was dismissed as "already +> a ratio assertion". It is one, and it ejected #669 anyway: it failed at ratio **8.02 against a bound +> of 8.0**, a 0.25 percent margin. The best-of-3 sampling it relies on landed 2026-07-28 in +> `db53fd45d`, so the code that failed is the code in the tree today. Being a ratio rather than a +> wall-clock budget did not save it, and this needs its own read. **Cluster:** CI / merge queue. **Priority:** P2. **Verdict:** build. **Severity:** a PR whose every required check is green cannot land, and nothing on the PR says why. diff --git a/scripts/ci/report_ci_red.py b/scripts/ci/report_ci_red.py new file mode 100644 index 000000000..662124fae --- /dev/null +++ b/scripts/ci/report_ci_red.py @@ -0,0 +1,278 @@ +#!/usr/bin/env python3 +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""Read the ``ci-red`` label back, and say WHICH run reddened each pull request (BACKLOG #1385). + +``failure-signal.yml`` writes the label. Until this script, **nothing read it** -- a grep for +``ci-red`` returned the writing workflow plus prose in ``CLAUDE.md`` and ``docs/METHOD.md`` saying so. +A signal nobody reads is not a signal, and this is the other half. + +THE DEFECT IT EXISTS FOR, measured on PR 669. That pull request entered the merge queue and was +ejected, twice, while **its own required contexts were green on the PR page**. The failures were in +``merge_group`` runs -- the branch merged with ``main``, a different set of runs the PR page does not +surface at all. Three full CI cycles were spent discovering that by hand. The attribution was +recoverable from the API the whole time; nothing asked. + +WHAT THIS PRINTS that ``gh pr view`` structurally cannot: for each labelled pull request, the newest +FAILING run attributed to it, marked ``[merge_group]`` when the run is one the PR page cannot show. +That mark is the finding, not decoration -- it is the difference between "your change is broken" and +"your change conflicts with what landed since", and the PR page renders the second as green. + +TWO RULES ARE COPIED FROM THE WRITER ON PURPOSE, because a reader that classifies differently from +the writer reports causes the label was never applied for: + + * **The watched workflows** (``_WATCHED``) match ``failure-signal.yml``'s ``workflows:`` list. CLA + Assistant is excluded there, deliberately, and so is excluded here. + * **Only ``failure`` counts** (``_RED``). A CANCELLED run is not a red -- branch protection gates on + the latest head, so a cancelled predecessor says nothing about the current one. Counting it would + misattribute every merge-queue ejection, which cancels its siblings on the way out. + +AND THE ATTRIBUTION RULE IS THE WRITER'S, IN THE WRITER'S ORDER. ``pull_requests[0]`` where GitHub +supplies it; otherwise the ``pr--`` parse off the ref, **gated on ``event == "merge_group"``**. +That gate is a security control, not a tidiness one: a branch name is chosen by whoever opened the +branch, and a fork cannot produce a ``merge_group`` event. A ref named ``pr-999-whatever`` on any +other event resolves to nothing here, exactly as it does in the workflow. + +A LABELLED PULL REQUEST WITH NO FAILING RUN IS REPORTED, NOT DROPPED. It reads ``UNATTRIBUTED``. The +common cause is benign -- the run aged out of the API window, or the label outlived the run it was +applied for -- but "I could not attribute this" must never render as "this is fine", which is the +defect class this whole signal chain exists to close. + +USAGE + python scripts/ci/report_ci_red.py # uses gh's auth + python scripts/ci/report_ci_red.py --repo owner/name + python scripts/ci/report_ci_red.py --warn-only # report, always exit 0 + python scripts/ci/report_ci_red.py \\ + --prs-json prs.json --runs-json runs.json # offline/testing + +EXIT + 0 nothing carries the label (or --warn-only) + 1 at least one pull request carries it -- there is a red to attribute + 2 the query itself failed; fail closed rather than report a clean repo +""" + +from __future__ import annotations + +import argparse +import json +import re +import subprocess +from dataclasses import dataclass +from pathlib import Path + +#: The label ``failure-signal.yml`` applies. One string, so the reader and any future writer edit +#: cannot silently disagree about which label is being talked about. +CI_RED_LABEL = "ci-red" + +#: Workflows whose failure earns the label. Mirrors ``failure-signal.yml``'s ``workflows:`` list -- +#: see the module docstring for why CLA Assistant is not in it. +_WATCHED = frozenset({"CI", "Security", "CodeQL", "backlog-hygiene"}) + +#: The only conclusion that is a red. Mirrors the writer's ``conclusion == 'failure'`` gate. +_RED = "failure" + +#: ``gh-readonly-queue//pr--``. Anchored on a path segment so a branch merely CONTAINING +#: the text (``feature/pr-12-notes``) cannot match -- and read only for a ``merge_group`` run anyway. +_MERGE_QUEUE_REF = re.compile(r"(?:\A|/)pr-(\d+)-[0-9a-f]+\Z") + +#: The pull-request fields this reader needs. Beside the parser so the two cannot drift. +PR_FIELDS = "number,title,state,headRefName" + + +@dataclass(frozen=True) +class Red: + """One pull request carrying the label, and the run it was earned by (if that is recoverable).""" + + number: int + title: str + run_name: str | None = None + run_event: str | None = None + run_url: str | None = None + created_at: str | None = None + + @property + def attributed(self) -> bool: + return self.run_name is not None + + @property + def hidden_from_the_pr_page(self) -> bool: + """True when the run is one the pull request's own checks list does not show. + + This is the whole point of the report. A ``merge_group`` run tests the branch MERGED WITH the + base, which is not the head the PR page reports on, so the page can read fully green while + this is the thing blocking the merge. + """ + return self.run_event == "merge_group" + + def line(self) -> str: + # ASCII only: this lands in operator consoles whose code page is cp1252, where a non-ASCII + # dash renders as a replacement character. + if not self.attributed: + return ( + f"#{self.number} {self.title[:60]} -- UNATTRIBUTED: no failing run for this pull " + f"request in the window queried (aged out, or the label outlived its run)" + ) + where = ( + " [merge_group -- NOT VISIBLE ON THE PR PAGE]" if self.hidden_from_the_pr_page else "" + ) + return f"#{self.number} {self.title[:60]} -- {self.run_name} failed{where} {self.run_url}" + + +def _pr_for_run(run: dict[str, object]) -> int | None: + """The pull request a run belongs to, by the writer's rule in the writer's order. + + Returns ``None`` rather than guessing. In particular a ``pr--`` ref on any event other than + ``merge_group`` resolves to ``None``: that ref is only trustworthy because a fork cannot raise a + ``merge_group`` event, and dropping the gate would let a branch name anybody can choose steer the + attribution. + """ + supplied = run.get("pull_requests") + if isinstance(supplied, list) and supplied: + first = supplied[0] + if isinstance(first, dict) and isinstance(first.get("number"), int): + return int(first["number"]) + if str(run.get("event") or "") != "merge_group": + return None + found = _MERGE_QUEUE_REF.search(str(run.get("head_branch") or "")) + return int(found.group(1)) if found else None + + +def attribute(prs: list[dict[str, object]], runs: list[dict[str, object]]) -> list[Red]: + """Join labelled pull requests to the newest failing run of a watched workflow. + + Pure: no network, no git. The CLI supplies both payloads so tests drive THIS function rather than + a re-implementation of the rule -- a test asserting a copy of the rule proves nothing about the + rule. Ordering is newest-run-first by ``created_at``; a run with no timestamp sorts last rather + than being dropped. + """ + newest: dict[int, dict[str, object]] = {} + for run in runs: + if not isinstance(run, dict): + continue + if str(run.get("name") or "") not in _WATCHED: + continue + if str(run.get("conclusion") or "").lower() != _RED: + continue + number = _pr_for_run(run) + if number is None: + continue + stamp = str(run.get("created_at") or "") + held = newest.get(number) + if held is None or stamp > str(held.get("created_at") or ""): + newest[number] = run + + found: list[Red] = [] + for pr in prs: + if not isinstance(pr, dict): + continue + raw = pr.get("number") + # Narrow rather than coerce: a surprising payload must become a finding, never a crash. + number = raw if isinstance(raw, int) else 0 + run = newest.get(number) + found.append( + Red( + number=number, + title=str(pr.get("title") or ""), + run_name=str(run.get("name") or "") if run else None, + run_event=str(run.get("event") or "") if run else None, + run_url=str(run.get("html_url") or "") if run else None, + created_at=str(run.get("created_at") or "") if run else None, + ) + ) + return sorted(found, key=lambda r: r.number, reverse=True) + + +def _gh(cmd: list[str]) -> object: + # B603: fixed argv, no shell. The only variable element is --repo, an operator-typed CLI argument. + # Same posture as check_stalled_prs.py; see the note there. + out = subprocess.run( # noqa: S603 # nosec B603 - fixed argv, no shell, operator-supplied repo + cmd, capture_output=True, text=True, timeout=180 + ) + if out.returncode != 0: + raise RuntimeError( + f"{cmd[0]} {cmd[1]} failed ({out.returncode}): {out.stderr.strip()[:400]}" + ) + return json.loads(out.stdout) + + +def _fetch_prs(repo: str | None) -> list[dict[str, object]]: + cmd = ["gh", "pr", "list", "--label", CI_RED_LABEL, "--state", "open"] + cmd += ["--limit", "100", "--json", PR_FIELDS] + if repo: + cmd += ["--repo", repo] + payload = _gh(cmd) + return [p for p in payload if isinstance(p, dict)] if isinstance(payload, list) else [] + + +def _fetch_runs(repo: str | None) -> list[dict[str, object]]: + slug = repo or ":owner/:repo" + # per_page=100 deliberately: any gh api list route DEFAULTS TO 30, and a reader that silently + # cannot see two thirds of its own corpus reports a clean repo. (BACKLOG #1385's own notes record + # a session that concluded a label had never been re-applied off exactly that truncation.) + cmd = ["gh", "api", f"repos/{slug}/actions/runs?status=failure&per_page=100"] + payload = _gh(cmd) + if not isinstance(payload, dict): + return [] + runs = payload.get("workflow_runs") + return [r for r in runs if isinstance(r, dict)] if isinstance(runs, list) else [] + + +def main(argv: list[str] | None = None) -> int: + parser = argparse.ArgumentParser( + description=__doc__, formatter_class=argparse.RawDescriptionHelpFormatter + ) + parser.add_argument("--repo", default=None, help="owner/name; defaults to gh's current repo") + parser.add_argument("--prs-json", type=Path, default=None, help="a saved payload (testing)") + parser.add_argument("--runs-json", type=Path, default=None, help="a saved payload (testing)") + parser.add_argument( + "--warn-only", action="store_true", help="report and exit 0 rather than 1 on a finding" + ) + args = parser.parse_args(argv) + + try: + if args.prs_json is not None: + loaded = json.loads(args.prs_json.read_text(encoding="utf-8")) + prs = [p for p in loaded if isinstance(p, dict)] if isinstance(loaded, list) else [] + else: + prs = _fetch_prs(args.repo) + if args.runs_json is not None: + loaded = json.loads(args.runs_json.read_text(encoding="utf-8")) + runs = [r for r in loaded if isinstance(r, dict)] if isinstance(loaded, list) else [] + else: + runs = _fetch_runs(args.repo) if prs else [] + except (RuntimeError, json.JSONDecodeError, subprocess.SubprocessError, OSError) as exc: + # FAIL CLOSED. "I could not ask" must never render as "nothing is red" -- that is this + # script's own defect class, one level up. + print(f"::error::could not read the {CI_RED_LABEL} state ({exc!r}). Treating as a FAILURE.") + return 2 + + # Liveness receipt: say what was EXAMINED. "nothing is red" and "the query returned nothing" + # are otherwise indistinguishable from the exit code alone. + # "run(s)", not "failing run(s)": the live fetch asks for status=failure, but --runs-json takes + # whatever the caller supplies, and a receipt must not assert a property of its input it did not + # check. `attribute` applies the conclusion filter itself. + print( + f"ci-red: {len(prs)} open pull request(s) carry {CI_RED_LABEL}; " + f"scanned {len(runs)} run(s) for attribution" + ) + if not prs: + print("ci-red: no pull request is carrying a red.") + return 0 + + reds = attribute(prs, runs) + for red in reds: + print(f"::warning::{red.line()}") + + hidden = [r for r in reds if r.hidden_from_the_pr_page] + if hidden: + print( + f"::error::{len(hidden)} pull request(s) were reddened by a merge_group run. Their own " + "checks can read GREEN on the PR page: a merge-queue run tests the branch MERGED WITH the " + "base, which is not the head the page reports on. Read the run linked above, not the PR's " + "check list -- re-queueing without reading it spends a full CI cycle to learn nothing." + ) + return 0 if args.warn_only else 1 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/tests/test_api_request_timeout.py b/tests/test_api_request_timeout.py index 62d4641b3..ec414239b 100644 --- a/tests/test_api_request_timeout.py +++ b/tests/test_api_request_timeout.py @@ -28,10 +28,30 @@ TIMEOUT_STATE_ATTR, ) -#: Long enough that no scheduling hiccup finishes it inside the deadline, short enough that the RED -#: run is not a wait: the deadlines below are 0.05-0.2s. +#: Long enough that no scheduling hiccup finishes it inside either deadline below, short enough that +#: a RED run (middleware removed) is not a wait. _SLOW_SECONDS = 3.0 +#: The deadline for the arms asserting the bound FIRES. Tight is safe here because the margin runs the +#: forgiving way: the handler sleeps ``_SLOW_SECONDS``, so a stalled runner can only delay the refusal, +#: never turn it into a pass. 30x. +_TRIPS_DEADLINE = 0.1 + +#: The deadline for the arms asserting a prompt handler is UNTOUCHED. **The margin runs the other way +#: here and that is why this number is not ``_TRIPS_DEADLINE``.** A runner stall pushes a trivial +#: handler PAST the deadline and 503s it, so the value has to absorb the stall rather than merely beat +#: the handler's ~1ms of real work. These arms used to run at 0.1s, which is inside the range of a GC +#: pause or a scheduling gap on a shared 32-way-parallel runner -- a wall-clock assertion that fails +#: differently on each attempt, which is the shape BACKLOG #1385 exists to remove. +#: +#: **Raising it costs no discrimination, because tightness was never what these arms measured.** Their +#: job is to catch a middleware that refuses EVERYTHING -- without them, one that 503'd every request +#: would pass the bound arms above. A 5s deadline catches that identically. The claim that the +#: CONFIGURED value is the one in force is carried by the trips arms instead, and carried better: a +#: 3.0s handler under a 0.1s deadline reds if the middleware ever falls back to the shipped 120s +#: default, which no fast-path assertion at any deadline can see. +_PASSES_DEADLINE = 5.0 + def _app_with_a_slow_route(timeout_seconds: float | None) -> tuple[object, list[str]]: """The real app plus one deliberately slow route, and a list that records whether the handler @@ -64,7 +84,7 @@ def _client(app: object) -> TestClient: def test_a_slow_handler_is_refused_with_a_bounded_error() -> None: """The bound itself. Mutation: remove the `RequestTimeoutMiddleware` registration from `create_app`. Red: 200 with `{"status": "finished"}` after the handler ran to completion.""" - app, finished = _app_with_a_slow_route(0.1) + app, finished = _app_with_a_slow_route(_TRIPS_DEADLINE) with _client(app) as client: response = client.get("/_test/slow") assert response.status_code == 503, ( @@ -79,7 +99,7 @@ def test_the_refusal_carries_the_baseline_security_headers() -> None: it did not set them itself the 503 would be the one response in the API with none of them. Mutation: drop `_TIMEOUT_HEADERS`. Red: the missing header is named.""" - app, _ = _app_with_a_slow_route(0.1) + app, _ = _app_with_a_slow_route(_TRIPS_DEADLINE) with _client(app) as client: response = client.get("/_test/slow") assert response.status_code == 503 @@ -93,10 +113,17 @@ def test_the_refusal_carries_the_baseline_security_headers() -> None: def test_a_fast_handler_is_untouched() -> None: - """Live positive control: with the SAME deadline in force, a handler that answers promptly - returns its own response. Without this, a middleware that 503'd everything would pass the bound - test above.""" - app, finished = _app_with_a_slow_route(0.1) + """Live positive control: with a deadline in force, a handler that answers promptly returns its + OWN response. Without this, a middleware that 503'd everything would pass the bound test above. + + Read `_PASSES_DEADLINE` for why the number is 5.0 and not the 0.1 the trips arms use. The three + assertions are separate claims, not one restated: the status says it was not refused, the body says + the response is the handler's rather than the middleware's, and `finished` says the handler ran to + completion rather than being cancelled and answered for. + + Mutation: make `RequestTimeoutMiddleware.__call__` refuse unconditionally (`send` the 503 before + `await self.app(...)`). Red: 503, not 200.""" + app, finished = _app_with_a_slow_route(_PASSES_DEADLINE) with _client(app) as client: response = client.get("/_test/fast") assert response.status_code == 200 @@ -111,7 +138,7 @@ def test_the_deadline_is_on_the_route_not_only_on_unauthenticated_paths() -> Non `/status` requires auth and, with no auth service attached, fails closed. What is asserted here is only that a real route still answers under a deadline in force: a middleware that swallowed or delayed authenticated routes would show up as a 503 instead of the fail-closed status.""" - app, _ = _app_with_a_slow_route(5.0) + app, _ = _app_with_a_slow_route(_PASSES_DEADLINE) with _client(app) as client: assert client.get("/health").status_code == 200 assert client.get("/status").status_code in (401, 403, 503) @@ -119,12 +146,25 @@ def test_the_deadline_is_on_the_route_not_only_on_unauthenticated_paths() -> Non def test_a_disabled_deadline_lets_a_slow_handler_finish() -> None: """`<= 0` disables the deadline. This is the escape hatch a deployment with a genuinely long - admin operation uses, and it must actually disable rather than clamp to some floor.""" + admin operation uses, and it must actually disable rather than clamp to some floor. + + **This asked the FAST route until BACKLOG #1385, which is not what the name claims and could not + fail for the stated reason** -- a trivial handler returns 200 whether the deadline is disabled, + enabled, or clamped to any floor above a millisecond, so the arm passed identically with the + control it exists to test switched on. It now asks the SLOW route, which is the only request that + can tell "disabled" from "clamped": `_SLOW_SECONDS` exceeds every deadline in this file, so a 200 + here means no deadline ran at all. + + Mutation: clamp a `<= 0` override up to `_TRIPS_DEADLINE` instead of passing through. Red: 503 with + `finished == []`. Under the old fast-route version that same mutation stayed GREEN.""" app, finished = _app_with_a_slow_route(0.0) with _client(app) as client: - response = client.get("/_test/fast") - assert response.status_code == 200 - assert finished == ["fast"] + response = client.get("/_test/slow") + assert response.status_code == 200, ( + f"a disabled deadline still refused the handler with {response.status_code}" + ) + assert response.json() == {"status": "finished"} + assert finished == ["slow"] def test_the_deadline_sits_inside_the_network_gate_and_outside_everything_else() -> None: diff --git a/tests/test_ci_red_reader.py b/tests/test_ci_red_reader.py new file mode 100644 index 000000000..ca045c278 --- /dev/null +++ b/tests/test_ci_red_reader.py @@ -0,0 +1,222 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""The ``ci-red`` reader: does the label get read back, and read back correctly (BACKLOG #1385)? + +``failure-signal.yml`` has written the label since PR #716. Nothing read it until +``scripts/ci/report_ci_red.py``, so these are that script's first tests. + +The rows below are shaped like real payloads because two of them ARE real: the merge-queue refs and +run names in :func:`test_the_real_pr_669_ejection_is_attributed` were read from this repository's +Actions API for PR 669, the ejection the backlog item was filed about. +""" + +from __future__ import annotations + +import importlib.util +import json +import sys +from pathlib import Path +from typing import Any + +import pytest + +_SCRIPT = Path(__file__).resolve().parents[1] / "scripts" / "ci" / "report_ci_red.py" + + +def _load() -> Any: + """Import the script by path -- ``scripts/`` is not a package, so a plain import cannot see it.""" + spec = importlib.util.spec_from_file_location("report_ci_red", _SCRIPT) + assert spec is not None and spec.loader is not None + module = importlib.util.module_from_spec(spec) + sys.modules["report_ci_red"] = module + spec.loader.exec_module(module) + return module + + +mod = _load() + + +def _run( + *, + name: str = "CI", + event: str = "merge_group", + conclusion: str = "failure", + branch: str = "gh-readonly-queue/main/pr-669-3760a93bfce37092b1add060dd6075c83cf4313a", + created: str = "2026-08-29T12:42:51Z", + pull_requests: list[dict[str, object]] | None = None, +) -> dict[str, object]: + return { + "name": name, + "event": event, + "conclusion": conclusion, + "head_branch": branch, + "created_at": created, + "html_url": "https://github.com/MEFORORG/MessageFoundry/actions/runs/33253197221", + "pull_requests": pull_requests if pull_requests is not None else [], + } + + +def _pr(number: int = 669, title: str = "fix(connscale): the FD probe re-walked") -> dict[str, Any]: + return {"number": number, "title": title, "state": "OPEN", "headRefName": "claude/x"} + + +# --- the defect the script exists for --------------------------------------- + + +def test_the_real_pr_669_ejection_is_attributed() -> None: + """The end-to-end claim, on the real refs. PR 669's required contexts were GREEN on the PR page; + it was ejected by a ``merge_group`` CI run the page never surfaced. The reader must name it and + must mark it as invisible on the page, because that mark is what stops a reader re-queueing.""" + reds = mod.attribute([_pr()], [_run()]) + assert len(reds) == 1 + assert reds[0].attributed + assert reds[0].hidden_from_the_pr_page + assert "merge_group -- NOT VISIBLE ON THE PR PAGE" in reds[0].line() + + +def test_an_ordinary_pull_request_run_is_not_flagged_as_hidden() -> None: + """The discriminating negative for the mark above. A ``pull_request`` run IS on the PR page, so + flagging it would train readers to ignore the mark that matters.""" + run = _run(event="pull_request", branch="claude/x", pull_requests=[{"number": 669}]) + reds = mod.attribute([_pr()], [run]) + assert reds[0].attributed + assert not reds[0].hidden_from_the_pr_page + + +# --- the security gate copied from the writer ------------------------------- + + +def test_a_spoofed_pr_ref_on_a_non_merge_group_event_resolves_to_nothing() -> None: + """A branch name is chosen by whoever opened the branch. The ``pr--`` parse is trustworthy + ONLY because a fork cannot raise a ``merge_group`` event, so the gate is a security control. + + Mutation: drop the ``event != "merge_group"`` check in ``_pr_for_run``. Red: this attributes a + fork's ``pr-669-deadbeef`` branch to pull request 669.""" + spoof = _run(event="pull_request", branch="pr-669-deadbeef", pull_requests=[]) + assert mod._pr_for_run(spoof) is None + assert not mod.attribute([_pr()], [spoof])[0].attributed + + +def test_the_merge_queue_ref_must_be_a_whole_path_segment() -> None: + """A branch merely CONTAINING the text is not a queue ref.""" + assert mod._pr_for_run(_run(branch="feature/pr-12-notes-and-things")) is None + assert mod._pr_for_run(_run(branch="gh-readonly-queue/main/pr-42-abc123")) == 42 + + +def test_the_supplied_pull_request_wins_over_the_ref_parse() -> None: + """The writer's order: ``pull_requests[0]`` is authoritative where GitHub supplies it.""" + run = _run(branch="gh-readonly-queue/main/pr-669-abc123", pull_requests=[{"number": 42}]) + assert mod._pr_for_run(run) == 42 + + +# --- the two filters that mirror failure-signal.yml ------------------------- + + +def test_a_cancelled_run_is_not_a_red() -> None: + """A merge-queue ejection CANCELS its siblings on the way out. Counting a cancellation would + misattribute every ejection to whichever sibling died first, and the writer does not label for + one either (``failure-signal.yml:51``). + + Mutation: accept ``cancelled`` in ``_RED``. Red: this attributes the cancelled run.""" + assert not mod.attribute([_pr()], [_run(conclusion="cancelled")])[0].attributed + + +def test_an_unwatched_workflow_is_not_a_red() -> None: + """CLA Assistant is excluded from the writer deliberately -- a CLA failure is the contributor's + to resolve. A reader that attributed it would report a cause the label was never applied for.""" + assert not mod.attribute([_pr()], [_run(name="CLA Assistant")])[0].attributed + + +# --- fail closed ------------------------------------------------------------ + + +def test_a_labelled_pull_request_with_no_failing_run_is_reported_not_dropped() -> None: + """ "I could not attribute this" must never render as "this is fine". The run aging out of the + API window is the common, benign cause -- and silently dropping the row would hide the label + entirely, which is the same blindness this script was written to end.""" + reds = mod.attribute([_pr()], []) + assert len(reds) == 1 + assert not reds[0].attributed + assert "UNATTRIBUTED" in reds[0].line() + + +def test_the_newest_failing_run_wins() -> None: + """Both of PR 669's ejections failed. The reader must name the LATER one -- the earlier is + already-acted-on history.""" + older = _run(created="2026-08-29T12:42:51Z") + newer = _run(created="2026-08-29T13:01:54Z", name="Security") + assert mod.attribute([_pr()], [older, newer])[0].run_name == "Security" + assert mod.attribute([_pr()], [newer, older])[0].run_name == "Security" + + +# --- the CLI contract ------------------------------------------------------- + + +def test_the_cli_exits_zero_and_says_so_when_nothing_carries_the_label( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + prs = tmp_path / "prs.json" + prs.write_text("[]", encoding="utf-8") + assert mod.main(["--prs-json", str(prs), "--runs-json", str(prs)]) == 0 + out = capsys.readouterr().out + assert "0 open pull request(s) carry ci-red" in out # the liveness receipt, not just the code + + +def test_the_cli_exits_one_and_names_the_hidden_run_when_a_pr_is_red( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + prs = tmp_path / "prs.json" + runs = tmp_path / "runs.json" + prs.write_text(json.dumps([_pr()]), encoding="utf-8") + runs.write_text(json.dumps([_run()]), encoding="utf-8") + assert mod.main(["--prs-json", str(prs), "--runs-json", str(runs)]) == 1 + out = capsys.readouterr().out + assert "#669" in out + assert "NOT VISIBLE ON THE PR PAGE" in out + + +def test_the_cli_fails_closed_when_the_payload_is_unreadable( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + """Exit 2, distinct from both 0 and 1. A query that could not run must not read as a clean repo.""" + bad = tmp_path / "bad.json" + bad.write_text("{not json", encoding="utf-8") + assert mod.main(["--prs-json", str(bad)]) == 2 + assert "Treating as a FAILURE" in capsys.readouterr().out + + +def test_warn_only_downgrades_the_finding_but_still_reports_it( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + prs = tmp_path / "prs.json" + runs = tmp_path / "runs.json" + prs.write_text(json.dumps([_pr()]), encoding="utf-8") + runs.write_text(json.dumps([_run()]), encoding="utf-8") + assert mod.main(["--prs-json", str(prs), "--runs-json", str(runs), "--warn-only"]) == 0 + assert "#669" in capsys.readouterr().out + + +# --- the reader must not drift from the writer ------------------------------ + + +def test_the_watched_workflows_match_the_writer() -> None: + """The reader classifies with two rules copied from ``failure-signal.yml``. If the workflow's + list moves and this does not, the reader reports causes the label was never applied for. + + This reads the workflow rather than restating it, so the assertion cannot pass by agreeing with + a stale copy of the list.""" + workflow = ( + Path(__file__).resolve().parents[1] / ".github" / "workflows" / "failure-signal.yml" + ).read_text(encoding="utf-8") + declared = next(line for line in workflow.splitlines() if line.strip().startswith("workflows:")) + names = {n.strip() for n in declared.split("[", 1)[1].rstrip("]").split(",")} + assert names == set(mod._WATCHED), ( + f"failure-signal.yml watches {names}, the reader watches {set(mod._WATCHED)}" + ) + + +def test_the_label_the_reader_asks_for_is_the_one_the_writer_applies() -> None: + workflow = ( + Path(__file__).resolve().parents[1] / ".github" / "workflows" / "failure-signal.yml" + ).read_text(encoding="utf-8") + assert f"--add-label {mod.CI_RED_LABEL}" in workflow diff --git a/tests/tooling_manifest.txt b/tests/tooling_manifest.txt index bb5029a6f..2f1da2fde 100644 --- a/tests/tooling_manifest.txt +++ b/tests/tooling_manifest.txt @@ -44,6 +44,7 @@ tests/test_blanket_stage_guard.py tests/test_ci_docs_only_detector.py tests/test_ci_engine_step_excludes_webconsole.py tests/test_ci_leg_data_class.py +tests/test_ci_red_reader.py tests/test_ci_retry_native_crash.py tests/test_ci_step_margin.py tests/test_ci_tooling_gate.py