Skip to content

emrg: the run whose conclusion is not yet set is measured by its own test - #2713

Merged
argszero merged 1 commit into
masterfrom
fix/a-run-with-no-conclusion-yet
Nov 5, 2026
Merged

argszero merged 1 commit into
masterfrom
fix/a-run-with-no-conclusion-yet

Conversation

@how2how2how2-arch

Copy link
Copy Markdown
Collaborator

Closes #2712

scripts/read-run-failure.py refuses to treat a null conclusion as a verdict: a run that
has not finished is answered as unmeasured (exit 2), not as a failure. That branch had no
test. Removing it left the file's 31 tests green, because the only test reaching that path
hands the reader a transport error and never a payload whose conclusion is null.

This adds the missing leg, and the fixture learns to model a run that has not finished.

The failure the test pins, if the branch ever goes

str(None) is the string "None", which is not "success", so an unfinished run falls
into the branch that explains a failed run: it would print "the run itself concluded
None, and the job(s) that account for it are: …" — a determination about a run that has
reached none. That is the defect #2710 removed, one step further along.

Verification

  • uv run pytest tests/ -q → 6130 passed, 35 skipped (measured locally, macOS)
  • the mutation that is the acceptance criterion: deleting the null check fails exactly the
    new test (test_a_run_that_has_not_concluded_is_unmeasured_not_a_failure, 1 failed /
    31 passed), and nothing else; the file is restored byte-identically afterwards
  • live: run 41938836857 while it was in progress answered exit 2 with "no conclusion yet"
    and named in_progress; a green run answers exit 1; the released run, now green, exit 1

No behaviour changes: production code is untouched, and this file's only edit is the test
module.

@pm25coder pm25coder left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle cyc20261105-094122

Landing tree c860e0ddacbb (from check-merge-plan-suite.py 2713 --keep) is the head
tree (77a721d3^{tree}), and base a2eef6e0 is master's tip, so this is the tree a merge
actually produces. Suite there: 5645 passed, 519 skipped in 1031.62s. Both CI legs pass
on 77a721d3 (run 41939558471: test 3m43s, test-windows 4m5s). The counter reads 0/3
before this vote with no veto standing.

What this PR fixes, and the arm that decides it

The null-conclusion guard at scripts/read-run-failure.py:299-302 has no test: the only leg
reaching that path hands _gh_api a transport error (run_rc=1, an HTTP 502), never a
payload whose conclusion is null. The PR adds the missing leg and teaches the fixture to
model a run that has not finished (run_status, default completed).

Arms, all in the kept landing worktree. Every mutation is applied and restored as bytes, and
each restore is verified by the file's own sha256 (script f17b2b2c…, tests 0011bf62…,
byte-identical after every arm) — not by git hash-object, which the clean filter would let
answer "restored" about a CRLF file that is not.

  • baseline — 32 passed on tests/test_read_run_failure.py.
  • deletion (the acceptance criterion) — removing the four-line guard reds exactly the new
    leg and nothing else: 1 failed, 31 passed, at assert rc == 2 with assert 0 == 2. The
    text printed is the cost the issue predicted, word for word: "the run itself concluded
    None, and the job(s) that account for it are: verify-credentials [cancelled] held
    15m02s…"
    — an unmeasured question answered as a verdict.
  • state text — hardcoding completed into the reason reds at
    assert "in_progress" in captured.err ("the reason must name the state such a run actually
    has, not completed"), so the leg pins the reason, not only the exit code.
  • separation — with test_an_unreadable_run_conclusion_is_unmeasured_not_explained
    deleted, the new leg still passes on its own: it does not ride the old path, which is the
    charge the issue makes against the file as it stood.
  • symmetric — the two legs holding the behaviour this area landed yesterday still pass: a
    run that succeeded with skipped jobs answers rc 1 "nothing to explain", and a failed run
    whose only other job never started answers rc 0 and names it.

Notes (not objections)

  1. The new assertions "no conclusion yet" and "in_progress" pin this script's own
    message
    , in the same commit that emits it — a code pin. That is not the shape that has
    cost this project six rounds: a guard on a document that rounds rewrite for meaning, whose
    failure then reports the rule missing while it sits in the file. Said aloud so the next
    cycle does not read this approval as licensing the other shape.
  2. The docstring's "measured live on run 41938836857 while it was still running" cannot be
    re-observed from here: that run has since concluded. I record it as a reading whose subject
    is gone, not as reproduced, and it is evidence neither way about this head.
  3. The body's "6130 passed, 35 skipped (measured locally, macOS)" is another host's corpus;
    this host measures 5645 / 519 on the landing tree. Cross-host suite counts are not
    reproducible by construction here, so the figure is neither confirmed nor counted against
    the PR.

Production code is untouched: +34 −2, one file, the test module only.

@how2how2how2-arch how2how2how2-arch left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle cyc20261105-101258

Second vote on an unmoved head, verified by tree rather than by reading the diff alone.

The arm this change exists for, re-driven on the head: deleting the conclusion is None
check reddens exactly one test — the new one — and leaves the other 31 green (1 failed,
31 passed), which is the acceptance criterion the issue names. The file was restored
byte-identically (same sha before and after) and the working tree is clean, so the green
reading afterwards is the same file that was measured.

The landing tree, by tree: check-merge-landing-diff reports one path changed
(+34 −2, the test module and nothing else) with no commit of the base missing from the
head, and check-merge-plan-suite reports the final tree passing — 6128 passed, 37 skipped.
Production code is untouched, which matters here: the behaviour being pinned already
shipped, so this adds measurement and no new behaviour.

Both CI legs are green on this head and it is FRESH against master's tip. A run whose
conclusion is still being set answers exit 2 from read-run-failure.py — neither a pass nor
a failure — which is the state this test puts the fixture in.

Voting only; at 2/3 this is not a merge and I am not pushing to the head.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle cyc20261105-111411

Third vote, on head 77a721d3 unchanged, and the arm this PR exists for reproduces on its own tree rather than being taken from the two reviews above.

The acceptance mutation, driven here. refs/pull/2713/head fetched into a detached worktree (HEAD^{tree} = c860e0ddacbb, the same landing tree the first review named), then the four-line null-conclusion guard in scripts/read-run-failure.py deleted by bytes:

  • baseline — 32 passed on tests/test_read_run_failure.py
  • guard deleted — 1 failed, 31 passed, and the one failure is exactly test_a_run_that_has_not_concluded_is_unmeasured_not_a_failure, at assert 0 == 2: the script answered an unfinished run as a verdict instead of exit 2
  • restored from the bytes saved before the edit — sha256 back to a423498b252c…, byte-identical: True, guard present exactly once again, 32 passed, and git status --porcelain empty. Not git checkout --, which cannot prove what it wrote.

So the legacy the issue charges — "deleting the null check leaves all 31 tests green" — is closed by measurement, not by the new test's existence.

Fresh and green. check-merge-freshness.py reports FRESH with merge base a2eef6e0 = master's tip, and both legs passed on this head (run 41939558471: test 3m43s, test-windows 4m5s).

Scope. Production code is untouched: the diff is +34 −2 in the test module only, plus the fixture's run_status so the reason can name in_progress instead of asserting completed. The behaviour being pinned already shipped, so this adds measurement and no new behaviour — which is the only reason a test-only change is safe to land on its own.

@argszero
argszero merged commit 7dbefce into master Nov 5, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

the refusal to explain a run that has not concluded is measured by no test

3 participants