8 Commits

Include renames

Author	SHA1	Message	Date
timothy	2a2dcacd58	test(649): cover the review-verdict status read, and the guards that only fire on the bot path ... Additive tests for properties #666 shipped correctly but left unguarded. No behaviour change. The stub's status read was hardcoded to "no verdict yet", so two whole branches of the classify step were unreachable from any test. Four mutations survived the full suite because of it — including re-introducing the literal ersatztv#647 fail-open, and overwriting an existing human verdict. The stub now models a transport error, a garbage body, and an existing verdict. `test_an_EMPTY_enumeration_is_not_exempt_even_for_a_BOT` needs the bot author to test anything: with a non-bot, the blank line an empty list produces already fails DOCS_ONLY, so the `count -eq 0` guard never decides the outcome. On the bot path it is the ONLY thing between an enumeration that read nothing and an unattended success. Verified by mutation — `grep -c .` -> `grep -c ''` grants a bot PR success while every other test stays green. Same short-circuit shape as the PROTECTED/DOCS_ONLY disjointness this file already documents. Two anchors were also unguarded: `grep -qxF` (author `ova` is a substring of `renovate`) and DOCS_ONLY's `$` (`evil.mdx` reads as docs-only). Five of the six mutations are caught behaviourally. The sixth — dropping the shell emptiness check — cannot be caught locally: `jq -e` over empty input exits 4 on jq 1.8 so the guard still fires on a dev Mac, and 0 on the runner's 1.6 where it is the actual bug. A structural assertion closes that gap, with comments stripped first, since a raw substring search is satisfiable by moving the guard into a comment while deleting the real one — verified. refs #649, #672	2026-07-27 07:40:00 +02:00
timothy	e960d5b918	test(649): make the POST-wiring assertion unable to opt out or accept the wrong host ... Re-review found the verifier could disable itself two ways, both of which look like coverage: - it was guarded by `if url_file.exists()`, so deleting the recorder in the stub turned the whole assertion into a no-op and every test stayed green; - it compared only the URL SUFFIX, so a POST to the right path on the wrong HOST or the wrong REPO passed — which is exactly the class the assertion was added to catch. It now requires the URL to have been recorded whenever a status was posted, and compares the full URL against the env the job was given. Mutation-verified three ways: wrong host, wrong repo, and deleting the recorder each redden the suite. Refs #649	2026-07-26 23:38:25 +02:00
timothy	322dd43d10	fix(649): close the test-isolation gaps cold review found, and make the job's Gitea config authoritative ... Four findings acted on; two more are real but pre-existing and are being filed rather than fixed here (see below). **The job's Gitea config was not authoritative.** `pr-changed-files.sh` resolves `ETV_GITEA_URL` BEFORE `GITEA_BASE_URL` (and `ETV_GITEA_TOKEN` before `GITEA_TOKEN`), because its other caller is a developer Mac using the ETV_* convention. Setting only the GITEA_* names meant a runner exporting a stale ETV_GITEA_URL would enumerate a DIFFERENT Gitea instance and this job would post a verdict here from a diff read there. Both names are now set to the same value, so precedence cannot matter. **Three guards passed their tests for the wrong reason.** Each was confirmed by deleting the clause and watching the suite stay green — the reviewer asserted it, mutation proved it: - The explicit empty-response clause was uncovered on jq 1.8, because jq 1.8 rejects empty input by itself. jq 1.6 does not, and THE RUNNER SHIPS 1.6 — so the one environment where the clause is load-bearing had no coverage. That is the #643/#647 failure class reproduced inside the suite meant to prevent it. Now covered by importing the existing jq-1.6 shim (imported, not copied — a second quirk emulator is the same drift problem one level down), with a verify-the-verifier test and a positive control. - `type == "array"` needed a body whose VALUES are valid rows. Two earlier attempts failed for a third reason: `jq`'s `all(.[]; …)` iterates an object's values, so `{"message":"…"}` and a single flat row are both rejected by `.filename` erroring on a string. Only `{"0": {…valid row…}}` reaches the fail-open, where a non-array body enumerates as a complete docs-only list. - `.filename | ok` is now isolated by a row carrying a valid `.status` and no filename, removing the closed-allow-list as a second reason to reject. **Two assertions proved less than their names claimed.** `"jq-preflight.sh" in code` also matched the `[ -x … ]` presence guard, so deleting the invocation left it green; it now requires an invoking line. `_run_classify` accepted every POST, so a status aimed at the wrong endpoint or sha would not have been noticed; it now asserts the POST lands on `/statuses/<full head sha>`. **One test name overclaimed** and is narrowed rather than left implying coverage it does not have: the head-movement test proves "final head != expected sha", not movement *during* enumeration. Deferred, both pre-existing and neither introduced here — filed as follow-ups: - A commit status is repo-global, so a `review-verdict/h10=success` obtained for head H on one PR is inherited by any other PR with the same head, including one opened against a different base. Same class as #632, reached by a third route. - The A->B->A force-push race: paging is several round-trips and the head is re-read once at the end, so a restore to the original sha passes the binding while the pages came from two states. Inherent to enumerating a mutable list over an API with no commit-pinned files endpoint. Refs #649	2026-07-26 23:23:52 +02:00
timothy	9114a7e8af	fix(649): point the ENFORCED review-verdict gate at the shared PR-file enumeration ... #658 landed the shared implementation, `scripts/pr-changed-files.sh`, and rewired the ADVISORY hook onto it. The ENFORCED copy — the one that writes the branch-protection- required `review-verdict/h10` status — was left byte-identical to main, so its fail-closed behaviour on a malformed or empty response stayed INCIDENTAL: an empty `n` erroring `[ "$n" -lt 50 ]` to false. That is #649's second Done-when box, and the whole point of the issue was that the gate with real authority was weaker than the gate with none. `review-verdict.yml` now: - checks out the PR's BASE ref (`base.sha`, `persist-credentials: false`), never the head, so a PR cannot supply the code that judges it; - runs `scripts/jq-preflight.sh` in FLOOR-ONLY mode — `--expect` here would deadlock every merge on `main` the day the runner's jq changes; - calls `scripts/pr-changed-files.sh` and reads its EXIT STATUS, never its stdout on a failure path. The env trap flagged in review is handled: the script reads GITEA_BASE_URL and takes owner/repo as two separate arguments, so passing BASE_URL and a combined `owner/repo` would have silently fallen back to the hardcoded LAN default. The ~40 lines of inline enumeration are deleted, so the two copies can no longer drift. A base ref predating #658 has no such script; that posts `pending` with the reason rather than dying with no status at all. The drift guard is re-tightened from "the hook uses the shared script" to "BOTH callers do", and the workflow's own preconditions are pinned by parsing the YAML rather than substring-matching it — `head.sha` for `base.sha` is a nine-character diff. Verified by mutation, six mutants, each killed by its intended test: ignore the exit status; check out the head; drop `persist-credentials`; add `--expect`; re-inline a `pulls/N/files?` fetch; delete the PROTECTED clause. That last one initially MISSED, and the miss was the useful finding. The test used a docs-only-plus-protected file list and passed with the clause deleted, because PROTECTED (`.claude/ .gitea/ .husky/ scripts/ docker/ci/`) and DOCS_ONLY (`docs/`, root `*.md`) are disjoint — on the docs-only path that clause can never fire, and DOCS_ONLY was doing all the work. PROTECTED is load-bearing only on the BOT path, so the test now covers a Renovate PR editing the shared script, with a positive control proving the bot exemption fires at all. The caller contract is tested by EXECUTING the workflow's `run:` block against a stubbed enumeration that fails while emitting a perfectly docs-only list — the one combination the "every failure path also happens to print nothing" redundancy cannot absorb, and the exact mutation that survived the whole suite last round. Docs: both "Landing note" blocks removed, and the record's base-ref paragraph converted from a future-tense requirement to present-tense fact with its staging rationale kept as history. Refs #649 Decisions-Edit: yes	2026-07-26 23:05:14 +02:00
timothy	4e094637c6	fix(648): the version parser was fail-OPEN on a jq that cannot start ... Round 3, and it found that round 2's fix was a REGRESSION on the case that matters most. `raw=$(jq --version 2>&1 || true)` did two wrong things at once: folded stderr into the parse input and discarded the exit status. Combined with a pattern that matched the first <digits>.<digits> ANYWHERE, a jq broken by a glibc mismatch — which exits 127 and writes "version `GLIBC_2.34' not found" to stderr — parsed as version 2.34 and PASSED the floor. The strip-based parse this replaced failed CLOSED there. So the fix for a fail-open bug introduced a worse fail-open bug, in the one script whose entire purpose is to refuse to certify a version it did not parse. Same mechanism, second symptom: an unanchored match let a prefix outrank the real version. `2026.07.26 jq-1.6` parsed as 2026.07; a leading warning line carrying any number won too. Now: jq's exit status is captured explicitly (`$?` inside `if ! cmd` is the NEGATED status, so that needed care too), stderr is kept out of the parse, and the pattern is anchored to the leading `jq` token. Every legitimate form still parses — `jq-1.6`, `jq version 1.6`, `jq-1.7.1`, `jq-1.6-dirty`, `jq-1.6 (Debian 1.6-2.1)`, `jq-1.10` (numeric compare, so the two-digit minor is not read lexically). The tests could not have caught any of this: the shim always exited 0 and never wrote to stderr, so every case it could express was clean. It now takes stderr and an exit code, and the four new cases turn red under the exact mutation. Also: the drift guard now strips comment lines before matching. A future comment citing `pulls/$pr/files?limit=100` as an example of what not to do would otherwise have reddened script-tests — which, per this branch's own correction, blocks merges. And the record no longer over-corrects: the combined-status read is guarded by `if [ "$mwcs" != "true" ]`, so a red script-tests blocks the hook-mediated merge path, not literally every merge. Decisions-Edit: yes	2026-07-26 22:21:07 +02:00
timothy	5e7623b8d5	fix(648,649): security-review round 2 — close the version-parse hole and the untested caller contract ... Two real defects, and three docs claims that were simply wrong. jq-preflight.sh parsed the version by stripping around the first `-` and `.`, which assumed the format is exactly `jq-X.Y`. A build printing `jq version 1.6` left major empty; the sanity check concatenated major+minor into "6", which is non-empty and all-digits, so it PASSED. The floor comparison then ran `[ "" -lt 1 ]`, which errors — and `set -e` exempts a failing command in an `if` condition, so the conditional read false and the script exited 0 having asserted nothing, after printing a plausible "parsed" line. The silently-untested-axis failure this script exists to eliminate, reproduced inside the script itself. Now parsed by explicit regex, failing closed with a diagnosis when there is no <digits>.<digits> match. Also: `--expect` with no value exited 1 with empty output on both streams. The hook's exit-status check was pinned by nothing: mutating `if files=$(...)` into `files=$(...) || true; files_complete=yes` left the ENTIRE suite green. It survived only by redundancy — the script writes stdout once, right before exit 0, so failures also happen to yield empty stdout and `[ -n "$files" ]` catches it. Safe by accident, which is the exact criticism this branch levels at the old code. Four tests now pin it, with a stub that FAILS while emitting a docs-only list (the one case redundancy cannot absorb) plus a positive control proving the harness can see the difference. Verified: the mutation now turns exactly those tests red. Docs corrections. The record claimed the --expect pin was safe because script-tests is "advisory, not a required check" — false. The merge-consent hook reads the COMBINED status (ci.advisory-red-blocks-the-merge-gate, #598), so firing the tripwire blocks every non-docs-only merge until someone re-pins. Kept anyway, for a stated reason, but no longer described as free. The record also asserted in the present tense that review-verdict.yml checks out the base ref; it has no checkout step at all, so that is now a future-tense requirement on the follow-up. And the documented .status allow-list named GitHub's `removed`, which the code rejects. The drift-guard regex anchored on `?limit=`, so a re-inlined copy written `files?page=1&limit=50` would have walked past it. Decisions-Edit: yes	2026-07-26 22:21:07 +02:00
timothy	2c10f057b8	fix(648,649): stage the enforced-gate wiring behind the scripts it calls ... Splits the review-verdict.yml rewiring out of this PR. That workflow checks out the PR's BASE ref — deliberately, so a PR cannot rewrite the gate that judges it — and the base is main, which does not yet contain scripts/pr-changed-files.sh or scripts/jq-preflight.sh. Wiring it here would make the job exit 127 on its own PR and block the merge gate through the combined status, which reads red jobs as blocking. So this PR lands the scripts, their tests, the hook rewiring and the script-tests jq pin; the follow-up points review-verdict.yml at them once they exist on main. The two tests that asserted on review-verdict.yml are scoped accordingly, each carrying the reason. test_review_verdict_never_pins_a_jq_version is asserted NOW rather than in the follow-up, so the no-pin constraint on the required check is already enforced when the wiring lands. Decisions-Edit: yes	2026-07-26 22:21:07 +02:00
timothy	2fd798cccf	fix(648,649): one shared PR-file enumeration + an explicit jq version contract ... #649 — the enforced review-verdict.yml guard had drifted strictly WEAKER than the advisory merge-consent hook: four rounds of #643 hardening landed on the copy whose failures produce a human prompt, and never reached the copy that writes the branch-protection-required review-verdict/h10 status. Its fail-closed behaviour on a garbage response was also incidental (an empty `n` erroring a bash conditional to false), not designed. Extract scripts/pr-changed-files.sh as the single implementation both call. Shared MECHANISM, not policy: the two docs-only allow-lists differ deliberately and stay separate. review-verdict.yml now checks out the BASE ref, never the PR head, so a PR cannot rewrite the gate that judges it. #648 — baking jq into docker/ci/Dockerfile provably cannot cover the gate that broke: review-verdict.yml is runs-on:small with no toolchain pin, so it gets the host's jq 1.6 (checked, not assumed). Add scripts/jq-preflight.sh: floor+observable everywhere, and a --expect tripwire on script-tests only — pinning the required merge check would deadlock every merge on a jq bump. Verified by mutation: six guards individually broken, each turning exactly its own test red, then restored byte-identical. fixes #648 fixes #649	2026-07-26 22:21:07 +02:00