timothy/ersatztv
1
0
Fork 0
Code Issues 58 Pull Requests 8 Actions Packages Projects Releases 9 Wiki Activity

One shared verdict vocabulary instead of two shell copies plus a parity test #788

New Issue
Closed
opened 2026-08-13 22:21:30 +02:00 by timothy · 3 comments
timothy commented 2026-08-13 22:21:30 +02:00
Owner
Copy Link
Copy Source

Raised by the Codex cold review of #774. The H10 verdict vocabulary lives in two hand-written shell copies — post-review-verdict.sh's case arms (write) and check-review-verdict.sh's POS_RE/NEG_RE (read). #774 shipped scripts/tests/test_review_verdict_vocabulary_parity.py, which extracts both with regexes and asserts set equality.

STATUS (2026-08-13): the interim test described below was WITHDRAWN — see the comment on this issue. The duplication is currently UNMITIGATED and only a comment in post-review-verdict.sh holds the two lists together. The body below is kept for the reasoning; read the strikethrough as history, not as current state.

That is detector A applied to a duplication that detector C says should not exist. The reviewer's objection is correct on the merits: regex-scraping two executable shell representations cannot prove semantic equality, only textual agreement in the shapes the extractors happen to model. #774 already had to patch one hole (a SHIP*) arm the strict pattern could not read, leaving a non-empty set identical to the read side) and add a single-assignment check (the extractor unions POS_RE= assignments while the classifier runs the last one). Each patch is a round in the string-predicate treadmill this repo has budgeted 3+ rounds for before.

The end state is one declarative source both scripts consume — a small file sourced by both, or a generated fragment.

Why it was not done in #774: introducing a shared-sourcing convention between two scripts on the merge-consent path is a larger change to a security-relevant surface than the drift it prevents, and it deserves its own review rather than an addendum at the end of a long session.

The parity test is the interim measure and should be DELETED by this issue, not kept alongside. Superseded: #774 deleted it after six review rounds (see the comment). There is nothing left for this issue to remove — it now has to BUILD the shared vocabulary, with no interim guard standing in the meantime.

Done-when

Raised by the Codex cold review of #774. The H10 verdict vocabulary lives in two hand-written shell copies — `post-review-verdict.sh`'s `case` arms (write) and `check-review-verdict.sh`'s `POS_RE`/`NEG_RE` (read). #774 shipped `scripts/tests/test_review_verdict_vocabulary_parity.py`, which extracts both with regexes and asserts set equality. > **STATUS (2026-08-13): the interim test described below was WITHDRAWN — see the comment on this issue. The duplication is currently UNMITIGATED and only a comment in `post-review-verdict.sh` holds the two lists together. The body below is kept for the reasoning; read the strikethrough as history, not as current state.** **That is detector A applied to a duplication that detector C says should not exist.** The reviewer's objection is correct on the merits: regex-scraping two executable shell representations cannot prove semantic equality, only textual agreement in the shapes the extractors happen to model. #774 already had to patch one hole (a `SHIP*)` arm the strict pattern could not read, leaving a non-empty set identical to the read side) and add a single-assignment check (the extractor unions `POS_RE=` assignments while the classifier runs the last one). Each patch is a round in the string-predicate treadmill this repo has budgeted 3+ rounds for before. The end state is one declarative source both scripts consume — a small file sourced by both, or a generated fragment. **Why it was not done in #774:** introducing a shared-sourcing convention between two scripts on the merge-consent path is a larger change to a security-relevant surface than the drift it prevents, and it deserves its own review rather than an addendum at the end of a long session. ~~The parity test is the interim measure and should be DELETED by this issue, not kept alongside.~~ **Superseded:** #774 deleted it after six review rounds (see the comment). There is nothing left for this issue to remove — it now has to BUILD the shared vocabulary, with no interim guard standing in the meantime. ## Done-when - [x] One declarative vocabulary, consumed by both scripts - [x] ~~`test_review_verdict_vocabulary_parity.py` deleted, its inventory row removed~~ — done in #774 - [x] A mutation proof that a word added to the shared source reaches both sides - [x] Adversarial review passed
timothy added the ci-cdpriority: medium labels 2026-08-13 22:21:30 +02:00
timothy commented 2026-08-13 23:07:52 +02:00
Author
Owner
Copy Link
Copy Source

Update (2026-08-13, from #774): the interim mitigation named in this issue has been withdrawn, so the duplication is currently unmitigated.

scripts/tests/test_review_verdict_vocabulary_parity.py extracted both vocabularies from shell source with regexes and compared them. Six cold-review rounds each found another shell construction that either escaped the extractor or made it red on a correct tree:

Round Construction
1 SHIP*) — glob in the arm label, invisible to `[A-Z
2 indented later POS_RE= reassignment; alternative outside the captured group
3 #FOO) state=bar — compact comment false-RED
4 SHIP-IT) state=success — unquoted value, missed by BOTH strict and loose
5 <word>) state= in a heredoc / unrelated case — false-RED
6 column-zero esac in a string truncates the scoped match, silently dropping a real arm

Each fix was locally correct and the sequence converged on nothing, because a regex over shell source is not a shell parser. It was deleted rather than patched a seventh time — see docs/decisions/records/testing/guard-derives-population-from-source.md, which now carries this as the worked example of "a weak detector is itself the symptom-keyed mistake".

This raises the priority of the real fix. Only a comment in post-review-verdict.sh now holds the two lists together, and that comment had already gone stale once (it named .claude/hooks/pretooluse-merge-consent.sh as the home of the regex; the hook carries no copy and delegates to check-review-verdict.sh).

Do not reintroduce a text-comparison test as a stopgap. The lesson of those six rounds is that this specific shape does not converge; dedup by construction is the only thing that closes it.

**Update (2026-08-13, from #774):** the interim mitigation named in this issue has been **withdrawn**, so the duplication is currently **unmitigated**. `scripts/tests/test_review_verdict_vocabulary_parity.py` extracted both vocabularies from shell source with regexes and compared them. Six cold-review rounds each found another shell construction that either escaped the extractor or made it red on a correct tree: | Round | Construction | |---|---| | 1 | `SHIP*)` — glob in the arm label, invisible to `[A-Z|-]` | | 2 | indented later `POS_RE=` reassignment; alternative outside the captured group | | 3 | `#FOO) state=bar` — compact comment false-RED | | 4 | `SHIP-IT) state=success` — unquoted value, missed by BOTH strict and loose | | 5 | `<word>) state=` in a heredoc / unrelated case — false-RED | | 6 | column-zero `esac` in a string truncates the scoped match, silently dropping a real arm | Each fix was locally correct and the sequence converged on nothing, because a regex over shell source is not a shell parser. It was deleted rather than patched a seventh time — see `docs/decisions/records/testing/guard-derives-population-from-source.md`, which now carries this as the worked example of "a weak detector is itself the symptom-keyed mistake". **This raises the priority of the real fix.** Only a comment in `post-review-verdict.sh` now holds the two lists together, and that comment had already gone stale once (it named `.claude/hooks/pretooluse-merge-consent.sh` as the home of the regex; the hook carries no copy and delegates to `check-review-verdict.sh`). Do **not** reintroduce a text-comparison test as a stopgap. The lesson of those six rounds is that this specific shape does not converge; dedup by construction is the only thing that closes it.
timothy added the in-progress label 2026-08-26 19:40:08 +02:00
timothy commented 2026-08-26 19:40:09 +02:00
Author
Owner
Copy Link
Copy Source

Claiming this (Claude Code session, 2026-08-26).

Pre-claim checks per process.parallel-session-claim, all four run against fresh state:

  • No open PR referencing #788 (open PRs: #841, #829, #828, #805, #804, #761 — none reference it).
  • No remote branch naming 788 (git ls-remote --heads origin).
  • Comments read: the 2026-08-13 update from #774 (interim parity test WITHDRAWN, duplication currently unmitigated).
  • Fresh git fetch origin main @ 35affecd2.

Three other sessions are active on #746, #734 and #732 — all disjoint from this scope (docker-build.yml, SPA screens).

Scope I am NOT folding in: siblings #742 and #763 both touch .gitea/workflows/review-verdict.yml. This issue is about scripts/post-review-verdict.sh + scripts/check-review-verdict.sh sharing one declarative vocabulary; its own body argues this surface deserves its own review rather than a bundled addendum, so they stay separate.

Holding the line from the #774 comment: no text-comparison stopgap. Dedup by construction, plus a mutation proof that a word added to the shared source reaches both sides.

Claiming this (Claude Code session, 2026-08-26). Pre-claim checks per `process.parallel-session-claim`, all four run against fresh state: - No open PR referencing #788 (open PRs: #841, #829, #828, #805, #804, #761 — none reference it). - No remote branch naming 788 (`git ls-remote --heads origin`). - Comments read: the 2026-08-13 update from #774 (interim parity test WITHDRAWN, duplication currently unmitigated). - Fresh `git fetch origin main` @ `35affecd2`. Three other sessions are active on #746, #734 and #732 — all disjoint from this scope (`docker-build.yml`, SPA screens). **Scope I am NOT folding in:** siblings #742 and #763 both touch `.gitea/workflows/review-verdict.yml`. This issue is about `scripts/post-review-verdict.sh` + `scripts/check-review-verdict.sh` sharing one declarative vocabulary; its own body argues this surface deserves its own review rather than a bundled addendum, so they stay separate. Holding the line from the #774 comment: **no text-comparison stopgap.** Dedup by construction, plus a mutation proof that a word added to the shared source reaches both sides.
timothy commented 2026-08-26 22:59:18 +02:00
Author
Owner
Copy Link
Copy Source

Closing record

Outcome: Shipped — PR #846, squash-merged as 469d19852. scripts/lib/review-verdict-vocabulary.sh declares the H10 verdict words once as two bash arrays; post-review-verdict.sh classifies through etv_verdict_class and check-review-verdict.sh builds POS_RE/NEG_RE from etv_verdict_alternation. Neither script enumerates a verdict word any more — including the usage banner and the unknown-word error, which were themselves further copies. Only the WORD SET is shared; the grammar (column-0 anchor, whole-word boundary, anchored @ <sha> field) stays in the classifier, where every #629 false-open actually lived.

Root cause: The vocabulary was duplicated because the read and write sides were written at different times against different needs — a case statement wants words, a classifier wants a regex — and the shapes are different enough that neither author saw a shared representation. What kept it alive was that the duplication is invisible until the two disagree, and they had not yet.

Decisions/conventions changed: Added release.verdict-vocabulary-shared. Corrected release.review-verdict-gate, whose rule: (and hence the catalog) enumerated the vocabulary without LGTM — a word the code has accepted since #629. Same omission fixed in CLAUDE.md and one merge-consent-hook comment.

Reusable knowledge — three things worth carrying:

  1. A set -u abort inside a function skips BOTH branches of if ! f. This shipped a real fail-open in the first commit and cold review caught it. ${#arr[@]} is nounset-safe only for a declared-empty array — on an unset name or a scalar it is fatal — and because check-review-verdict.sh deliberately runs without set -e, the abort skipped validation and let unvalidated words reach the regex: an explicit BLOCKED @ <head> classified positive, exit 0, with the only signal on a stderr the hook discards. The general form: a control-flow gate can be skipped by an abort; a data dependency cannot. Validation now sets a sentinel on its last line and the derived views refuse without it.
  2. Two guards on one condition hide each other, and the second one's "proof" can be a count. The declare -p array assertion removed the abort, so validation began returning 1 cleanly and the ordinary if fired — meaning every refusal test passed with the sentinel deleted. 150 tests stayed green with the headline mechanism removed. The fix was to disarm the other guard first, so the sentinel is the only thing left standing. Of the three sentinel copies only the read side's has a behavioural alone-deletion red; that is now stated rather than implied.
  3. A count in prose rots exactly like the duplicated list it describes. The number of prose vocabulary copies was wrong three times running (one → two → two, while a third sat four lines away), then a pattern replacing the count still missed the generated catalog, because a Markdown table escapes its pipes. Every attempt asserted a completeness nobody had established. The end state is a re-runnable sweep with its completeness claim explicitly withdrawn — it finds separator-joined enumerations, it is better than a number because it can be re-run, and it is not evidence that no other copy exists.

Verification: 989 passed / 2 skipped (pytest scripts/tests); ruff, ruff format --check, shellcheck clean; catalog regenerated; decisions-validate: OK; full CI green on e66196f. Behavioural equivalence: a 4788-case differential fuzz of the new classifier against origin/main's found 0 divergences, and independent reviewers ran further 2612- and 7560-payload fuzzes, also 0 — including every #629 false-open case. Fail-closed matrix: missing, unreadable, truncated, syntax error, top-level exit, unset array, scalar list, duplicate word, empty list, regex metacharacter and glob all refuse on both sides — reader exit 2, writer exit 1 with nothing posted, checked against a POST-logging curl stub. The declared clause mutation is executed by the repo's harness every run.

Review: six cold adversarial rounds, each from a fresh review-only brief in an isolated worktree; the author reviewed none of their own work. Round 1 found the fail-open above. Rounds 2–6 found no fail-open; what they kept finding was inaccurate prose — every commit carried at least one false statement, several introduced while correcting false statements. A Codex cross-family pass was attempted and is not counted: it exhausted its account quota before reaching a verdict, though its partial run did independently confirm behavioural equivalence and the absence of glob/split defects.

Deferred: Prose copies of the vocabulary remain in the merge-consent hook and a few docs. They are inert — nothing outside the library classifies a word — and the record carries the sweep that re-derives them rather than a number. The library is located relative to ${BASH_SOURCE[0]} without resolving symlinks, so a script reached through a symlink looks beside the link; that fails closed and is recorded as a known limitation. Siblings #742 and #763 (both review-verdict.yml) were deliberately not bundled — this issue's own body argues the merge-consent surface deserves its own review.

Also observed, not fixed: test_hook_fire_log.py::test_the_suite_does_not_write_to_the_PRODUCTION_log failed once and did not reproduce on a re-run of the identical tree; an independent reviewer saw the same failure and the same non-reproduction. It compares mtimes of the shared ~/.cache/ersatztv/hook-fire/ directory across a window, so any concurrent writer fails it — a concurrency manifestation distinct from the stale-env one #809 describes. Noted on #809 rather than fixed here.

Docs updated: docs/ci-cd.md (Review-verdict gate), new docs/decisions/records/release/verdict-vocabulary-shared.md + regenerated catalog, docs/guard-inventory.md (new row + summary counts), docs/remote-state-inventory.md, plus the LGTM corrections in CLAUDE.md and release.review-verdict-gate.

## Closing record **Outcome:** Shipped — PR #846, squash-merged as `469d19852`. `scripts/lib/review-verdict-vocabulary.sh` declares the H10 verdict words once as two bash arrays; `post-review-verdict.sh` classifies through `etv_verdict_class` and `check-review-verdict.sh` builds `POS_RE`/`NEG_RE` from `etv_verdict_alternation`. Neither script enumerates a verdict word any more — including the usage banner and the unknown-word error, which were themselves further copies. Only the WORD SET is shared; the grammar (column-0 anchor, whole-word boundary, anchored `@ <sha>` field) stays in the classifier, where every #629 false-open actually lived. **Root cause:** The vocabulary was duplicated because the read and write sides were written at different times against different needs — a `case` statement wants words, a classifier wants a regex — and the shapes are different enough that neither author saw a shared representation. What kept it alive was that the duplication is invisible until the two disagree, and they had not yet. **Decisions/conventions changed:** Added `release.verdict-vocabulary-shared`. Corrected `release.review-verdict-gate`, whose `rule:` (and hence the catalog) enumerated the vocabulary **without `LGTM`** — a word the code has accepted since #629. Same omission fixed in `CLAUDE.md` and one merge-consent-hook comment. **Reusable knowledge — three things worth carrying:** 1. **A `set -u` abort inside a function skips BOTH branches of `if ! f`.** This shipped a real fail-open in the first commit and cold review caught it. `${#arr[@]}` is nounset-safe only for a *declared-empty* array — on an unset name or a scalar it is fatal — and because `check-review-verdict.sh` deliberately runs without `set -e`, the abort skipped validation and let unvalidated words reach the regex: an explicit `BLOCKED @ <head>` classified **`positive`, exit 0**, with the only signal on a stderr the hook discards. The general form: **a control-flow gate can be skipped by an abort; a data dependency cannot.** Validation now sets a sentinel on its last line and the derived views refuse without it. 2. **Two guards on one condition hide each other, and the second one's "proof" can be a count.** The `declare -p` array assertion removed the abort, so validation began returning 1 cleanly and the ordinary `if` fired — meaning every refusal test passed with the sentinel deleted. 150 tests stayed green with the headline mechanism removed. The fix was to disarm the *other* guard first, so the sentinel is the only thing left standing. Of the three sentinel copies only the read side's has a behavioural alone-deletion red; that is now stated rather than implied. 3. **A count in prose rots exactly like the duplicated list it describes.** The number of prose vocabulary copies was wrong three times running (one → two → two, while a third sat four lines away), then a *pattern* replacing the count still missed the generated catalog, because a Markdown table escapes its pipes. Every attempt asserted a completeness nobody had established. The end state is a re-runnable sweep with its completeness claim **explicitly withdrawn** — it finds separator-joined enumerations, it is better than a number because it can be re-run, and it is not evidence that no other copy exists. **Verification:** 989 passed / 2 skipped (`pytest scripts/tests`); ruff, `ruff format --check`, shellcheck clean; catalog regenerated; `decisions-validate: OK`; full CI green on `e66196f`. **Behavioural equivalence:** a 4788-case differential fuzz of the new classifier against `origin/main`'s found **0 divergences**, and independent reviewers ran further 2612- and 7560-payload fuzzes, also 0 — including every #629 false-open case. **Fail-closed matrix:** missing, unreadable, truncated, syntax error, top-level `exit`, unset array, scalar list, duplicate word, empty list, regex metacharacter and glob all refuse on both sides — reader exit 2, writer exit 1 with nothing posted, checked against a POST-logging curl stub. The declared clause mutation is executed by the repo's harness every run. **Review:** six cold adversarial rounds, each from a fresh review-only brief in an isolated worktree; the author reviewed none of their own work. Round 1 found the fail-open above. Rounds 2–6 found no fail-open; what they kept finding was inaccurate prose — every commit carried at least one false statement, several introduced *while correcting* false statements. A Codex cross-family pass was attempted and is **not** counted: it exhausted its account quota before reaching a verdict, though its partial run did independently confirm behavioural equivalence and the absence of glob/split defects. **Deferred:** Prose copies of the vocabulary remain in the merge-consent hook and a few docs. They are inert — nothing outside the library classifies a word — and the record carries the sweep that re-derives them rather than a number. The library is located relative to `${BASH_SOURCE[0]}` without resolving symlinks, so a script reached through a symlink looks beside the link; that fails closed and is recorded as a known limitation. Siblings #742 and #763 (both `review-verdict.yml`) were deliberately **not** bundled — this issue's own body argues the merge-consent surface deserves its own review. **Also observed, not fixed:** `test_hook_fire_log.py::test_the_suite_does_not_write_to_the_PRODUCTION_log` failed once and did not reproduce on a re-run of the identical tree; an independent reviewer saw the same failure and the same non-reproduction. It compares mtimes of the shared `~/.cache/ersatztv/hook-fire/` directory across a window, so any concurrent writer fails it — a concurrency manifestation distinct from the stale-env one #809 describes. Noted on #809 rather than fixed here. **Docs updated:** `docs/ci-cd.md` (Review-verdict gate), new `docs/decisions/records/release/verdict-vocabulary-shared.md` + regenerated catalog, `docs/guard-inventory.md` (new row + summary counts), `docs/remote-state-inventory.md`, plus the `LGTM` corrections in `CLAUDE.md` and `release.review-verdict-gate`.
timothy removed the in-progress label 2026-08-26 22:59:35 +02:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
ad-hoc
One-off / ad-hoc work not tracked by a dedicated issue
 api
REST API / HTTP endpoints
 bug
Something isn't working
 ci-cd
Build, test, deploy pipeline
 content
Channel content / schedules / playlists
 dependencies
Dependency updates (Renovate)
 enhancement
New feature or improvement
 frontend
ChicoryTV React SPA frontend
 in-progress
Claimed by an active session — do not pick up
 jellyfin
Jellyfin tuner / IPTV integration
 parked
Excluded from automatic queue pickup; work only when explicitly selected
 priority: high
priority: low
priority: medium
review
Adversarial review finding
 security
Security / vulnerability fix
No labels ci-cd priority: medium
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
renovate timothy (Timothy)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: timothy/ersatztv#788