Commit Graph
7 Commits
Author SHA1 Message Date
timothyandtimothy 469d19852c fix(788): one declarative H10 verdict vocabulary, derived by both sides (#846)
Build ErsatzTV Image / CI toolchain image resolves (push) Successful in 6s
Build ErsatzTV Image / Delimiter ban (release path) (push) Successful in 25s
Build ErsatzTV Image / Build & test (.NET) (push) Successful in 8m34s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (push) Successful in 6m17s
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (push) Successful in 5m50s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (push) Skipped
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (push) Skipped
Build ErsatzTV Image / Build & push image (amd64) (push) Successful in 4m22s
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
timothyandClaude Opus 5 50bcd7b0c7 fix(629): strip raw HTML blocks, and state where the hardening stops
Review verdict / Set review-verdict status (pull_request) Successful in 2s
PR Gates / CI image pin matches docker/ci (pull_request) Successful in 15s
PR Gates / Docs update reminder (pull_request) Successful in 16s
PR Gates / decisions lifecycle (pull_request) Successful in 18s
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (pull_request) Successful in 6m7s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Successful in 10s
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Successful in 7s
review-verdict/h10 Review-verdict: MERGEABLE @ 50bcd7b
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 19m51s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 25m5s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped
Round 5: raw HTML is the third code-block form. `<pre>`, `<code>` and HTML comments all render their
contents literally, so a verdict inside one is an example, not an approval:

  <pre> / <code> / <!-- ... -->  containing a verdict  ->  positive

Now stripped, tracked as a marker count rather than parsed — the direction of error is to strip MORE,
which can only ever withhold approval. Mutation-verified: removing the stripper fails all five cases.

AND THE HARDENING STOPS HERE, deliberately. The record now says so, because otherwise the next
session re-derives it: this is a best-effort heuristic, not a markdown parser. It covers the three
code-block forms markdown has (fenced; indented, via the column-0 rule; raw HTML) and is not proof
against every way to render text as non-prose.

Stopping is safe because the comment is NOT the load-bearing gate. Since #622 the authoritative
signal is the `review-verdict/h10` commit status, written only by post-review-verdict.sh from
explicit arguments — a comment cannot forge it. This classifier is condition (c) of the PreToolUse
hook: defense in depth on an agent's merge call. A residual false-open means the hook does not
object; it does not mean a merge happens.

Five rounds found five code-block forms, four of them introduced while fixing the previous round.
The generalisable rule, now in the record: when a heuristic keeps failing at the edges, check whether
it is actually the thing enforcing the invariant before spending another round on it.

Also measured, against the real corpus: a "verdict must be the first line" rule would have killed
every code-block form at once, but 14 of 18 verdict markers ever posted in this repo are NOT on the
first line — so it was rejected as a retroactive break, not deferred.

178 tests. refs #629

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Decisions-Edit: yes
2026-07-26 01:14:59 +02:00
timothyandClaude Opus 5 28a9d0dbfd fix(629): round-4 — require the marker at COLUMN 0, narrowing the grammar instead of patching again
review-verdict/h10 Awaiting review verdict for 28a9d0d
Review verdict / Set review-verdict status (pull_request) Successful in 3s
PR Gates / CI image pin matches docker/ci (pull_request) Successful in 11s
PR Gates / Docs update reminder (pull_request) Successful in 12s
PR Gates / decisions lifecycle (pull_request) Successful in 16s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 8m39s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Successful in 8s
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Successful in 7s
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (pull_request) Successful in 17m24s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 23m58s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped
Round 4 found the last false-open: markdown has a SECOND code-block form the fence stripper does
not cover — indented blocks (4 spaces or a tab). A pasted indented example still self-approved:

    Example:

        Review-verdict: MERGEABLE @ <head>     ->  positive

Adding an indented-code stripper would be the same move that produced rounds 2, 3 and 4: fixing the
instance, not the class. So the grammar is narrowed instead — the marker must sit at COLUMN 0. That
kills every indentation-based ambiguity at once (4-space, tab, list-nested, arbitrary indent).

Cost, accepted deliberately: a verdict indented under a list item is now ignored and classifies
`absent`, which asks a human. For a gate, erring toward ignoring is the safe direction. Fence
detection KEEPS its leading-whitespace tolerance, because stripping more is always safe.

`test_leading_indent_is_tolerated` asserted the old behaviour and is replaced by
`test_falseopen_an_indented_verdict_is_not_a_verdict`, parameterised over four indent shapes and
mutation-verified: restoring `^[[:space:]]*` fails all four, control green. 172 tests.

Round 4 verified clean by execution: the rc plumbing fails closed for a forced failure in the inner
jq, awk, grep AND the pipeline producer (rc 3/4/5/93 -> exit 2); fence-length semantics, mismatched
markers, CRLF fences, 10-marker fences, blockquote fences; emoji, CRLF, a 120k line, 200 comments;
NUL rejection with no JSON-encoding bypass.

refs #629

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Decisions-Edit: yes
2026-07-26 00:52:11 +02:00
timothyandClaude Opus 5 299e7b27af fix(629): round-3 fixes — fence LENGTH semantics, and stop masking reader failures
review-verdict/h10 Awaiting review verdict for 299e7b2
Review verdict / Set review-verdict status (pull_request) Successful in 3s
PR Gates / CI image pin matches docker/ci (pull_request) Successful in 12s
PR Gates / Docs update reminder (pull_request) Successful in 17s
PR Gates / decisions lifecycle (pull_request) Successful in 25s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 8m10s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Successful in 10s
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Successful in 18s
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (pull_request) Successful in 17m21s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 22m25s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped
Third review round, third set of real findings. Both reproduced before fixing.

1. High: markdown closes a fence only with N-or-more of the SAME marker it was opened with, so a
   ```` block legitimately CONTAINS a ``` line as content. Toggling on any 3+ marker left the
   fence at that inner line and graded the verdict below it as a real approval:

     ````text / ``` / Review-verdict: MERGEABLE @ <head> / ````   ->  positive

   Now tracks the opening marker's character and length; a shorter or different marker while a
   fence is open is content, so it neither closes the fence nor escapes it.

2. Medium: `awk ... | grep ... || true` flattened "no match" (grep rc 1, normal) together with a
   real tool failure (rc >= 2). A failing reader produced no verdict lines at all — `absent` —
   silently discarding a real BLOCKED verdict. awk and grep are now checked separately, and only
   "no match" is tolerated.

Also fixed while writing (2): `[ rc = 0 ] && printf` as the loop body's LAST command would leave
the subshell exiting 1 whenever the newest comment carried no verdict, which the rc check would
then report as a failure to read comment bodies — an ordinary PR reading as broken. Uses an `if`.

169 tests. Both findings mutation-verified: restoring the naive fence toggle fails all four
longer-fence cases, restoring `|| true` fails the failing-grep case, control green. Verified the
ordinary shapes still work: plain ``` and ~~~ fences and lang-tagged fences still stripped, an
unclosed fence still swallows, a real verdict beside a fenced example still counts, and a fenced
positive alongside a real BLOCKED still classifies negative.

refs #629

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Decisions-Edit: yes
2026-07-26 00:47:40 +02:00
timothyandClaude Opus 5 f151b93245 fix(629): review fixes — tilde fences, an unbounded sha field, and a forgeable comment boundary
Cross-family review of 38a96f47 returned BLOCKED with three findings. All reproduced first:

  ~~~ fence           -> positive   fences were stripped for ``` only; markdown also takes ~~~
  @ <40hex>f / ZZZ    -> positive   the sha matched {7,40} with NO right boundary, so an
                                    over-long or malformed token was TRUNCATED into a passing one
  \x01BODY-BOUNDARY\x01 -> positive an in-band separator joined comment bodies, so a body
                                    containing that line forged a boundary, reset fence state
                                    mid-comment, and exposed a verdict inside an unclosed fence

Fixes: both fence markers honoured; the hex run matched whole, required to end at a
non-alphanumeric boundary, with its length validated separately so an out-of-range token is
rejected rather than trimmed to fit; and bodies carried OUT-OF-BAND (one JSON-encoded string
per line), which removes the forgery class instead of escaping the sentinel.

The third is the one worth remembering: an in-band delimiter is forgeable by whoever writes the
data, and here that is anyone who can comment on the PR.

Two of these fixes broke previously-green tests, both of which were right to break:
- an over-long token now classifies `no-sha`, not `stale`. The fixture asserting `stale` was 45
  hex chars, so it had been exercising the length guard while claiming to test the prefix rule.
  Rebuilt as a well-formed 40-char sha that contains the head prefix without starting with it.
- `jq -e` exits 4 when a filter produces NO output, which is the legitimate empty-comment-list
  case. Treating it as an error turned "no comments yet" into an input error — and callers fail
  closed on those, so a new PR would have read as unclassifiable. Exit 4 is now accepted.

44 classifier tests, 155 total. `~~~` and the boundary fixes are each mutation-verified; the
out-of-band fix has no equivalent mutation (it is structural, not a regex) so its evidence is the
direct reproduction against the previous commit.

refs #629

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Decisions-Edit: yes
2026-07-26 00:47:40 +02:00
timothyandClaude Opus 5 0f83b54334 fix(629): close three false-opens in the H10 verdict grammar, and give it tests
The H10 classification lived inline in `pretooluse-merge-consent.sh` with no tests. Three
protections the `release.review-verdict-gate` record described were never actually
implemented, and each graded an unreviewed head as approved. All three reproduced first:

  1 MERGEABLE-LATER   -> positive     the token was prefix-matched, so any word STARTING
                                      with mergeable/approved/lgtm passed
  2 fenced code block -> positive     the line-start anchor is satisfied inside ```, so
                                      documentation showing the convention was a verdict
  3 URL-borne sha     -> positive     the sha came from the first `@<hex>` ANYWHERE on the
                                      line, so a markdown link could supply it

Fixes: whole-word token matching, with a token in neither vocabulary classified `unknown`
(never positive, and not guessed into a block either — it goes to a human); fenced blocks
stripped with fence state reset per comment body; the sha read from the verdict's OWN
`@ <sha>` field, which also makes multi-`@` lines unambiguous.

The grammar moves to `scripts/check-review-verdict.sh` so it can be tested at all — 38 tests,
and each fix mutation-verified: restoring the old regex/extraction makes exactly the
corresponding test fail, control green.

#629's fourth reported item is NOT a defect and is not claimed as a fix. A later `@ <head>`
on a BLOCKED line was reported as "masking a negative"; under the documented grammar that
line is a verdict for the sha in its own field, so `stale` is correct — and was correct
before this change too. Kept as a characterization test.

`test_post_review_verdict.py`'s cross-check re-implemented the hook's regexes in Python and
asserted the shell still contained them. That mirror is removed: it is the same duplication
that let these three survive, and a Python copy would keep passing while the shell drifted.
It now runs the real classifier.

The decision record is corrected — it asserted the URL protection this commit actually adds.

Note: the active corpus is 5637 lines against a 5600 budget, so the validator emits its
consolidation warning (non-blocking). That is #620's subject, not regressed here.

fixes #629

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Decisions-Edit: yes
2026-07-26 00:47:40 +02:00
timothy fba5233caf feat(610): split the decision corpus into one YAML-frontmatter file per record
PR Gates / CI image pin matches docker/ci (pull_request) Successful in 11s
PR Gates / Docs update reminder (pull_request) Successful in 16s
PR Gates / decisions lifecycle (pull_request) Failing after 23s
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Successful in 1m17s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Successful in 1m29s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 8m5s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 16m5s
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (pull_request) Successful in 17m6s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped
168 records -> docs/decisions/records/<area>/<topic>.md (163 active, 23 dirs) and
docs/decisions/archive/<area>/<topic>.md (5 archived). The filename IS the key,
so one-active-record-per-key becomes a filesystem property rather than a
validator check, and supersession becomes a `git mv`.

WHY: the monolith was a concurrency problem before an aesthetic one. A
3,900-line append target made parallel sessions collide -- PR #605 and PR #614
both hit append-vs-append conflicts during routine rebases, and hand-resolving
those inside the corpus is exactly the operation the rationale-rewrite guard
exists to police.

HOW IT IS VERIFIED: a ~170-file diff cannot be meaningfully read, so correctness
does not rest on reading it. The parser was taught BOTH formats first, so the
body-diff guard parses the old form at the merge-base and the new form at head --
the migration validates itself, no bypass. The proof is a field-level equivalence
harness: 168 records before and after, zero lost, zero gained, zero field
mismatches, zero rationale bodies differing. Reviewers should scrutinise the
harness; it is the actual evidence.

What measuring caught that reading would not have:

- ~500 lines sit OUTSIDE any record -- decisions.md's lifecycle schema and each
  topic file's preamble, mostly the only copy. Source files are kept and
  stripped, never deleted. They also cannot be filed per-area: topic files hold
  several areas and 4 of 23 areas span several files.
- Archive discovery was a non-recursive glob; after the split it found ZERO
  archived records, surfacing as four bogus "supersedes points to unknown key"
  errors rather than an obvious failure.
- ~32 live docs point into the corpus BY DATE, which the split dangles. Each
  stripped file now ends with a generated "Records formerly in this file" index,
  which also rescues the identical breadcrumbs in old issue comments.
- decisions.md's "In this file:" list was 97 same-file anchor bullets that the
  split makes WRONG, not merely stale. Dropped; the generated index replaces
  them with links that resolve.

The equivalence harness now runs against a checked-in FIXTURE, not the live
corpus. The earlier version migrated the real tree, which made it a one-shot:
the moment the migration landed there was nothing left to move and the tests
failed for reasons unrelated to the code. A fixture keeps them testing the
SCRIPT rather than the repo's current state.

Keys preserved verbatim, warts included: `sched` (12) and `scheduling` (1) remain
two directories for one concept. Renaming a key is not a move -- it changes
identity, breaks the equivalence proof, and invalidates MemPalace's per-key
drawers. Taxonomy normalisation is separate work.

refs #610
2026-07-25 19:45:09 +02:00