Repository navigation
fix(ci): the night report judges only its declared jobs (#830) - #926
Conversation
) The report judged every completed job of the run, so a verdict depended on whether an auxiliary job beside it — streak, verdict — happened to finish first: the same night read red when the streak job was completed and green while it was still running. The script now takes the job names it judges in NIGHT_JUDGED_JOBS, the reusable workflow gains a judged-jobs input, and runtime-proof.yml passes the four measuring jobs. A judged name matching no job of the run is an error, not a green night. The population test drives three payloads of the same night (streak completed, streak running, verdict red) and a static guard fails any caller that neither waits for every other job nor names the ones it judges. Assisted-by: Muse (Muse Spark 1.3)
There was a problem hiding this comment.
Thanks for this. The core idea is right and the shell is careful: the matching is exact (no substring, no glob), the parentheses and comma in stacks (incus-ovn, last release) are inert, set -euo pipefail holds and nothing new swallows a status. The zero-jobs and truncated-page paths get louder with the judged list, not quieter.
Three things block merge. All three are the same family, and it is the one this repository is most strict about: a control that is written in a comment and not made true by the code.
1. The static guard checks for a string, not for a list.
tools/ci/nightly_reporting_test.go:177 is strings.Contains(section, "judged-jobs:"). Replace the block at runtime-proof.yml:1168-1172 with judged-jobs: '' and the guard passes, while night-report.sh:80 takes [ -n "${NIGHT_JUDGED_JOBS:-}" ] false and falls back to judging every completed job. The race of #830 is back with mise run check green over it. The guard needs to require a non-empty block, not the key.
2. The closure check does not exist, and the comment says it does.
night-report.sh:82-83 computes $names - [.[].name]: judged names absent from the run. Nothing computes the other direction. A job that exists, fails, and is not in the list is dropped at the narrowing and the night reads green; with --apply it also closes an open scheduled-red issue.
That is fine for a rename, which is what the check catches. It is not fine for the other half of the sentence at runtime-proof.yml:1165: "update this list when a measured job is renamed or its matrix gains a leg, or the script refuses the night rather than judging a narrowed population". Add a leg to the runtime matrix at line 73 and nothing refuses anything.
Either the comment stops claiming the check, or the check exists: run jobs, minus judged, minus a declared ignore list (streak, verdict, the report job), must be empty. The second is the shape used elsewhere here, and it makes a new job force a decision instead of vanishing.
3. The completed filter runs after the narrowing, so a judged job still running is dropped in silence.
night-report.sh:81-92: the missing check and the narrowing both run over all jobs, then select(.status == "completed") removes what is left unfinished. A judged job at in_progress therefore passes the check, survives the narrowing, is removed, and the script exits 0 green.
runtime-proof.yml cannot hit this today because its report job has needs: [runtime, stacks]. But the whole point of the new guard is to bless the other shape, "names the jobs it judges" instead of "waits for every other job". Under the shape the guard permits, this is #830 again with timing in place of renaming. After narrowing, a judged job that is not completed should be an error on the same footing as a missing one.
On the falsify note in the description. It does not hold here. On the maintainer's station, on the exact spec you named:
$ mise run falsify -- tools/falsify/specs/a-silent-night-is-refused.json
baseline: green
compiled=yes the test bit ...
compiled=yes the test bit ...
after restoration: green
EXIT=0
Same result with your verbatim python3 tools/falsify/falsify.py <spec>. The mechanism in your note is also not what happens: falsify.py passes the parent environment through (env = {**os.environ, ...}), and systemd-run --user --scope execs the command, so it inherits it. More likely your container has no systemd user session, in which case systemd-run --user exits non-zero with empty output and the harness reports it as "the copy is red before any mutation".
So please re-run the harness on a-red-night-opens-an-issue.json and paste the output, or report the actual error text rather than a cause. "All 5 mutations bite in a manual replay" is currently an assertion, and this repository treats an unrun harness as an unrun harness. For what it is worth, your fifth mutation looks well formed: it neutralises with && false rather than deleting a name, its find occurs exactly once, and it should redden the test it names through the race-night-completed payload.
Two gaps no mutation covers, worth adding while you are in there. The judged list travels three hops and only the third is tested: the Go tests re-declare the four names as a const (night_report_test.go:417) and never read either workflow. Comment out the NIGHT_JUDGED_JOBS: line in night-report.yml and every test stays green. Same for narrowing the list in runtime-proof.yml.
Smaller, non-blocking:
night-report.sh:85prints an unquoted name, so a trailing space or a CRLF producesjudged job(s) not found: incusnext to a job that looks identical. Quote them, or@json.night-report.sh:83usesjoin(", "), which is ambiguous forstacks (incus-ovn, last release).- A whitespace-only value diagnoses "has no completed job", which is the wrong reason.
nightly_reporting_test.go:134collects ids with[A-Za-z0-9_-]+, whilejobSection(drift_workflow_test.go:550) only terminates on^ [a-z][a-z-]*:\n. A job id with a digit or underscore would not close the previous section, and a neighbour'sjudged-jobs:could satisfy the check for the reporting job. Latent today, cheap to close by aligning the two.needs:as a YAML block sequence or quoted list is not parsed, so every other job counts as "beside". That fails conservatively, so it is safe, but it will surprise someone.- In
TestARedAuxiliaryJobBesideTheReportDoesNotDecideTheVerdict,race-night-runningis green with and without the fix: the pre-existingcompletedfilter already drops the streak job. It is a witness of the race, not a control on the fix, and the comment reads as if all three payloads bite. The other two do discriminate, so the test is sound; it is the sentence that overstates. Same for "differ in exactly one field", which is 22 lines. stacks (incus-ovn, last release)carriescontinue-on-error: ${{ matrix.control }}, so its conclusion issuccessby construction. Keeping it judged buys a rename alarm, not a verdict, which is slightly less than "the two jobs that measure" claims.
One separate point, not yours to fix in this PR but worth saying out loud: the judged list means streak and verdict reds are now reported by nothing at all, and runtime-proof.yml:1026 records six nights of streak exiting 127. That needs its own issue rather than living in a comment: it is #930, and it is not yours to fix here.
About the pre-existing failure you disclosed. You are right on all three counts, and thank you for writing it down rather than working around it. It is TestADroppedConnectionIsClaimedOnlyWhenItHappened, it fails 4 times out of 10 on unmodified main at 51612e6, and it has nothing to do with your change. It is tracked as #925 and fixed in #929. Once that lands, rebase and your CI should be clean.
No patch proposed: the work is yours. Happy to re-review as soon as you push.
…control - Static guard now requires a non-empty judged-jobs (and ignored-jobs) list, not just the key, so judged-jobs: '' no longer passes. - night-report.sh gains the closure check: every job in the run must be judged or declared ignored, else the night is refused. - A judged job that is not completed is now an error, not silently dropped by the completed filter. - Tests read the judged/ignored lists and the env wiring from the workflows instead of re-declaring them, and the guard parses block needs: sequences and quoted items. - Falsify spec gains mutations 6 and 7 for the two new checks. Assisted-by: Muse (Muse Spark 1.3)
|
Thanks for the review — all three blockers are one family (a control written in a comment that the code did not make true), and I fixed them as a family. Pushed as 1. Guard checks a string, not a list — fixed.
2. Closure check — now exists.
3. Judged job not completed — now an error. After narrowing, a judged job with status ≠ Wiring gaps — closed. The tests no longer re-declare the four names:
Non-blocking items — all applied. Missing-job names are quoted ( Falsify — you were right, and here is the actual error text. You were right about the mechanism: Exit 2. The real cause on this machine: there is no systemd user bus — the container runs as root with no user session ( So the harness cannot run here at all (its baseline copy is red before any mutation, for the same reason), and the manual replay below is the substitute: a driver applied each mutation to a fresh copy of the tree and ran its test — baseline green, all 7 mutations bite. Mutation 5 is well formed as you said. Verification on the final tree.
One thing intentionally not in this PR, as you noted: |
stephrobert
changed the title
|
The two reviewers who raised the blocking points have re-read your commit. All three BLOCKERs are closed, and one of your additions is a control neither of them asked for. What they verified, rather than took on trust:
Three things left. The first is the only one that is red. 1. No type. The pattern is Rather than send you round again for one line, the maintainer is rewriting that subject to The rebase was needed anyway: your branch merged an older 2. The pull request body still carries the statement you withdrew in a comment. Thank you for that comment: the diagnosis is honest and checkable, and the missing user bus in your container is the real cause. But the body is what becomes the squash message, so it is what 3. One residual of the same shape as the finding it answers, and it is yours to judge. judged-jobs: >-
ignored-jobs: |
Consecutive green scheduled runs
An unrecognised indicator should refuse, not accept. It is a small inversion, it is not blocking, and it is written here rather than left to be discovered. If you would rather not take it in this pull request, say so and it becomes its own issue. Two notes with no action attached. Nothing ties the lists to the One measurement is still owed and it is the maintainer's, not yours: running |
|
Correction to my previous comment, on one factual point. I wrote that the rebase brings your branch onto a What that changes for you: nothing you need to do. Your two new mutations name tests that exist, so the rule would pass either way. But the sentence was wrong and you should not plan around it. When #923 lands, the lint gains that rule, and your spec is already compatible with it. The rest of the previous comment stands: the three blocking points are closed, the subject rewrite and the rebase are being pushed to your branch, and the pull request body still carries two sentences to edit, the withdrawn |
- Static guard now requires a non-empty judged-jobs (and ignored-jobs) list, not just the key, so judged-jobs: '' no longer passes. - night-report.sh gains the closure check: every job in the run must be judged or declared ignored, else the night is refused. - A judged job that is not completed is now an error, not silently dropped by the completed filter. - Tests read the judged/ignored lists and the env wiring from the workflows instead of re-declaring them, and the guard parses block needs: sequences and quoted items. - Falsify spec gains mutations 6 and 7 for the two new checks. Assisted-by: Muse (Muse Spark 1.3)
06ae8be to
4893420
Compare
|
You fixed the subject yourself and One thing left that will cost you time otherwise. Your ten payload fixtures under The review itself stands: the three blocking points are closed, and the body still carries the two sentences to edit, the withdrawn |
|
Follow-up to my reply above: the |
…happen (#941) Ten payload fixtures under tools/ci/testdata/race-night-*/ landed on main without a final newline. The repository's own end-of-file-fixer hook rewrites them on every commit, sees files it modified, and exits 1. Every push from any clone where `pre-commit install` has been run therefore fails, on a diff that has nothing to do with the files it names. Measured three times today: once on the contributor's branch of #926, and twice on unrelated lots whose own files were all correct. The hook's output names the ten fixtures and ends `failed to push some refs`, so the symptom points at the wrong lot and costs the diagnosis each time. One byte per file, the closing brace followed by a newline, and the ten documents still parse. No test reads the trailing byte: `go test ./tools/ci/` is green before and after. Why CI never caught this, which is the part worth keeping: pre-commit runs in no workflow. It is a local hook only, deliberately, so a red over documentation freshness never teaches anybody --no-verify. The cost of that choice is exactly this: a file that the hook refuses can reach main through a green pull request and then block everybody's push until somebody reads the hook's output carefully enough to notice the named files are not theirs. Assisted-by: Claude Code (claude-opus-5)
Fixes #830.
Repro (on main 51612e6, before)
The same night — run 43353368876, every proof job green, the streak job red with no failing step — reports opposite verdicts depending only on the streak job's status: red when completed, green while it is still in_progress. The payload is driven through tools/ci/night-report.sh with a stubbed gh.
The fix, the declared-list way
The issue and the maintainer's comment agree the script should take the job names it judges:
tools/ci/night-report.shtakes the job names it judges inNIGHT_JUDGED_JOBS(one display name per line, as the jobs API reports them) and ignores every other job, whatever its status. Empty means the old behaviour: judge every completed job. A judged name matching no job of the run is an error, not a green night — a renamed job would otherwise narrow the verdict silently..github/workflows/night-report.ymlgains ajudged-jobsinput, wired to that variable..github/workflows/runtime-proof.ymlpasses the four measuring jobs:incus,incus-ovn,stacks (incus-ovn),stacks (incus-ovn, last release). The other five callers leave the input empty and are untouched.tools/ci/nightly_reporting_test.gonow fails if a job callingnight-report.ymlneither waits for every other job of its workflow nor names the ones it judges.Tests (tools/ci, all green)
TestARedAuxiliaryJobBesideTheReportDoesNotDecideTheVerdict— three payloads of the same night (streak completed, streak running, verdict red) all agree green with the judged list; without it the first two disagreed, which is the race.TestAJudgedListStillMeasuresTheJobsItNames— the control: incus failing on "Scaleway ssh suite" still reads red, naming only that step.TestAJudgedJobThatNeverRanIsAnErrorNotAGreenNight— a bogus judged name fails with the missing job named.go test ./tools/ci/ok,go test -race ./...ok (36 packages),mise run lint0 issues,mise run shapes:checkgreen.Falsify
a-red-night-opens-an-issue.jsongains mutations 5, 6 and 7: the judged-list application, the closure check and the completeness check, one each. All seven bite in a hand-written replay by the author, and the baseline is green.The real harness has not run on this change, and that is the one measurement this pull request does not carry.
python3 tools/falsify/falsify.py <spec>exits 2 in the author's container with "the copy is red before any mutation". The cause is a missing systemd user bus there:/run/user/0is absent andsystemctl --user showanswers "Failed to connect to bus: No medium found". An earlier draft of this section blamed a clean PATH without mise Go; the author retracted that in a comment on this pull request, and the maintainer measured the opposite on their own station, where the harness runs green on the spec that draft named.What the hand-written replay does not give, and the harness does: the compile and
go vetgate on each mutant, the distinction between a void verdict and a real one, the restore check, and the exactly-once fragment rule. The three mutations were instead verified statically against this commit's blobs: eachfindoccurs exactly once, no mutation drops an identifier, both new mutations name tests that exist at top level in./tools/ci/, and each is traced to the defect it declares on the fixture it drives.The measurement is therefore deferred to the first scheduled
falsify:all, which replays every spec nightly and reddens with its own issue if any mutation has stopped biting. That is weaker than a replay before merge and it is written here rather than left out.Pre-existing failure, disclosed
mise run check's test step fails onTestADroppedConnectionIsClaimedOnlyWhenItHappened(internal/core/emulator, faults_transport_test.go:149). It fails identically on unmodified base and passes in the fullgo test -race ./...run on this tree, so it is a flaky, pre-existing failure unrelated to this change — this PR touches nothing under internal/.Not in this PR
As the issue says: #731 (the
kind: "infra"classification) and #876 (the runtime-proof PR gate).