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
timothyandClaude Opus 5
f54667cc5a