diff --git a/.github/workflows/nightly-notice.yml b/.github/workflows/nightly-notice.yml index 9828a20f..abbd017e 100644 --- a/.github/workflows/nightly-notice.yml +++ b/.github/workflows/nightly-notice.yml @@ -20,14 +20,23 @@ name: Nightly failure notice # SCOPE: schedule-only, deliberately. A push/PR failure is already visible on the PR itself; alerting # there would be pure noise, and `workflow_run` fires for every CI completion regardless of trigger. # -# TWO WORKFLOWS, ONE ISSUE PER WORKFLOW. `Security` was added because its daily cron carries jobs that -# do not run on a PR -- `released-line-audit` most of all, whose whole subject (the LATEST RELEASE) no -# PR can change. The issue title is DERIVED from the completed workflow's name, so a green nightly CI -# cannot close an issue opened by a red Security run: a single shared title would let one signal close -# the other, which is the same silence this workflow exists to end. +# THREE WORKFLOWS, ONE ISSUE PER WORKFLOW. `Security` was added because its daily cron carries jobs +# that do not run on a PR -- `released-line-audit` most of all, whose whole subject (the LATEST +# RELEASE) no PR can change. `DAST` was added for a stronger version of the same reason (BACKLOG +# #318): it has NO `pull_request` trigger AT ALL, deliberately, so before this every DAST finding +# surfaced in the Actions tab and nowhere else -- an authenticated authorization sweep reporting into +# the void, which is precisely the shape this workflow exists to end. +# +# A WATCHED WORKFLOW MUST HAVE A `schedule:` TRIGGER. The job below fires only when the completed +# run's event was `schedule`, so watching a workflow with no cron adds a name that can never match: +# dead config that reads as coverage. `tests/test_nightly_notice.py` pins this for every watched name. +# +# The issue title is DERIVED from the completed workflow's name, so a green nightly CI cannot close an +# issue opened by a red Security or DAST run: a single shared title would let one signal close +# another, which is the same silence this workflow exists to end. on: workflow_run: - workflows: ["CI", "Security"] + workflows: ["CI", "Security", "DAST"] types: [completed] # Read-only by default; the one job that writes escalates to `issues: write` and nothing else. @@ -93,7 +102,7 @@ jobs: exit 0 fi - BODY="Nightly (scheduled) CI failed. + BODY="Nightly (scheduled) $WF_NAME failed. - run: $RUN_URL - commit: \`$HEAD_SHA\` @@ -102,7 +111,8 @@ jobs: A scheduled run is not a PR context, so this failure appears nowhere else. For CI that is the server-DB store, load/throughput and service-smoke legs the three required \`test\` legs skip; for Security it is the daily dependency audits and the released-line audit, whose subject is a - published release that no pull request can change. + published release that no pull request can change; for DAST it is the entire workflow, which + carries no \`pull_request\` trigger by design. This issue is opened once and commented on each subsequent failure; it closes itself when a nightly goes green again." diff --git a/harness/load/connscale/report.py b/harness/load/connscale/report.py index 0c0a7c8e..b6bea82a 100644 --- a/harness/load/connscale/report.py +++ b/harness/load/connscale/report.py @@ -24,6 +24,8 @@ from harness._spreadsheet import SPREADSHEET_FORMULA_TRIGGERS, spreadsheet_safe if TYPE_CHECKING: + from collections.abc import Callable + from harness.load.connscale.compare import ( ClaimModeComparison, FuseModeComparison, @@ -244,6 +246,88 @@ def to_json_dict(self) -> dict[str, object]: } +#: Hard cap on rows a CI step summary may carry. An oversized ``$GITHUB_STEP_SUMMARY`` write is dropped +#: ENTIRELY rather than trimmed, so a large profile must lose rows instead of losing the whole surface. +#: Truncation is always stated in the rendered text -- never a silent cap. +_MAX_SUMMARY_ROWS = 200 + + +def lane_label(sweep_mode: str, claim_mode: str) -> str: + """The lane's name as every connscale reading spells it. + + ``per_lane`` is the default claim mode and stays unqualified, so a pre-existing SLO detail string + is unchanged. Defined once because the emitter and the SLO must agree: a reading filed under + ``fixed_per_conn`` that a failure reports as ``fixed_per_conn/pooled`` is two distributions. + """ + return sweep_mode if claim_mode == "per_lane" else f"{sweep_mode}/{claim_mode}" + + +@dataclass(frozen=True) +class MonotonicPair: + """One consecutive smaller-N-to-larger-N comparison inside a single lane. + + The lane is ``(sweep_mode, claim_mode)`` — grouping by ``sweep_mode`` alone would chain a pooled + reading onto a per_lane one, which BACKLOG #1101 records as wrong independently of whether a + shipped profile currently triggers it. + + This is the SINGLE definition of the pairing. :func:`monotonic_pairs` is read both by the SLO that + fails on an excursion and by the emitter that records every reading, so the numbers a passing run + reports and the numbers a failing run reports cannot drift apart. + """ + + label: str # ``sweep_mode``, or ``sweep_mode/claim_mode`` when the claim mode is not per_lane + count: int # the LARGER N — the reading being judged + value: float # the metric at that N + prior: float # the metric at the previous N in the same lane + tolerance_floor: float # the FRACTION, e.g. 0.75 — what the detail string prints after "*" + threshold: float # ``prior * tolerance_floor`` — the number ``value`` must actually beat + ok: bool + + +def monotonic_pairs( + records: list[ConnScaleRecord], + key: Callable[[ConnScaleRecord], float | int | None], + *, + tolerance: float, +) -> list[MonotonicPair]: + """Every consecutive comparison the loose monotonicity smoke makes, passing ones included. + + The SLO only ever needed the violations. An excursion-only record cannot establish the metric's + variance, because the sample is selected on having already left the band (BACKLOG #1211) — so this + returns the whole sequence and lets the caller decide which half it wants. + + Readings of ``None`` are skipped rather than failed, and a skipped reading does not become the + ``prior`` for the next N. + """ + by_lane: dict[tuple[str, str], list[ConnScaleRecord]] = {} + for r in records: + by_lane.setdefault((r.sweep_mode, r.claim_mode), []).append(r) + + floor = 1.0 - tolerance + pairs: list[MonotonicPair] = [] + for (mode, claim_mode), rs in by_lane.items(): + prev_val: float | None = None + for r in sorted(rs, key=lambda r: r.count): + val = key(r) + if val is None: + continue + v = float(val) + if prev_val is not None: + pairs.append( + MonotonicPair( + label=lane_label(mode, claim_mode), + count=r.count, + value=v, + prior=prev_val, + tolerance_floor=floor, + threshold=prev_val * floor, + ok=not (v < prev_val * floor), + ) + ) + prev_val = v + return pairs + + @dataclass(frozen=True) class ConnScaleReport: profile: str @@ -301,6 +385,74 @@ def to_json_dict(self) -> dict[str, object]: def to_json(self) -> str: return json.dumps(self.to_json_dict(), indent=2) + def render_readings_markdown( + self, + metric: str, + key: Callable[[ConnScaleRecord], float | int | None], + *, + tolerance: float, + context: dict[str, str] | None = None, + max_rows: int = _MAX_SUMMARY_ROWS, + ) -> str: + """Every reading of one monotonic metric as a markdown table — the passing ones included. + + BACKLOG #1211 needs the metric's true variance, and the SLO records a number only when it has + already left the band. A sample selected on having excursioned cannot measure the distribution + it excursioned from, so this renders the whole sequence on every run. + + Pure: returns the text and writes nothing. Row count is capped because an oversized + ``$GITHUB_STEP_SUMMARY`` write is dropped in full rather than trimmed; a truncation SAYS so. + """ + pair_by_row = { + (p.label, p.count): p for p in monotonic_pairs(self.records, key, tolerance=tolerance) + } + rows: list[str] = [] + dropped = 0 + for r in sorted(self.records, key=lambda r: (r.sweep_mode, r.claim_mode, r.count)): + val = key(r) + if val is None: + continue + if len(rows) >= max_rows: + dropped += 1 + continue + label = lane_label(r.sweep_mode, r.claim_mode) + pair = pair_by_row.get((label, r.count)) + if pair is None: + # First reading in its lane: a real sample, but nothing to compare it against. + rows.append(f"| {label} | {r.count} | {float(val):.4g} | | | | first in lane |") + else: + margin = pair.value - pair.threshold + verdict = "within band" if pair.ok else "OUTSIDE BAND" + rows.append( + f"| {label} | {r.count} | {pair.value:.4g} | {pair.prior:.4g} " + f"| {pair.threshold:.4g} | {margin:+.4g} | {verdict} |" + ) + + head = [f"### connscale {metric} readings"] + ctx = { + "profile": self.profile, + "db_backend": self.db_backend or "sqlite", + **(context or {}), + } + head.append("") + head.append(" | ".join(f"{k}: {v}" for k, v in ctx.items())) + head.append("") + head.append( + f"Recorded on every run, pass or fail (BACKLOG #1211). Band is prior * " + f"{1.0 - tolerance:.2f}; a positive margin is inside it." + ) + head.append("") + if not rows: + head.append(f"No {metric} reading was produced by this run.") + return "\n".join(head) + "\n" + head.append(f"| lane | N | {metric} | prior | band floor | margin | verdict |") + head.append("|---|---|---|---|---|---|---|") + out = head + rows + if dropped: + out.append("") + out.append(f"{dropped} further row(s) not shown: capped at {max_rows}.") + return "\n".join(out) + "\n" + def to_csv(self) -> str: """One row per (sweep_mode, N) step — for spreadsheet curve plotting.""" buf = io.StringIO() diff --git a/harness/load/connscale/runner.py b/harness/load/connscale/runner.py index d4ca4fb7..2320461b 100644 --- a/harness/load/connscale/runner.py +++ b/harness/load/connscale/runner.py @@ -51,6 +51,7 @@ ConnScaleReport, NoLoss, SloCheck, + monotonic_pairs, ) from harness.load.corpus import Corpus, build_corpus from harness.load.correlator import Correlator @@ -1206,31 +1207,19 @@ def _monotonic_slo( # type: ignore[no-untyped-def] timing-derived counters on noisy CI runners (mf-ci-test-flakes), so a small dip is jitter, not a regression. Fails only on a real drop (``v < prior * (1 - tolerance)``). Missing readings (None) are skipped, not failed.""" - ok = True - detail_parts: list[str] = [] - # Group by (sweep_mode, claim_mode), NOT sweep_mode alone (BACKLOG #1101). Chaining prev_val across - # claim modes compares per_lane against pooled, and compare.py:22-25 states pooled's empty-claim - # rate SHOULD be materially lower — so a correct engine would fail this the moment a profile set - # claim_modes = ["per_lane", "pooled"]. No shipped profile does, which is the only reason it has - # never fired; the grouping is wrong independently of that. - by_mode: dict[tuple[str, str], list[ConnScaleRecord]] = {} - for r in records: - by_mode.setdefault((r.sweep_mode, r.claim_mode), []).append(r) - floor = 1.0 - tolerance - for (mode, claim_mode), rs in by_mode.items(): - ordered = sorted(rs, key=lambda r: r.count) - prev_val: float | None = None - for r in ordered: - val = key(r) - if val is None: - continue - v = float(val) - if prev_val is not None and v < prev_val * floor: - ok = False - label = mode if claim_mode == "per_lane" else f"{mode}/{claim_mode}" - detail_parts.append( - f"{label}@N={r.count}: {v:.3g} < prior {prev_val:.3g} * {floor:.2f}" - ) - prev_val = v - observed = "monotonic" if ok else "; ".join(detail_parts) - return SloCheck(name, f"non-decreasing vs N (±{int(tolerance * 100)}% jitter)", observed, ok) + # The pairing itself — including the group-by-(sweep_mode, claim_mode) rule BACKLOG #1101 records + # as load-bearing — lives in `monotonic_pairs`. The emitter that records EVERY reading reads the + # same function, so a passing run and a failing run cannot describe the sequence differently. + pairs = monotonic_pairs(records, key, tolerance=tolerance) + breaches = [p for p in pairs if not p.ok] + observed = ( + "monotonic" + if not breaches + else "; ".join( + f"{p.label}@N={p.count}: {p.value:.3g} < prior {p.prior:.3g} * {p.tolerance_floor:.2f}" + for p in breaches + ) + ) + return SloCheck( + name, f"non-decreasing vs N (±{int(tolerance * 100)}% jitter)", observed, not breaches + ) diff --git a/tests/test_connscale_empty_claims_per_msg.py b/tests/test_connscale_empty_claims_per_msg.py index 3fae6487..ce280d1f 100644 --- a/tests/test_connscale_empty_claims_per_msg.py +++ b/tests/test_connscale_empty_claims_per_msg.py @@ -21,8 +21,19 @@ import pytest -from harness.load.connscale.report import ConnScaleRecord, NoLoss -from harness.load.connscale.runner import _empty_claims_per_msg, _monotonic_slo +from harness.load.connscale.report import ( + ConnScaleRecord, + ConnScaleReport, + NoLoss, + lane_label, + monotonic_pairs, +) +from harness.load.connscale.runner import ( + _MONOTONIC_TOLERANCE, + _empty_claims_per_msg, + _monotonic_slo, +) +from tests.test_connscale_smoke import _append_step_summary, _record_ratio_readings def _rec( @@ -184,3 +195,210 @@ def test_a_regression_inside_one_claim_mode_is_still_caught() -> None: check = _monotonic_slo("empty_claims_monotonic", records, lambda r: r.empty_claims_per_msg) assert not check.ok assert "N=24" in str(check.observed) + + +# -------------------------------------------------------------------------------------------------- +# BACKLOG #1211: the readings must survive a PASSING run. +# +# The SLO writes a number into `observed` only once a reading has already left its band -- a passing +# run records the literal string "monotonic" and discards every value. So the only samples that ever +# survived were the excursions, and a sample selected on having excursioned cannot measure the +# distribution it excursioned from. #1211 requires that variance before anyone may touch the band, so +# these pin that every run records every reading. +# +# NOTHING HERE WIDENS THE BAND. That is limb two, and it stays blocked until the samples exist. +# -------------------------------------------------------------------------------------------------- + +_METRIC = "empty_claims_per_msg" +_KEY = lambda r: r.empty_claims_per_msg # noqa: E731 + + +def _report(*records: ConnScaleRecord) -> ConnScaleReport: + return ConnScaleReport( + profile="smoke", + engine_url="http://127.0.0.1:0", + db_backend=None, + shim_installed=True, + records=list(records), + slos=[], + result_ok=True, + exit_code=0, + ) + + +def _render(report: ConnScaleReport, **kw: object) -> str: + return report.render_readings_markdown(_METRIC, _KEY, tolerance=_MONOTONIC_TOLERANCE, **kw) + + +def test_a_passing_run_records_every_reading_not_only_the_excursions() -> None: + """The property #1211 exists for, asserted against a run whose SLO is GREEN. + + The healthy shape is asserted first, so this cannot pass by accidentally rendering a failure. + """ + recs = [_rec("fixed_per_conn", 12, per_msg=48.4), _rec("fixed_per_conn", 24, per_msg=60.0)] + assert _monotonic_slo("empty_claims_monotonic", recs, _KEY).ok, "the test lost its own premise" + + text = _render(_report(*recs)) + assert "48.4" in text and "60" in text, text + assert "OUTSIDE BAND" not in text + + +def test_the_slo_alone_would_have_recorded_neither_of_those_numbers() -> None: + """The other half of the pair: without this change a green run keeps no number at all. + + Stated as a test rather than as a claim in the item, because it is the entire justification for + emitting anything -- and it is the sort of premise that quietly stops being true. + """ + recs = [_rec("fixed_per_conn", 12, per_msg=48.4), _rec("fixed_per_conn", 24, per_msg=60.0)] + check = _monotonic_slo("empty_claims_monotonic", recs, _KEY) + assert check.observed == "monotonic" + assert "48.4" not in str(check.observed) and "60" not in str(check.observed) + + +def test_the_replayed_pr343_excursion_reproduces_the_items_own_arithmetic() -> None: + """BACKLOG #1211 records: ``fixed_per_conn@N=24: 36 < prior 48.4 * 0.75 (= 36.30) short by 0.30``. + + The floor and the margin are produced by the emitter here, not restated by hand, so a change to + either the tolerance or the arithmetic has to come back through this test. + """ + text = _render( + _report(_rec("fixed_per_conn", 12, per_msg=48.4), _rec("fixed_per_conn", 24, per_msg=36.0)) + ) + row = next(line for line in text.splitlines() if "| 24 |" in line) + assert "36.3" in row, row # the band floor, 48.4 * 0.75 + assert "-0.3" in row, row # short by 0.30 + assert "OUTSIDE BAND" in row, row + + +def test_the_emitted_band_tracks_the_slo_tolerance_rather_than_a_second_copy() -> None: + """A hard-coded 0.75 in the emitter would drift from the band the SLO actually enforces. + + Rendering the SAME records at two tolerances must move the floor, which is only true if the + emitter reads its tolerance rather than carrying its own. + """ + report = _report( + _rec("fixed_per_conn", 12, per_msg=100.0), _rec("fixed_per_conn", 24, per_msg=90.0) + ) + loose = report.render_readings_markdown(_METRIC, _KEY, tolerance=0.25) + tight = report.render_readings_markdown(_METRIC, _KEY, tolerance=0.05) + assert "| 75 |" in loose, loose # 100 * 0.75 + assert "| 95 |" in tight, tight # 100 * 0.95 + assert "within band" in loose and "OUTSIDE BAND" in tight + + +def test_the_emitted_lane_label_matches_the_slo_detail_string() -> None: + """A reading filed under ``fixed_per_conn`` that a failure reports as ``fixed_per_conn/pooled`` + is two distributions, not one. Both sides read ``lane_label``; this asserts they agree.""" + recs = [ + _rec("fixed_per_conn", 12, per_msg=40.0, claim_mode="pooled"), + _rec("fixed_per_conn", 24, per_msg=10.0, claim_mode="pooled"), + ] + detail = str(_monotonic_slo("empty_claims_monotonic", recs, _KEY).observed) + assert detail.startswith("fixed_per_conn/pooled@N=24"), detail + assert "| fixed_per_conn/pooled |" in _render(_report(*recs)) + assert lane_label("fixed_per_conn", "pooled") == "fixed_per_conn/pooled" + assert lane_label("fixed_per_conn", "per_lane") == "fixed_per_conn" + + +def test_undefined_readings_are_absent_rather_than_rendered_as_zero() -> None: + """``None`` means no messages were absorbed. A zero row would be a fabricated sample, and it + would drag any distribution built from these tables toward a value never measured.""" + text = _render( + _report( + _rec("fixed_per_conn", 12, per_msg=None), + _rec("fixed_per_conn", 24, per_msg=42.0), + ) + ) + assert "| 12 |" not in text, text + assert "| 24 |" in text + # The surviving reading is a lane HEAD: the skipped one never became its prior. + assert "first in lane" in text + + +def test_a_run_that_produced_no_reading_says_so_rather_than_rendering_an_empty_table() -> None: + text = _render(_report(_rec("fixed_per_conn", 12, per_msg=None))) + assert "No empty_claims_per_msg reading was produced" in text + assert "| lane |" not in text + + +def test_a_capped_table_states_what_it_dropped() -> None: + """An oversized step-summary write is dropped ENTIRELY, so a big profile must lose rows. It may + not lose them silently -- a truncated table that looks complete is worse than a short one.""" + recs = [_rec("fixed_per_conn", n, per_msg=float(n)) for n in range(12, 40)] + text = _render(_report(*recs), max_rows=5) + assert "capped at 5" in text + assert "23 further row(s) not shown" in text + assert len([line for line in text.splitlines() if line.startswith("| fixed_per_conn |")]) == 5 + + +# -------------------------------------------------------------------------------------------------- +# Writing it out. The renderer above is pure; these cover the one place that touches a file. +# -------------------------------------------------------------------------------------------------- + + +def test_the_step_summary_is_appended_never_truncated(tmp_path, monkeypatch) -> None: + """``$GITHUB_STEP_SUMMARY`` accumulates across every step of the job; overwriting it would eat + another step's output.""" + summary = tmp_path / "summary.md" + summary.write_text("## someone else's step\n", encoding="utf-8") + monkeypatch.setenv("GITHUB_STEP_SUMMARY", str(summary)) + + _append_step_summary("## mine\n") + _append_step_summary("## mine again\n") + + body = summary.read_text(encoding="utf-8") + assert body.startswith("## someone else's step") + assert body.count("## mine") == 2 + + +def test_the_recorder_writes_the_readings_through_to_the_summary(tmp_path, monkeypatch) -> None: + """End to end: a report in, the rows in the job summary, on a run whose SLO passes.""" + summary = tmp_path / "summary.md" + monkeypatch.setenv("GITHUB_STEP_SUMMARY", str(summary)) + monkeypatch.setenv("RUNNER_OS", "Windows") + monkeypatch.setenv("GITHUB_RUN_ID", "12345") + + _record_ratio_readings( + _report(_rec("fixed_per_conn", 12, per_msg=48.4), _rec("fixed_per_conn", 24, per_msg=60.0)) + ) + + body = summary.read_text(encoding="utf-8") + assert "48.4" in body and "60" in body + assert "runner_os: Windows" in body and "run_id: 12345" in body + assert "OUTSIDE BAND" not in body + + +def test_no_step_summary_env_falls_back_to_stderr_without_raising(capsys, monkeypatch) -> None: + """A local run has no job summary. It must not invent a file, and must not blow up.""" + monkeypatch.delenv("GITHUB_STEP_SUMMARY", raising=False) + _append_step_summary("readings go here\n") + assert "readings go here" in capsys.readouterr().err + + +def test_an_unwritable_summary_warns_rather_than_failing_the_run(tmp_path, monkeypatch) -> None: + """Turning a diagnostics failure into a red leg on an unrelated pull request is the disease + #1211 is treating. It must not be silent either: a dead emitter and a live one would render + identically, and the dead one would look like a clean run forever.""" + monkeypatch.setenv("GITHUB_STEP_SUMMARY", str(tmp_path / "no-such-dir" / "summary.md")) + with pytest.warns(UserWarning, match="could not record connscale readings"): + _append_step_summary("x\n") + + +def test_the_pairing_is_shared_rather_than_reimplemented() -> None: + """The SLO and the emitter must read ONE definition of the pairing. + + Asserted by driving `monotonic_pairs` directly and checking the SLO's detail string agrees with + the pair it reports -- if the SLO grew its own copy, this would diverge silently. + """ + recs = [_rec("fixed_per_conn", 12, per_msg=48.4), _rec("fixed_per_conn", 24, per_msg=36.0)] + pairs = monotonic_pairs(recs, _KEY, tolerance=_MONOTONIC_TOLERANCE) + assert len(pairs) == 1 + pair = pairs[0] + assert not pair.ok and pair.count == 24 and pair.prior == 48.4 + assert pair.threshold == pytest.approx(36.3) + + detail = str(_monotonic_slo("empty_claims_monotonic", recs, _KEY).observed) + assert detail == ( + f"{pair.label}@N={pair.count}: {pair.value:.3g} < prior {pair.prior:.3g} " + f"* {pair.tolerance_floor:.2f}" + ) diff --git a/tests/test_connscale_smoke.py b/tests/test_connscale_smoke.py index f9668e3f..4eec67ab 100644 --- a/tests/test_connscale_smoke.py +++ b/tests/test_connscale_smoke.py @@ -18,7 +18,9 @@ from __future__ import annotations +import os import sys +import warnings from collections.abc import Sequence import pytest @@ -26,7 +28,7 @@ from harness.load.connscale.probe import ProbeDegraded from harness.load.connscale.profile import load_connscale_profile_text from harness.load.connscale.report import ConnScaleRecord, ConnScaleReport -from harness.load.connscale.runner import run_connscale +from harness.load.connscale.runner import _MONOTONIC_TOLERANCE, run_connscale from tests._connscale_ports import ( INBOUND_PORT_HI, INBOUND_PORT_LO, @@ -176,6 +178,61 @@ def _assert_fd_probe(records: Sequence[ConnScaleRecord]) -> None: # --- the one expensive run, shared by every property below ---------------------------------------- +def _append_step_summary(text: str) -> None: + """Append to the job summary, which renders whether the job passed or failed. + + APPEND, never truncate: ``$GITHUB_STEP_SUMMARY`` accumulates across every step of the job, so + overwriting it would eat another step's output. + + A write that fails WARNS rather than raising. The reading is diagnostics, and turning a + diagnostics failure into a red leg on an unrelated pull request is the disease BACKLOG #1211 is + treating, not a cure for it. It is not swallowed either -- a silent emitter and a working one + would be indistinguishable. + """ + path = os.environ.get("GITHUB_STEP_SUMMARY") + if not path: + # Local runs have no job summary. stderr keeps the reading reachable without inventing a file. + sys.stderr.write(text) + return + try: + with open(path, "a", encoding="utf-8") as handle: + handle.write(text) + except OSError as exc: + warnings.warn( + f"could not record connscale readings to the step summary: {exc}", stacklevel=2 + ) + + +def _record_ratio_readings(report: ConnScaleReport) -> None: + """Persist every ``empty_claims_per_msg`` reading this run produced (BACKLOG #1211). + + The SLO records a number only once it has already left its band, so the only samples that ever + survived a run were the excursions -- and a sample selected on having excursioned cannot measure + the distribution it came from. #1211 needs the variance before anyone may touch the band, so the + readings are written here, from the FIXTURE, which runs before any assertion and therefore records + a passing run and a failing one alike. + + The tolerance is IMPORTED, not typed in. A second copy of 0.25 here would be a second definition + of the band, and the emitted floor could then drift away from the one the SLO actually enforces. + """ + context = { + # What a later reader needs to tell samples apart. #1211's whole question is whether the + # ratio moves with runner contention, so the core count is part of the reading, not trivia. + "runner_os": os.environ.get("RUNNER_OS", "local"), + "cpus": str(os.cpu_count()), + "run_id": os.environ.get("GITHUB_RUN_ID", "-"), + "sha": os.environ.get("GITHUB_SHA", "-")[:8] or "-", + } + _append_step_summary( + report.render_readings_markdown( + "empty_claims_per_msg", + lambda r: r.empty_claims_per_msg, + tolerance=_MONOTONIC_TOLERANCE, + context=context, + ) + ) + + @pytest.fixture(scope="module") async def smoke_report() -> ConnScaleReport: """ONE ``run_connscale`` sweep for the whole module (BACKLOG #1331). @@ -211,7 +268,7 @@ async def smoke_report() -> ConnScaleReport: # below the OS ephemeral floors, so the kernel cannot hand one out after it is probed. api_port, sink_port = reserve_api_and_sink_bases(profile, sink_ports=1) # type: ignore[arg-type] - return await run_connscale( + report = await run_connscale( profile, # type: ignore[arg-type] engine_api_port_base=api_port, sink_host="127.0.0.1", @@ -219,6 +276,9 @@ async def smoke_report() -> ConnScaleReport: sink_ports=1, install_executor_shim=True, ) + # In the FIXTURE, so the readings are recorded before any assertion can fail the module. + _record_ratio_readings(report) + return report # --- the properties, ONE NAME EACH ---------------------------------------------------------------- diff --git a/tests/test_nightly_notice.py b/tests/test_nightly_notice.py index 8c19fe15..d1a6f4b9 100644 --- a/tests/test_nightly_notice.py +++ b/tests/test_nightly_notice.py @@ -10,7 +10,7 @@ suites (exactly what the three required ``test`` legs SKIP) could break invisibly. ``.github/workflows/nightly-notice.yml`` turns that silence into one deduplicated issue. This module -pins the three ways it could quietly stop working. +pins the structural ways it could quietly stop working -- at least the ones checkable before it ships. WHAT CANNOT BE TESTED HERE, stated rather than papered over. A ``workflow_run`` workflow only triggers from the **default branch**, so this one cannot fire on the PR that adds it — its end-to-end behaviour @@ -70,6 +70,79 @@ def test_it_also_watches_the_security_workflow() -> None: ) +def test_it_also_watches_the_dast_workflow() -> None: + """DAST needs this more than either of the others (BACKLOG #318). + + ``dast.yml`` has NO ``pull_request`` trigger at all -- deliberately -- so before this widening a + genuine authorization finding surfaced in the Actions tab and nowhere else. An authenticated + security sweep reporting into the void is the exact shape this notice exists to end. + """ + watched = _on(_load(_NOTICE))["workflow_run"]["workflows"] + dast_name = _load(_WORKFLOWS / "dast.yml").get("name") + assert dast_name, "dast.yml has no `name:` -- workflow_run has nothing to key on" + assert dast_name in watched, ( + f"nightly-notice.yml watches {watched} but dast.yml is named {dast_name!r}. Its findings " + "would then reach nobody, which is the gap BACKLOG #318 recorded." + ) + + +def test_every_watched_workflow_exists_and_can_actually_fire() -> None: + """A watched name that no workflow answers to, or that has no cron, is dead config reading as + coverage. + + The notice job gates on ``workflow_run.event == 'schedule'``, so a watched workflow with no + ``schedule:`` trigger can never satisfy it -- the name sits in the list looking like protection + and matches nothing, forever, silently. That is the same failure the notice exists to fix, one + level up, so it is asserted for EVERY watched name rather than per workflow. + """ + watched = _on(_load(_NOTICE))["workflow_run"]["workflows"] + assert watched, "the watch list is empty" + + by_name: dict[str, Path] = {} + for path in sorted(_WORKFLOWS.glob("*.yml")): + name = _load(path).get("name") + if isinstance(name, str): + by_name.setdefault(name, path) + # Positive control: the scan must actually be reading workflows, or every assertion below would + # be vacuous against an empty map. + assert len(by_name) > 5, f"the workflow scan found only {len(by_name)} named files" + + for name in watched: + path = by_name.get(name) + assert path is not None, ( + f"nightly-notice.yml watches {name!r} but no workflow in {_WORKFLOWS.name}/ is named that. " + f"A workflow_run trigger matches on the NAME, so this entry can never fire. " + f"Names present: {sorted(by_name)}" + ) + triggers = _on(_load(path)) + assert "schedule" in triggers, ( + f"nightly-notice.yml watches {name!r} ({path.name}) but that workflow has no `schedule:` " + "trigger. The notice job only fires when the completed run's event was `schedule`, so " + "this entry can never match -- dead config that reads as coverage." + ) + + +def test_the_issue_body_names_the_workflow_that_failed() -> None: + """The TITLE was always derived from the completed workflow; the BODY was not. + + It opened with a hardcoded "CI failed" whatever had run, so a red Security run produced an issue + whose first line named the wrong workflow. Harmless-looking, and exactly the kind of thing a + reader uses to decide what broke. Widening the watch list to a third workflow made it worse + rather than introducing it. + """ + body = "\n".join( + str(s.get("run", "")) for s in _load(_NOTICE)["jobs"]["notice"]["steps"] if "run" in s + ) + assert body, "the notice job has no `run:` step to inspect" + assert "Nightly (scheduled) $WF_NAME failed." in body, ( + "the issue body does not name the workflow that actually failed. It must read from $WF_NAME, " + "the same value the title is derived from, or it will assert the wrong workflow broke." + ) + assert "Nightly (scheduled) CI failed." not in body, ( + "the body still hardcodes CI, so a Security or DAST failure opens an issue naming CI." + ) + + def test_it_only_reacts_to_scheduled_runs() -> None: """Without this gate every PR and push failure opens an issue.