From 20b117dabf0248d3c6b34d52fd6fa43956c0ea2e Mon Sep 17 00:00:00 2001 From: Timothy Date: Mon, 10 Aug 2026 21:08:10 +0200 Subject: [PATCH] =?UTF-8?q?fix(751):=20round-5=20findings=20=E2=80=94=20a?= =?UTF-8?q?=20rationale=20that=20was=20itself=20vacuous,=20and=20a=20third?= =?UTF-8?q?=20regex=20round?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fifth cold review: MERGEABLE, no Blocker, no High. Four Low findings, none behavioural. Fixing all four rather than accepting them, because two are the exact class this issue exists to retire: text that reads as a checked reason and is not. A VACUOUS RATIONALE, on the branch about vacuous rationales. The comment on the history stub's `page` guard said it 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 that 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. The real reason is the other half: page 2 must terminate for modes that describe page 1 only, and dropping just that `print("[]")` reddens `test_a_PRE_EXISTING_human_row_does_NOT_trigger_a_repair`. Comment now says which half is load-bearing and which was wrong. (The COMBINED endpoint's guard genuinely is counter-related — moving it reddens three mid-run-race tests.) THE FALSE REPAIR IS STICKY, and the previous commit undersold it as "a stall a reviewer can clear". It writes `$REPAIR_DESC`, which the classification refuses to grant an exemption over and re-writes as a fixed point on every later run — so a spurious repair removes that head's exemption PERMANENTLY, not for one run, and only a human verdict clears it. Still the right direction against a forged green over a rejection, but it is a per-sha loss of the exemption, and that is the argument for real paging (#763) rather than living with this. Said in the comment now. CORRECTING THE PREVIOUS COMMIT MESSAGE, which over-generalised: "uncertainty resolves to a stall … never to leaving green" is true of the page-2 probe and NOT of the enclosing path. An unreadable page 1, or a non-numeric high-water mark, still leaves the exemption `success` standing unverified. The workflow's own comments state that correctly; the message did not. THIRD ROUND ON ONE REGEX, which is the documented budget for a string-matching predicate. Assertion C started as `\w+\s*\(\)\s*\{`, gained `function\s+\w+` when review found `function mk {` slipped it, and STILL missed the union form `function mk() {` — the natural next spelling once the previous one is caught. Now `^\s*(function\s+)?\w+\s*(\(\s*\))?\s*\{`, verified against all seven spellings. THE COMPLETENESS COUNT, restored properly. Relaxing `len(bodies) >= 3` to `assert bodies` fixed a false red but 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 pass an unscanned delimiter. Now counted against the job's own step list, read directly rather than through the helper under test — which catches a helper reading the wrong key or dropping steps, while still tolerating a step being legitimately added or removed. Verification: 460 green. Three mutations, each as intended — the union spelling `function mk() {` (red, previously passed), a walk that drops the short steps (red, the property the count guard restores), and a legitimate step deletion (PASSES, confirming the false red it replaced stays fixed). Twenty-nine mutations across six rounds. Refs: #751 Co-Authored-By: Claude Opus 5 (1M context) --- .gitea/workflows/review-verdict.yml | 9 +++++ scripts/tests/test_pr_changed_files.py | 46 +++++++++++++++++++------- 2 files changed, 43 insertions(+), 12 deletions(-) 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}")