9 Commits

Include renames

Author	SHA1	Message	Date
timothyandClaude Opus 5	94cc924c36	fix(845): the retracted set -u mechanism survived seven lines below its own retraction — and the no-set -e case is now measured ... Found by sweeping the RETRACTED WORDS repo-wide rather than re-reading the file I had edited, which is the `stale-comments-sweep-by-subject` lesson the previous round already charged me with once. `scripts/lib/review-verdict-vocabulary.sh` still said "A control-flow gate can be skipped by an abort" seven lines under the paragraph retracting exactly that mechanism — the same self-contradiction shape as the sentinel comment in the last round, in the file that round corrected. MEASURING IT PROPERLY CHANGED WHAT I CHECKED. The decision record justifies the concern by noting the READ side deliberately runs without `set -e`, and both prior reviewers had measured the claim only under `set -euo pipefail`. If the original reading held without `set -e`, the correction shipped last round would itself have been wrong. Measured on bash 3.2.57 and 5.3.15 across all three modes: [set -u] in f / DEFINITELY_UNSET: unbound variable / exit=127 [set -eu] in f / DEFINITELY_UNSET: unbound variable / exit=1 [set -euo pipefail] in f / DEFINITELY_UNSET: unbound variable / exit=1 No branch taken and nothing after the gate, in every mode — including the one the record relies on. The correction stands. The old reading holds only inside a command substitution (`v=$(f) || v=FALLBACK` continues, fallback taken), which is not the shape gating either file. WHAT IS NOT IN QUESTION, stated because the retraction is easy to over-read: the fail-open that record documents is REAL — with `.*` in the positive list and the negative list written as a scalar, an explicit `BLOCKED @ <head>` classified `positive`, exit 0. Only the attributed mechanism is wrong: `${#name[@]}` on a scalar yields 1 rather than erroring, so no abort occurred in that scenario at all. The sentinel is still the right design, on the ground that survives — the caller that never runs validation. The same refuted mechanism remains in `release.verdict-vocabulary-shared` and in `test_review_verdict_vocabulary.py`. Correcting a decision record is its own change with its own review surface, so it is tracked as ersatztv#886 with the full measurement, and cross-referenced from the corrected file rather than folded in here. fixes #845 refs #886 Decisions-Edit: yes Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019zUmJZHhVP7kXg5DV237TW	2026-08-30 03:38:47 +02:00
timothyandClaude Opus 5	0f4bb4244d	fix(845): the invariant that replaced a drifting count was itself false, and two of the counts survived ... Fifth cold review, on the previous fix commit. Every finding is prose, and three of them are claims that commit's own message said it had fixed. On a branch whose stated thesis is "four prose claims were falsifiable by grep", shipping four more is the finding. Each verified here before acting. - "replaced in all four places" was wrong: there were FIVE occurrences of `31`. Two survived, one of them four lines below the sentence the same commit added saying the number drifts. - "the WHOLE writer suite reddens" is measurably false. Injecting a second anchored `H10_REVIEWERS=` gives 36 failed / 39 passed — a large part, not all. The commit invoked `state-the-invariant-not-the-measurement` and then wrote a claim STRONGER than the count it removed. Now stated without a count and without "whole". - The sentinel re-attribution retracted the environment-defeating ground in one paragraph and still asserted it seven lines below ("the two that were measured"). The sibling library corrected in the same commit got this right ("the ground that survives", singular). This is `stale-comments-sweep-by-subject`: after a retraction, grep the retracted words. - "spans THREE top-level directories" replaced a TRUE hedge with a false count, and did not match the workflow it was supposed to be synced with (which says "at least three"). The suite reads seven — `.claude`, `.gitea`, `.husky`, `docs`, `ErsatzTV`, `scripts`, `web` — so this PR's own doc edits are inputs to that job. Hedge restored, with the reason. Also from that review: the `if [ "$state" = "success" ]` body was unindented at column 0, and "Fails CLOSED on every branch" now means every branch OF THAT LOAD, since the load is itself conditional. Both fixed; the indent change is whitespace-only (`git diff -w` empty). The behaviour was attacked again and held. The reviewer traced a BLOCKED verdict end to end with `scripts/lib/h10-reviewers.sh` DELETED — rc=0, status and comment both posted, no unbound variable and no command-not-found, because `$state` is set by the `case` well before the guard and every reference to the library sits inside one of the two `state = success` blocks. An APPROVAL on the same tree still refuses before writing anything. Removing the guard reddens the new rejection test; `if true` reddens 20 tests. The declared mutation still reddens its named proof. fixes #845 Decisions-Edit: yes Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019zUmJZHhVP7kXg5DV237TW	2026-08-30 03:38:47 +02:00
timothyandClaude Opus 5	f54667cc5a	fix(845): a declared security invariant had no guard, and four prose claims were falsifiable by grep ... Fourth cold review, worktree-isolated, on the rebased head. Every finding reproduced before acting; the two behavioural ones were re-verified with the reviewer's own mutations. MEDIUM — "no environment variable selects the source file" was declared as a deliberate security decision, in the library header AND in `docs/remote-state-inventory.md`, with no test anywhere. The property held, but restoring the override the way a refactor naturally would (`ETV_H10_REVIEWERS_WORKFLOW="${ETV_H10_REVIEWERS_WORKFLOW:-}"`) left the whole suite green while an env var installed an arbitrary allow-list and an off-list account was accepted as a reviewer. That is the class this branch removed in `28aab4632`, in a clause labelled as a decision rather than as untested. Now pinned, and confirmed to redden under that exact mutation. LOW (behavioural) — a BLOCKED verdict was refused when the allow-list could not be DERIVED. Membership never applies to a `failure`, but the derivation was loaded unconditionally, so a workflow with a broken `H10_REVIEWERS` declaration refused rejections too — the exact outcome the success-only rule exists to prevent ("no supported way to record a rejection"), reached one condition earlier, on the branch most likely to have broken that declaration: the one editing it. The load is now guarded by state; both of its stated properties (after argument validation, before the first network call) survive. A test covers the rejection AND the approval on the same tree, so the guard was narrowed rather than removed. Prose, all four falsifiable by `grep` or `pytest`: - The comment claimed `review-verdict.yml` "rejected a hardcoded limit for its own read". It does not — it sends `?limit=100` on both reads. What it rejected was using that limit as a COMPLETENESS argument, which was dead code because the instance caps at 50. The real point (presence vs absence) was right and is kept; a reader who greps the workflow no longer finds the opposite of what this says. - "31 red writer tests" had drifted to 40 INSIDE this branch as tests were added, in all four places the previous commit said it had restated. Replaced with the invariant — the whole writer suite reddens — per `state-the-invariant-not-the-measurement`, which is exactly the lesson the stale number re-taught. - The sentinel-reset comment cited a measurement that does not isolate it: removing the reset produces a byte-identical refusal, because `load` overwrites both variables on that path. The reset's real ground is the caller that never reaches `load`, which is what `contains` returning 2 rests on. Re-attributed. - `docs/ci-cd.md` still mirrored a TWO-directory input set for `script-tests` after this same PR widened the workflow's own comment to three. Synced. Also: `scripts/lib/review-verdict-vocabulary.sh` still shipped the `set -u` claim this branch demonstrated to be wrong, one file over, and it was the source the new library copied it from. Corrected there too — measured on bash 3.2.57 and 5.3.15, the shell exits outright in both the `if ! f` and `f; rc=$?` shapes; the old reading holds only inside a command substitution. Its sentinel keeps the ground that survives. Nits: a stale "~10s" runtime in a sentence this branch rewrote, replaced with the reason that does not drift; an unresolvable breadcrumb citing a private memory filename rather than a decision key, dropped; "so only this run writes it" narrowed to what the description actually identifies; and the paging test's docstring now says it is a POSITIVE CONTROL rather than claiming a property pin it cannot deliver. fixes #845 Decisions-Edit: yes Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019zUmJZHhVP7kXg5DV237TW	2026-08-30 03:38:47 +02:00
timothyandClaude Opus 5	a55f47023e	fix(845): a cross-family review and a fix-commit re-review — the .state fallback recreated #845, and the paging model was backwards ... Two more independent cold reviews: GPT-5.6 via Codex (cross-family) and a worktree-isolated Opus re-review of the previous fix commit. They converged on the paging defect from different directions. Each finding below was reproduced before acting. HIGH — the `(.status // .state)` fallback recreated the bug it was added to prevent. The gate reads `.status` and nothing else. On a response carrying only `.state`, the fallback made THIS tool accept and report success while the gate saw no recognisable verdict and re-derived it — which is #845 exactly, reached through the check meant to stop it. My comment claimed it "degrades to working"; it degrades to the bug. Removed: the writer now reads exactly what the gate reads, and a shape neither understands is refused loudly. MEDIUM — the allow-list was applied to `failure` too, which the gate deliberately does not do. `review-verdict.yml` inherits a `success` only from a member but a `failure` from any ATTRIBUTABLE account, and says so at length: inheriting a rejection can only withhold an exemption, while re-deriving one can turn it green. So refusing an off-list `BLOCKED` refused a verdict the gate honours, told the reviewer their rejection would be re-derived when it would not, and — because the comment is withheld on refusal — left a real reviewer no supported way to record a rejection at all. Membership is now required for `success` only. MEDIUM — the paging model was backwards, and so was the reasoning built on it. Gitea selects each context's MAX row id, orders those DESCENDING, and paginates that. Measured: a head with ids [17,19,...,41,43] returns 41 and 43 at `?limit=2`, and on another head the h10 row appears at `limit=9` but not `limit=8`, exactly its rank by recency. The status this script has just POSTed is the newest on the head, so it sorts FIRST and page 1 always holds it. The shim appended it LAST and sliced from the front, which manufactured a truncation the server cannot produce — so `test_a_head_carrying_more_contexts_than_the_server_cap_refuses` asserted a fiction, and the `?limit=100` "witness" was circular: observable only because the fixture was wrong in precisely the direction that made it observable. The shim now orders newest-first, that test is deleted, and `?limit=100` is kept as declared-unwitnessed insurance rather than dressed up as a tested guard. Also corrected, each verified first: - The `readback_rows` fallback was unreachable and its test never reached the branch it named. The fallback is gone; the test now asserts the message an empty body actually produces. The blank-count bug was real and was fixed by the restructure, not by the clause I credited. - "A repo-wide merge outage" overstated the guard's role. Such an edit landing WOULD be one, but `script-tests` is ADVISORY — measured, the required contexts are `Build & test`, `EF migration integrity` and `review-verdict/h10` — so the writer suite already reddens the PR. What alignment buys is a diagnostic naming the cause instead of 31 unrelated failures. Restated in all four places, and connected to the snapshot boundary, which is the route by which the outage is actually reachable. - The IFS test's docstring blamed the wrong split. `load` stores the RAW string, so its pin affects validation only; membership is the `contains` split, and only that one is witnessed. - The two `|| die` guards on already-constrained jq output are declared unreachable rather than counted as protection, and a multi-document body now refuses instead of printing "0 0 separate rows". - Why the writer may hardcode a limit where the gate deliberately refused to: the gate must prove a verdict ABSENT, which no single page can establish; this only confirms a row it just wrote is PRESENT. Presence needs no completeness argument. CORRECTION to the previous commit message: it claimed four clauses were made witnessed, including the `set -f` restoration. Measured on that predecessor, `set -f` restoration already reddened its named test at both sites, and the one genuinely unwitnessed `set -f` — the `contains`-side pathname guard — still is, correctly, since the validator excludes every metacharacter before it runs. Three, not four. fixes #845 Decisions-Edit: yes Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019zUmJZHhVP7kXg5DV237TW	2026-08-30 03:38:47 +02:00
timothyandClaude Opus 5	04a3752432	test(845): the two identify-our-own-write arms masked each other; grammar in the library header ... Caught while a cross-family review was probing the same spot, and confirmed before fixing: `test_a_status_OVERWRITTEN_...` used a fixture differing in BOTH state and description, so either arm of `[ state != ] || [ desc != ]` could be deleted with the suite still green. Each arm was individually disarmed and the suite stayed at 69 passed both times — #685's duplicate-guards-mask-each-other shape. Each arm now has a fixture differing in exactly one field, and both were confirmed to redden their own named test. The creator in each is deliberately ALLOW-LISTED, so the refusal has to come from identifying the row as not-ours rather than from its author. The script now also states WHICH arm carries the safety, matching how this file already handles its overlapping `-z`/`-n` arms: the DESCRIPTION is the identifier (verdict word, short sha, base — only this run writes it), and the STATE is defence in depth, since our own writer cannot produce our description with a different state. Also: apostrophes restored in four library header sentences that read as typos. fixes #845 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019zUmJZHhVP7kXg5DV237TW	2026-08-30 03:38:47 +02:00
timothyandClaude Opus 5	e3a19227c1	fix(845): two independent cold reviews — the guard did not walk the same tree, and CLAUDE.md said the status is not written ... Two cold reviewers, independently briefed and worktree-isolated. One returned BLOCKED on the first two below; the other returned MERGEABLE having not examined them. No arbitration was needed — both are verifiable, and I reproduced each before acting. BLOCKING 1 — the CI guard parsed a different population than the library. `test_the_H10_REVIEWERS_list_is_glob_free_and_non_empty` read `_classify_step()["run"]`, one step's body. `scripts/lib/h10-reviewers.sh` greps the WHOLE YAML and refuses on anything but exactly one match. So a second anchored `H10_REVIEWERS=` in ANY other `run:` block was invisible to the guard while making the writer refuse every verdict — and `main` is PR-only with `review-verdict/h10` required, so that is a repo-wide merge outage. Reproduced: injecting one into the last step left the guard green and reddened 31 writer tests whose diagnostics said nothing about a second assignment. The guard now reads the whole file, so it walks the same tree as its subject; the header and the decision record no longer claim coverage that did not exist. BLOCKING 2 — `CLAUDE.md` said an off-list verdict "fails loudly at your terminal instead of being written". The status IS written; only the comment is withheld. The script, `ci-cd.md` and the record all said so correctly, and the most-read doc contradicted them. It now also says what the other docs did not: the green left standing satisfies the server-side required check, because branch protection binds the context NAME and not its issuer. Four clauses were shipped UNWITNESSED, found by the reviewer's own disarm matrix and each now reddening a named test: the `.statuses` array-TYPE test, the read-back GET's explicit `|| die`, `?limit=100`, and the `set -f` restoration. The first two passed only because a downstream check produced the same non-zero exit — the shape this very file already documents at the `-z`/`-n` arms. They now assert their own diagnostic. `?limit=100` was unobservable because the shim discarded the query string; the shim now models the measured server (default 30, clamped to 50, `total_count` reporting the page). Also fixed, all reproduced first: - `[0]` over same-context rows had no uniqueness guard. Unreachable today, but the alternative is judging one row's author while another is the one standing. - The row count in the not-found refusal came out BLANK on a 200 with an empty body: jq exits 0 with empty output, so the `||` fallback was dead exactly where the count matters. Tests the value now, not jq's exit. - The split pinned globbing but not IFS. A caller with `IFS=-` made `renovate` read as a member of `renovate-bot`. Unreachable from this script (bash resets IFS), pinned because the header invites reuse. - The `set -u` rationale was inherited and is WRONG. Both reviewers measured it: the shell EXITS, in both the `if ! f` and the `f; rc=$?` shapes, on bash 5.3 and 3.2. Nothing here depended on it; the sentinel now rests on the two grounds that were measured. - The `?limit=100` margin was overstated. This instance clamps to MAX_RESPONSE_ITEMS = 50, so the headroom is ~3x over today's 8-15 contexts, not 6x, and it shrinks as checks are added. - The snapshot boundary now names its DIRECTION in all three places: it is a false ACCEPT, and the likely edit (ADDING a reviewer) is exactly the one it mis-handles. - The overwrite refusal told the operator to re-review when a gate re-derive needs no review. A note on the measurements, since two of my own disarms were inert and briefly read as "no test covers this": a mutation must be checked to actually mutate. The glob-class disarm still excluded `*`, and the IFS disarm matched two sites and applied to neither. fixes #845 Decisions-Edit: yes Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019zUmJZHhVP7kXg5DV237TW	2026-08-30 03:38:47 +02:00
timothyandClaude Opus 5	afe61e80aa	fix(845): the verdict writer checks that the gate will honour what it just posted ... `.gitea/workflows/review-verdict.yml` inherits an existing `review-verdict/h10=success` only from a status whose `.creator.login` is on its `H10_REVIEWERS` allow-list (#742). `scripts/post-review-verdict.sh` writes those verdicts with whatever account owns `ETV_GITEA_TOKEN`/`ETV_GITEA_BASICAUTH` and never asked whose it was. The two values were coupled with nothing asserting the coupling, and the failure mode was the silent one: the status really is written, the tool really does report success, and then the next `pull_request_target` event re-derives it and posts over it. And again. The PR deadlocks and the only diagnostic is a `::warning::` inside a workflow run nobody is reading. The script now READS THE STATUS BACK after posting it, identifies its own write by state and description, and refuses — before the comment, so the surviving half-state is the documented `ask` one — unless the recorded creator is on the allow-list. Measured after the write rather than probed before it: a pre-flight `GET /user` tests what the credential claims to be, this tests what Gitea recorded as the author of this status, which is the value the gate reads. It also needs no scope beyond the repo access the POST already required. The allow-list is not restated. `scripts/lib/h10-reviewers.sh` derives it from the workflow's own literal, so there is ONE declaration rather than two held together by a parity test — the shape `testing.guard-derives-population-from-source` records six failed rounds against. It is a parse rather than a shared declaration both sides source, and that is not a preference: the gate runs against a checkout of the PR's BASE sha, so a PR whose base predates a new `scripts/lib/` file would not have it, and a missing `source` under `set -euo pipefail` kills the job — which posts no `review-verdict/h10` at all and blocks every merge to `main` including the PR that would repair it, with no admin override (#743). A fallback list for that case reintroduces the second copy. So the derivation runs on the side that can fail safely and the gate's body is untouched. Fail-closed on every branch of the derivation: 0 or >1 anchored assignments, an entry that is not a plain login, an empty list, or an unreadable file all refuse BEFORE any network call. The membership query answers member / not-a-member / cannot-tell as three distinct codes rather than two, so a caller cannot fold "cannot tell" into either. The safety DIRECTION is unchanged and the commit does not claim otherwise: a non-allow-listed verdict was always a stall, never a fail-open. What changes is where the signal lands — at the terminal that made the mistake, instead of one PR event later. Residual, stated rather than fixed: the check runs after the POST, so a refused verdict leaves a non-inheritable status standing with no comment beside it. That is the `ask` half-state `release.verdict-writes-status-before-comment` already designates as the safe one, and the gate re-derives it on the next PR event. A second corrective write is the sticky-sentinel mechanism #849 is separately designing; inventing a parallel one here would be two mechanisms for one invariant. `scripts/post-review-verdict.sh` moves BEHAVIOUR-ONLY -> MUTATION in the guard inventory, which the manifest's own UNDECLARED note called "the most valuable upgrade on this list". The declared clause is in the GATE, not the script: rewriting `H10_REVIEWERS` while the posting account stays fixed reddens the accept path only if the writer reads the list live AND the comparison gates the outcome. Disarming the comparison instead would prove the second and say nothing about the first, which is the half that was missing. Verification: full `scripts/tests` suite green (1239 passed, 2 skipped); the declared mutation executes and reddens its named proof with the declared diagnostic; four clauses disarmed by hand and each confirmed to redden its own test. fixes #845 Decisions-Edit: yes Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019zUmJZHhVP7kXg5DV237TW	2026-08-30 03:38:47 +02:00
timothy	761e575836	fix(787): derive the dropped-step guard's scope, and reconcile its snapshot against the server (#861) ... Co-authored-by: Timothy <timothy@noreply.gitea.tblindustries.be>	2026-08-27 22:37:02 +00:00
timothy	469d19852c	fix(788): one declarative H10 verdict vocabulary, derived by both sides (#846) ... The verdict words lived in two hand-written shell copies — the `case` arms of post-review-verdict.sh (write) and the POS_RE/NEG_RE regexes of check-review-verdict.sh (read) — held together by nothing but a comment that had already gone stale. scripts/lib/review-verdict-vocabulary.sh now declares them once and both sides derive; neither script enumerates a verdict word any more. Only the WORD SET moved. The grammar stays in check-review-verdict.sh, where every #629 false-open actually lived. No parity test: #774 shipped one and withdrew it after six rounds, because a regex over shell source is not a shell parser. The proof is behavioural and graded MUTATION — the harness restores the pre-#788 hardcoded POS_RE each run and requires it to redden. Enforcement is a DATA dependency, not a control-flow gate. Review round 1 found a real fail-open in the first commit: `${#arr[@]}` is nounset-safe only for a declared-empty array, and under `set -u` that error inside a function called as `if ! validate` skips BOTH branches — so on the reader (deliberately no `set -e`) an explicit BLOCKED @ head classified `positive`, exit 0. Validation now sets a sentinel on its last line and the derived views refuse without it. Six cold review rounds; rounds 2-6 found no fail-open across differential fuzzing (4788 / 2612 / 7560 payloads, zero divergences from origin/main's grammar), sentinel forgery, environment poisoning, declare -p evasion on bash 5.3 and 3.2, path/symlink resolution and probe TOCTOU. Every malformation fails closed: reader exit 2, writer exit 1 with nothing posted. Also corrected: CLAUDE.md and release.review-verdict-gate both enumerated the vocabulary without LGTM, a word the code has accepted since #629. fixes #788 Co-authored-by: Timothy <timothy@noreply.gitea.tblindustries.be>	2026-08-26 20:58:09 +00:00