timothy
0cf355e494
Two independent reviewers (one cross-family) converged on the same defect, and it was the
important one: the preflight WARNED and exited 0 on every answer that was not 200 or 404,
so a missing `curl`, a moved registry or a DNS change would have left it green forever —
"the check could not run" presenting as "the pin is fine", in a script whose own header
disclaimed exactly that. Unknown answers are now retried (3x, 5s) and then FAIL, with
wording kept distinct from the deleted case because the two send an operator to different
places.
Also from the reviews:
* An absent secret does not arrive as an unset variable. `${{ secrets.X }}:${{ secrets.Y }}`
interpolates to ":", a perfectly non-empty and perfectly useless credential, and the
tests covered only the unset shape. Both halves are now required, and the parametrised
test drives the production shape.
* HTTP 200 is not a manifest. A proxy or a login page answers 200 too, so the body is
fetched and matched for `schemaVersion` (a shell `case`, so no jq dependency and no
pipeline that can inject).
* The curl stub ignored `-u` and answered 200 regardless, so deleting the real `-u` would
have left the suite green while the live registry rejected every request. It now 401s an
unauthenticated read, as the registry does.
* The mutation's declared diagnostic changed with the script: now that unknown fails too,
the exit code no longer separates "deleted" from "could not check", so the proof turns on
the message and `expect` says so.
* docs/ci-cd.md: `scan` is no longer the only `docker-build.yml` job on the small lane, so
the tag-push exclusivity claim and the lane membership were both false. Fixed.
* "Immutable" was overstated: `ci-image.yml` tags `rev-parse --short HEAD`, so a dispatch or
a weekly no-cache run at the same HEAD republishes that tag from a rebuilt image. Stated,
along with what the rebuild recovery does NOT restore (mutable bases and apt, so equivalent
rather than bit-identical).
* The recovery recipe left you in a worktree checked out at the pin commit — where the
verify script does not exist, and where the workflow carries the pre-bump pin. It now
keeps `$repo`, returns, and removes the worktree. It also needed BuildKit's `http = true`
caveat: the container driver does not inherit the daemon's insecure-registries.
* The root cause carries its evidentiary limit and its reproduction commands, and says what
to conclude if a pin vanishes after server-management#842 lands (refuted, not re-applied).
* The `ci.required-job-step-execution-markers` carve-out named one container-free job; there
are two now, and the membership is what rots.
* The decision record's `''` YAML escapes leaked into rendered prose; "status, no comment ->
ask" is qualified (a prior positive verdict for the SAME head still satisfies condition
(c)); "exits 1" is "exits non-zero" (usage exits 2, jq its own status, signals 128+n).
refs #772
refs #792
Decisions-Edit: yes
4.2 KiB
key, title, status, since, supersedes, superseded-by, rule, signals, mechanics
| key | title | status | since | supersedes | superseded-by | rule | signals | mechanics |
|---|---|---|---|---|---|---|---|---|
| release.verdict-writes-status-before-comment | 2026-08-22 — the verdict STATUS is written before the verdict COMMENT, so the only reachable half-state is the safe one (#792) | active | 2026-08-22 | none | none | `scripts/post-review-verdict.sh` writes the sha-bound `review-verdict/h10` commit status FIRST and the human-readable `Review-verdict:` comment SECOND. Every refusal path still refuses (fail-closed, unchanged) and exits non-zero, and none of them may leave a verdict comment behind. An orphaned comment is therefore PREVENTED rather than tolerated. If the comment write fails after the status was written, that is an error too, but it degrades to an `ask` at the merge gate rather than to an apparent grant. | verdict comment without a status, half-posted verdict, orphaned Review-verdict comment, exit code lies, post-review-verdict exits 0 · paths: `scripts/post-review-verdict.sh`, `scripts/tests/test_post_review_verdict.py`, `.claude/hooks/pretooluse-merge-consent.sh` · issues: #792, #622, #632, #778 | `scripts/tests/test_post_review_verdict.py::test_the_status_is_written_BEFORE_the_comment`, `::test_no_refusal_leaves_a_VERDICT_COMMENT_standing_in_for_the_status` (every refusal mode), `::test_every_path_that_writes_NO_STATUS_exits_non_zero`, `::test_a_failed_COMMENT_after_a_written_status_is_still_an_error` |
The script has two writes and they are not equal. The status is the gate — a required context on
main, bound to one sha. The comment is the artifact a human reads, and the merge hook's
condition (c). Writing the comment first meant that every refusal between the two writes left a PR
carrying Review-verdict: MERGEABLE @ <head> with no status behind it: an artifact that reads as
granted consent, produced by the very run that refused to grant it. The refusals are correct and are
not what changed (ci.verdict-write-retarget-fence — the fence must keep refusing when it cannot
bind safely); what changed is which write survives a partial failure.
Ordering settles it without a rollback, and rollback is the option not taken: deleting or annotating the orphaned comment needs a Gitea call, and the refusals it would compensate for are frequently caused by Gitea being unreachable, so the compensating write is unavailable exactly when it is needed. Ordering costs nothing and cannot fail to apply.
The two surviving half-states are asymmetric, and that asymmetry is the whole justification:
- comment, no status → the hook's condition (c) classifies a positive verdict, the operator sees consent, and only the required check stands between that and a merge. Fail-open in appearance.
- status, no comment → the hook has no verdict for this head to classify, which it resolves as
ask. Fail-closed, visible, and cured by re-running the command. The qualification, because
the hook reads the whole comment history rather than this run's write: if a positive verdict for
the SAME head already exists, condition (c) is satisfied by it and the hook may grant — which is
correct, since that comment covers this exact sha. A stale or negative verdict yields deny. So
askis the outcome when the current head has no verdict comment, and nothing here can produce a grant over a head no one reviewed.
#792's premise about the exit code was wrong, and is corrected rather than repeated. The issue
reported the script printing its refusal and exiting 0. Re-measured on the tree that carries #632's
fence, every no-status path exits NON-ZERO (die exits 1, the usage path 2, a failing jq its own
status, a signal 128+n) — eight refusal modes are now driven through the real entry point
and asserted, and those assertions pass against the predecessor as well, which is how we know the
defect was never in the script. The observed 0 came from the invocation around it (a pipeline reports
its last command's status, not the script's). The exit-code contract is asserted anyway: it was true
by convention, held by one shared die helper, and nothing had ever executed it.
Read with release.verdict-status-check (why the status, not the comment, is the gate) and
release.review-verdict-gate (the comment convention itself).