diff --git a/.gitea/workflows/review-verdict.yml b/.gitea/workflows/review-verdict.yml index ffd206401..dd32be129 100644 --- a/.gitea/workflows/review-verdict.yml +++ b/.gitea/workflows/review-verdict.yml @@ -1010,6 +1010,15 @@ jobs: # as absence. Only checked when page 1 looked clean — a race already found needs no # further evidence. Unreadable page 2 also counts as raced: this is the fail-toward- # SUCCESS path, so uncertainty must resolve to `pending`, never to leaving green. + # + # BE HONEST ABOUT THE COST: a repair here is STICKY, not a one-run stall. It writes + # `$REPAIR_DESC`, and the classification above refuses to grant an exemption over that + # sentinel and re-writes it as a fixed point on every later run — so a SPURIOUS repair + # (a head with more status rows than the page cap and no actual race) removes that head's + # exemption permanently, and only a human `post-review-verdict.sh` clears it. That is + # still the right direction, because the alternative is a forged green over a human + # rejection. But it is a per-sha loss of the exemption, not an inconvenience, which is + # the argument for replacing this with real paging (#763) rather than living with it. if [ "$raced" = "0" ]; then more_hist=$(gh "$BASE_URL/repos/$REPO/statuses/$SHA?limit=100&page=2") || more_hist="UNREADABLE" if [ "$more_hist" = "UNREADABLE" ]; then diff --git a/scripts/tests/test_pr_changed_files.py b/scripts/tests/test_pr_changed_files.py index eeec10e6b..eed2be30d 100644 --- a/scripts/tests/test_pr_changed_files.py +++ b/scripts/tests/test_pr_changed_files.py @@ -970,10 +970,19 @@ if "/timeline" in url: if "/statuses/" in url: mode = os.environ.get("STUB_HISTORY_MODE", "none") - # PAGE, honoured, and BEFORE the read counters — same reason as the combined endpoint (#751). The - # post-write check now reads page 2 to decide whether page 1 was the whole list, and the - # `*-after-post` modes key on how many times the job has LOOKED at the history; the page-2 probe is - # part of the same look. Letting it bump those counters would shift "raced row appears on read N". + # PAGE, honoured. THE REASON IS THE TERMINATOR, NOT THE COUNTERS — and that distinction was itself + # a finding (#751 round 5). An earlier version of this comment said the guard sits ahead of the + # read-counting modes "so the page-2 probe cannot shift 'raced row appears on read N'", by analogy + # with the combined endpoint. Measured: moving this guard AFTER the counter modes reddens NOTHING, + # because no history mode that counts reads ever issues a page-2 request — `raced=1` on page 1 + # short-circuits the probe. So that half of the rationale read as a checked reason and was not, + # which is the precise class this branch exists to retire. + # + # What IS load-bearing: page 2 must terminate for every mode that describes page 1 only, or the job + # re-reads page 1's rows as a second page and concludes the list is longer than it is. Dropping + # just the `print("[]")` below reddens `test_a_PRE_EXISTING_human_row_does_NOT_trigger_a_repair`. + # (The ordering guard on the COMBINED endpoint is a different story and genuinely is counter- + # related — moving that one reddens three mid-run-race tests.) # # PAST THE END THIS ENDPOINT RETURNS `[]`, NOT `null` — measured at 1.27.1: # `/statuses/{sha}?limit=100&page=99` is the two bytes `[]`, while `/commits/{sha}/status` past the @@ -2363,13 +2372,22 @@ def test_the_verdict_workflow_has_NO_expression_delimiter_in_any_run_body(): # classifier is ~700 lines; a walk that returned nothing, or only the short steps, is the failure # to catch. Both are asserted on content, which no spelling change can spoof. bodies = [s["run"] for _, s in _iter_workflow_steps(WORKFLOW) if s.get("run")] - # NOT `>= 3`. The file has exactly three `run:` bodies today, so that bound had zero slack: cold - # review deleted the optional jq-preflight step — a legitimate simplification — and this redded - # with a message claiming the classifier had not been examined, which was untrue. The length - # assertion below already carries the whole property, so the count only needs to be non-empty. - assert bodies, ( - "the YAML walk reached NO run: bodies in review-verdict.yml, so the assertion above examined " - "nothing and a green here proves nothing") + # NOT `>= 3`, and not merely non-empty either. `>= 3` had zero slack — cold review deleted the + # optional jq-preflight step, a legitimate simplification, and this redded claiming the classifier + # had not been examined, which was untrue. But relaxing it to `assert bodies` threw away the only + # check that the walk reached ALL run-bearing steps: `max(len) > 5000` proves it reached the + # classifier and nothing about the short ones, so a helper that silently stopped yielding them + # would let an unscanned delimiter through. + # + # So count against the job's own step list, read here rather than through the helper under test. + # That catches the failure the count existed for (a helper looking at the wrong key, or dropping + # steps) without breaking when a step is legitimately added or removed. + import yaml as _yaml + _steps = (_yaml.safe_load(WORKFLOW.read_text())["jobs"]["set-verdict-status"]["steps"] or []) + declared = sum(1 for s in _steps if isinstance(s, dict) and s.get("run")) + assert len(bodies) == declared, ( + f"the YAML walk reached {len(bodies)} run: bodies but the job declares {declared} — the " + "assertion above did not examine every body, so a green here proves nothing") assert max(len(b) for b in bodies) > 5000, ( "the YAML walk did not reach a substantial run: body — the ~700-line classifier is the one " "that must be scanned, so this test would be vacuous") @@ -2623,7 +2641,11 @@ def test_the_dropped_step_guard_BEHAVIOURALLY_fails_without_the_marker_and_passe # assertion makes the intent explicit and fails loudly rather than subtly. body_before = "\n".join(ln for ln in prologue.splitlines() if not ln.lstrip().startswith("#")) - assert not re.search(r"^\s*(\w+\s*\(\)|function\s+\w+)\s*\{", body_before, re.M), ( + # Covers all four bash spellings: `mk() {`, `mk(){`, `function mk {`, `function mk() {`. The third + # was added when review found the first regex missed it, and the FOURTH still slipped that fix — + # the union form is the natural next spelling once `function mk {` is caught. Budget three rounds + # for any string-matching predicate. + assert not re.search(r"^\s*(function\s+)?\w+\s*(\(\s*\))?\s*\{", body_before, re.M), ( "a function is defined before the marker write, so the write may be inside it and unreached " f"in production while this test still passes:\n{body_before}")