From 71e559ac6bfc381fddbf22bbbd6c09f80b33703e Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Tue, 25 Aug 2026 14:53:52 -0500 Subject: [PATCH 1/2] test(connscale): record every empty_claims_per_msg reading, not only the excursions (BACKLOG #1211) The `empty_claims_monotonic` 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 a run 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, and the readings to establish it were being produced on every CI run and thrown away. This records them. The emission sits in the module fixture, which runs before any assertion, so a passing run and a failing one are recorded alike. WHY THE STEP SUMMARY AND NOT A PRINT. A bare `print` in the smoke test does not work: pytest captures a PASSING test's stdout and shows it only on failure, which leaves the tail-only sampling exactly as it was. Measured on a throwaway repo -- the sample line was absent from a green run's output. `$GITHUB_STEP_SUMMARY` renders on pass and fail alike, is already the idiom in 19 workflow files here, and needs no ci.yml change, because CI runs connscale through pytest rather than through a step this could hang an upload on. NOTHING HERE WIDENS THE BAND. That is limb two and it stays blocked until the samples exist. No constant in this change is a judgement about what the band should be. The pairing is EXTRACTED rather than copied. `monotonic_pairs` and `lane_label` are now the single definition of how readings are grouped into lanes and compared, read by both the SLO that fails on an excursion and the emitter that records every reading -- so the numbers a green run reports and the numbers a red run reports cannot drift apart. The group-by-(sweep_mode, claim_mode) rule that BACKLOG #1101 records as load-bearing moves with it. `_monotonic_slo` keeps its exact detail string: a differential over 4,648 cases (exhaustive over orderings of None / zero / ties / the exact band boundary, plus 4,000 randomized multi-lane sets) found zero divergence from the old inline implementation, and the differential was shown able to fail against a deliberately broken variant. Verified end to end: a real passing `tests/test_connscale_smoke.py` run wrote four readings to a step summary and left a prior step's content intact. Seven mutants, all killed, each by a distinct set of tests: emit only the excursions; hard-code the band instead of reading the tolerance; drop the claim-mode qualification from the lane label; render an undefined reading as zero; cap the table silently; truncate the summary instead of appending; swallow a failed write. Every mutant asserted a unique anchor and a changed file hash before scoring, and every restore was byte-identical. Co-Authored-By: Claude Opus 5 --- harness/load/connscale/report.py | 152 +++++++++++++ harness/load/connscale/runner.py | 45 ++-- tests/test_connscale_empty_claims_per_msg.py | 222 ++++++++++++++++++- tests/test_connscale_smoke.py | 64 +++++- 4 files changed, 451 insertions(+), 32 deletions(-) diff --git a/harness/load/connscale/report.py b/harness/load/connscale/report.py index 0c0a7c8ef..b6bea82aa 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 d4ca4fb78..2320461b1 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 3fae64877..ce280d1f6 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 f9668e3f9..4eec67ab2 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 ---------------------------------------------------------------- From b19a40c43854e56d3af2a55ddc21ee88fddeb1ff Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Tue, 25 Aug 2026 15:02:59 -0500 Subject: [PATCH 2/2] ci(notice): a red DAST nightly notified nobody, and the issue body named the wrong workflow (BACKLOG #318) #318 carries this as a tracked follow-up rather than deferred scope: `dast.yml` has NO `pull_request` trigger, deliberately, and is not a required context, so every finding from the authenticated authorization sweep surfaced in the Actions tab and nowhere else. An authenticated security sweep reporting into the void is the exact shape `nightly-notice.yml` exists to end -- it just was not watching it. Widened `workflows:` to include DAST, which is the fix the item names. ALSO FIXED, because widening made it worse rather than introducing it: the issue BODY opened with a hardcoded "Nightly (scheduled) CI failed." whatever had actually run. The TITLE was already derived from $WF_NAME; the body was not, so a red Security run has been opening an issue whose first line names CI. With a third watched workflow that is a reader deciding what broke from a wrong sentence. It now reads from the same $WF_NAME the title does. NEW GUARD, one level up from the item. A watched name that no workflow answers to, or one whose workflow has no `schedule:` trigger, can never match: the notice job fires only when the completed run's event was `schedule`, so such an entry sits in the list looking like protection and matches nothing, forever, silently. That is the notice's own failure mode turned on itself. `test_every_watched_workflow_exists_and_can_actually_fire` asserts both arms for EVERY watched name, so a fourth workflow added later cannot be added wrongly. Its scan carries a positive control, because an empty name map would make every assertion in it vacuous. The module docstring claimed it "pins the three ways" the notice could stop working; there were already six tests. A count in prose has no checker and has to be maintained by whoever adds the next test, so it now states the kind rather than the number (SDS-3.6). Four mutants, all killed: drop DAST from the list; watch a name no workflow answers to; watch a real workflow that has no cron; restore the hardcoded CI in the body. Every mutant asserted a unique anchor and a changed file hash before scoring, and every restore was byte-identical. Two share a test by design -- they are its two arms -- and their failure messages were checked to differ, the typo case listing every name actually present. Scope: the nightly-notice widening only. Increment 2 of #318 (schema-driven breadth, the MLLP/TCP/X12 ingress fuzzing, the /ui plane, a TLS black-box target) is untouched. Co-Authored-By: Claude Opus 5 --- .github/workflows/nightly-notice.yml | 26 +++++++--- tests/test_nightly_notice.py | 75 +++++++++++++++++++++++++++- 2 files changed, 92 insertions(+), 9 deletions(-) diff --git a/.github/workflows/nightly-notice.yml b/.github/workflows/nightly-notice.yml index 9828a20fd..abbd017ec 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/tests/test_nightly_notice.py b/tests/test_nightly_notice.py index 8c19fe15f..d1a6f4b97 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.