640 Commits

Include renames

Author	SHA1	Message	Date
timothyandClaude Opus 5	a973fc48e2	fix(651): review round 6 — fail closed on a missing ETag; validate elements, not containers ... HIGH: "the draft is only created alongside the ETag" was not the invariant I claimed. The response can OMIT the header, in which case the draft was still created and the subsequent PUT carried no `If-Match` — the round-4 force-write hole in a new form. The editor now fails closed: no concurrency token, no editable draft (error + Retry/Back). Six tests were leaning on ETag-less detail mocks, which is exactly how this stayed invisible; every single-record GET mock now goes through a `detailResponse` helper that always sets one, and the absent case is tested explicitly — asserting zero PUTs are even reachable. MEDIUM: the detail GET had neither deadline nor recovery affordance, so a never-settling request left a bare spinner with no way out. It is now raced against a 15s deadline, the loading view carries a Back control, and the error view offers Retry. MEDIUM: the malformed-body guard checked the container, not the elements. `[null]` passes `Array.isArray`, reaches `setResults`, and throws on `option.id` during render; a wrong-typed `id` would commit an invalid value through `onSelect`. Each element is now validated, and a malformed payload is treated as a failed attempt so it stays retryable rather than cached. MEDIUM: the decision record's `rule:` — the authoritative string, copied verbatim into the catalog that is the documented entry point — still mandated the machinery round 5 deleted: touched-field hydration, `replaceDraft`, conflict reconciliation. Anyone following it would have rebuilt the rejected design. Rewritten to the initialize-once policy and the catalog regenerated; historical prose no longer says `replaceDraft` "is now" separate. MEDIUM: the replacement Reload test resolved its second GET immediately and returned a non-null selection, so it observed neither a pending reload nor the dirty-selection discard — removing `setDraft(null)` could leave it green. It now holds the reload open, asserts the form is ABSENT while pending, and returns `selectedId: null` to pin the case round 3 showed could resurrect a dirty id over a collaborator's change. Checklist item taken from this: when a mechanism is deleted, the decision record's `rule:` is the single most likely thing left stale, and the one string that propagates. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-27 04:25:14 +02:00
timothyandClaude Opus 5	78cd9e0ebf	fix(651): review round 5 — delete the draft-reconciliation layer instead of extending it ... Took the coordinator's proposal. Rounds 2-4 built and rebuilt a layer that merged a late detail response into a draft the user was already editing; it produced a HIGH finding in three consecutive rounds, three of them cross-user lost updates. Round 5's finding was unfixable in kind: `identityConflicts` had no immutable baseline, so it could not tell "the user changed this" from "the server changed this" — giving both a missed conflict (same type, different id: v2 ETag installed over the user's id, third lost update) and a false one (local type switch: dialog opens spuriously, and "Keep editing" leaves `etagRef` null so the next PUT is a silent FORCE-WRITE). The race is removed rather than refereed. `RerunCollectionEditor` now initializes its draft EXACTLY ONCE from the detail GET and withholds the form until it lands; conflicts are detected at save time by the `If-Match` -> 412 -> Reload path that already existed. Deleted: `touchedRef`, `hydrateDraft`, `hydrateSelection`, `hydrateIdentity`, `identityOf`, `identityConflicts`, `replaceDraft`, `replacePending` and the hydrate/replace load mode. Reload simply sets the draft back to null and re-runs the same load, so the form is unmounted while the replacement is in flight — round 4's MEDIUM-4 becomes structural. Two facts make this lossless rather than a regression. The list row could never have helped: `GetPagedRerunCollectionsHandler` applies ZERO `.Include()`s where `GetRerunCollectionByIdHandler` applies fourteen, and both project through the same mapper, so the list response is a strict SUBSET of the detail one — the id round 1 preserved from it is null in production for every row (#671), and existed only in test fixtures. And FillerPresetsScreen/PlaylistsScreen already worked this way; RerunCollections was the outlier, which is why nearly every finding in rounds 3-5 traced to it. The ETag is now written in the same callback that sets the draft, so `draft != null` implies an ETag and a PUT without `If-Match` is unreachable by construction. MEDIUM-2: a failed search retried every debounce forever — a fresh `{ok:false}` re-ran the effect and the success guard declined it. `ok` (is the held answer authoritative) is now separate from `attemptRef` (have we already tried this exact source+query); only an explicit reopen/focus/edit re-arms a retry. MEDIUM-3: a malformed 2xx body resolves as `undefined` (client.ts swallows the SyntaxError), and `setResults(undefined)` threw on the next render. A non-array is now treated as a failed attempt, and `search` is raced against a 10s deadline since a caller-supplied promise carries no abort signal. Two tests were hiding findings and are fixed: the failed-search test pressed Escape before the unintended retry could fire, and the round-4 conflict test claimed to select id 9 while firing the already-selected id 5. Fourteen tests of the deleted machinery are gone with it, replaced by six asserting the new invariants. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-27 04:25:14 +02:00
timothyandClaude Opus 5	f601d957a6	fix(651): review round 4 — an id never travels without its namespace ... Round 4's through-line: rounds 2-3 made HYDRATION treat {collectionType, selectedId, selectedName} as one value, but the fix was applied to the structure that was named rather than to every structure that carries an id. Three more instances of the same class, plus two independent findings. HIGH-1 + HIGH-2 (one change, per the structural directive): every result, option and cached result set now carries its SOURCE, and identity is compared as (type, id). - `SearchPicker` takes a REQUIRED `source` prop and caches results against (source, query), not query text. Keying on text let the round-3 re-query guard SUPPRESS the new source's request and leave the previous namespace's hit clickable under the new label — a Collection id stored as a SmartCollection id. Results from another source are now hidden outright rather than dimmed: they are not stale, they are wrong. Required rather than defaulted, because a default would silently opt every caller out. - `pickerFor` tags list-backed options with the type they were loaded for, on both RerunCollectionsScreen and PlaylistsScreen, so the previous type's rows stop being selectable the instant the active type changes rather than lingering through the replacement load. HIGH-3: a touched identity contradicting the server's type is a CONFLICT. Pinning the user's edit was right; adopting the response's newest ETag alongside it authorized a Save that silently overwrote the collaborator's type change with no 412. The conflict dialog is raised and the stale ETag kept, so even a forced Save 412s. With round 3's Reload defect this is the second cross-user lost update, so it is recorded as a category: never install a save-authorizing ETag over a local edit the server contradicts. MEDIUM-4: the editor is inert while a Reload is pending. The dialog closes immediately, so an edit typed before the replacement landed was silently erased along with the touched set that protected it. MEDIUM-5: cached search provenance records `ok`, so a transient 500 is retried instead of being cached as an authoritative "No matches" that reopening can never clear. Also: `npx tsc --noEmit` typechecks NOTHING in web/ — the root tsconfig is solution-style ("files": [] + references), so it resolves to zero inputs and exits 0. The real gate, and what CI runs, is `npm run typecheck` (tsc -b). Verified by planting a deliberate type error: --noEmit stayed green, -b caught it. Running the real gate surfaced four genuine errors in tests written earlier this branch (a missing required prop and three `never has no call signatures` from closure-assigned mock variables), fixed with the repo's existing holder-object pattern rather than casts. Recorded in spa-conventions so the next session doesn't repeat it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-27 04:25:14 +02:00
timothyandClaude Opus 5	5b0ba08aab	fix(651): review round 3 — cover the class, not the named instance ... Round 3 found four defects that are all one mistake: each earlier fix enumerated one instance of a class instead of covering the class. Fixed by asking what else is in each class rather than patching the named case. CRITICAL — cross-user lost update. Conflict "Reload" ran through the refresh policy with a touched-set reset. Because a reloaded RemoteStream reports `selectedId: null` under the #671 server gap, the keep-ours-when-the-id-is-missing fallback restored the user's DIRTY selection, the fresh ETag was installed, and the next Save silently overwrote the collaborator's change — with the very edits the user had explicitly asked to discard. "Keep ours when the server omits the id" is a REFRESH policy; a reload is a REPLACE. `replaceDraft` is now a separate function and the mode travels with the load, so the two cannot be confused at the call site. HIGH — `collectionType` and the selection still hydrated apart. `collectionType` says which table an id indexes, so it is part of the same indivisible value as the id and its name; splitting it is the identical bug to splitting id from name. A record retyped server-side mid-load hydrated the new type while retaining the old id, displaying and saving a Collection id as a RemoteStream id. All three fields now resolve as one `Identity` unit: either half touched pins the whole thing, a differing type takes the response's unit whole (null selection included), and only once both sides agree on the type does the id/name rule apply. HIGH — stale results were still committable by pointer. Enter was gated and `onClick` was not: the same defect in another modality. The guard moved into the single `choose()` sink so every commit path is covered, including any added later, and a superseded list is now genuinely inert (`aria-disabled` + dimmed) rather than looking normal and silently no-opping. MEDIUM — reopening after Escape re-queried an already-current result set; the duplicate response reset the cursor the user had since moved, leaving Enter doing nothing. The effect now skips the search when the cached results match the trimmed query, and reopening places the cursor per the ARIA APG instead of swallowing the keypress. Also corrects an overstated justification in FillerPresetsScreen: the render-time id check was dropped because there is no reachable path TODAY, not because "every writer sets both" — the initial load writes the id alone, and a stale resolver can repopulate the label after a clear. Same enumeration error as the findings above; the comment now says what is actually true and what to do if a path appears. Note: an apostrophe I introduced inside the single-quoted `rule:` scalar broke PyYAML while `decisions_validate.py` (hand parser) stayed green — the same trap class as the unquoted `#`, caught only by scripts/tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-27 04:25:14 +02:00
timothyandClaude Opus 5	ba52219a9a	fix(651): review round 2 — hydrate untouched fields, never merge a selection field-by-field ... Re-review of a04d9f0b came back BLOCKED: the F1 merge fixed a visible data loss and introduced a silent one. Four blockers, all web-only. BLOCKER 1: `selectedId` and `selectedName` were coalesced independently, but they are one value. Against a Song response (id resolves, name does not), a user selecting a different song while the detail GET was in flight got the NEW name paired with the STORED id — chip read "New Song", Save wrote 42, no error and no visual cue. Strictly worse than the original defect, which at least cleared the field visibly. The same merge resurrected a deliberate clear and reverted a type switch. Replaced with two rules. The id/name pair resolves atomically (no id in the response -> keep ours whole; same id -> its name may fill ours in; different id -> take theirs whole). And hydration loses every race against the user: a `touchedRef`, fed by a single `edit()` funnel so "touched" cannot drift from "changed", limits the refresh to fields the user has not edited; an explicit conflict-reload clears it, since discarding local edits is its entire purpose. Three interleaving tests hold the detail response open, act as the user, then release it. BLOCKER 2: Enter could commit a result from the previous query — highlight Alpha for "Al", retype "Be", press Enter before the debounce. The highlight now drops on input change rather than when the next response happens to arrive, and every keyboard action is gated on the results matching what is typed. The stale list stays visible (hiding it flickers on each keystroke), it just stops being actionable. BLOCKER 3: Escape closed the popup while focus stayed in the input, where `onFocus` can never re-arm it — the picker was dead until the user blurred and refocused. Typing and ArrowDown now both reopen it. BLOCKER 4: the LCG boundary test recomputed the divisor instead of exercising `lcg`, so all three tests passed with the old `/ 0xffffffff`. Since the recurrence is a bijection mod 2^32, the seed whose first step lands on 0xffffffff is solvable exactly (653637408); the tests now drive the real generator into that state. Also: the rerun #id-degrade and re-save tests did not await the refresh, so they were satisfiable from the initial draft; they now await it and re-read live textContent. The Episode/MusicVideo cases are relabelled as error-path guards — a 500 never reaches the hydrate. FillerPresets' render-time id comparison is dropped as unreachable (every writer sets label and id together, and the one async writer refuses a mismatched id); an unreachable guard is an untested one. #671 is referenced from spa-conventions §3b as the server-side root cause the client guard defends against. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-27 04:25:14 +02:00
timothyandClaude Opus 5	e7e425fa25	fix(651): review round 1 — never clear an unnamed id, complete the Lucene escaping, keyboard-operable picker ... Cold cross-family review of 57aefcdf. Six findings, all web-only. F1 (HIGH, data-loss shaped): RerunCollectionsController.ProjectToResponseModel derives BOTH selectedId and selectedName from the same eager-loaded navigation, and GetRerunCollectionByIdHandler loads media metadata only for Show/Season/Artist/Movie while MediaCollections/Mapper maps RemoteStream through `_ => null`. So opening a RemoteStream rerun collection returned HTTP 200 with a null selection and the edit-load refresh CLEARED a stored id, leaving Save permanently disabled. The refresh now merges instead of replacing, so no path can clear an id it merely failed to name; the label degrades to `#id`. Covered per affected type — RemoteStream, Episode, MusicVideo, Song, OtherVideo, Image — plus a re-save assertion. The read-model gaps themselves are server-side and are NOT touched here. F2: `&` and `|` were missing from the escaped set, so `Rock && Roll` compiled with the boolean operator live. Pre-existing in Auto-Tune's original helper, but propagated to three more pickers — and now fixed for Auto-Tune too, since the helper is shared. The test that claimed to cover "every Lucene special" carried its own hand-copied sample and could not see its own omissions; it is now driven per-character off an exported LIBRARY_PICKER_LUCENE_SPECIALS. F3: a slow edit-load name resolution could relabel a newer selection. The label is now keyed to the id it was resolved for AND refuses to overwrite a label naming a different id — keying the render alone stops the mislabelling but discards the correct new label. F4: searchLibraryPickerOptions clamps pageSize instead of merely defaulting it. A bound a caller can exceed is not a bound. F6: replacing a native <select> with an input+listbox dropped keyboard operability. Full ARIA combobox pattern added — role/aria-expanded/aria-controls/aria-autocomplete, Arrow/Home/End over aria-activedescendant, Enter to commit, Escape to dismiss, options as non-tab-stops, cursor reset on each new result set. F7: both is-mounted tests were unsound. React 19 no longer warns on setState-after-unmount and an unmounted tree renders nothing either way, so the DOM assertion could not fail; the hook re-arm test used rerender rather than an effect cleanup. Now: a hook-module mock proving SearchPicker actually reads the guard and sees false, and a StrictMode double-invoke for the re-arm. Both verified by removing the mechanism and watching them fail. Same for the LCG divisor, which now has a direct boundary test. F5 (FillerPresets collection-family names) is filed as #670, not fixed here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-27 04:25:14 +02:00
timothyandClaude Opus 5	fad6805b91	feat(651): media-library pickers resolve by search instead of a bounded window ... The three `getLibraryBrowseItems` pickers (RerunCollectionsScreen, PlaylistsScreen, FillerPresetsScreen) populated a native <select> from a 100-row window over media-library tables that can hold tens of thousands of rows. #644 made that truncation visible; it did not make the picker usable, and paging to completeness would have been worse than the bug (~200 serial requests, each more expensive than the last). They now resolve by SEARCH through the shared `SearchPicker` over a new `searchLibraryPickerOptions` helper: zero requests on mount or on a type switch, at most ONE bounded request (25 rows) per settled query, nothing below 2 characters. Typed text is compiled via the now-shared `titleContainsQuery` (`title:*<escaped>*`) rather than forwarded raw, since the index's default field does not match bare title words. The current selection renders from the owning record — `selectedName` for rerun collections and playlist items, and for filler presets (which store only an id) a single by-id detail read — so editing an existing record can never lose or fail to name its selection. Class A stays put: bounded-by-construction admin lists still page to completeness via `loadAllPages`, and the collection-family filler-preset types keep their bounded single page (their `query` is a SQL LIKE, which a compiled Lucene query would not match). No server-side cap is raised; this is a web-only change. Folded in from #578: the rule-builder facet typeahead arms on focus rather than on mount (an N-rule tree fired N unrequested lookups), both typeaheads pair their `seqRef` guard with a shared `useIsMountedRef`, and the roundtrip test's LCG divides by 2^32 so `pick()` can no longer index one past the end. Decision record `spa.list-completeness-vs-bounded-pickers` is archived as superseded by the new `spa.library-pickers-resolve-by-search`; spa-conventions §3b rewritten to match. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-27 04:25:14 +02:00
timothy	8de02d5bde	Merge pull request 'fix(649): point the ENFORCED review-verdict gate at the shared PR-file enumeration' (#666) from fix/649-enforced-verdict-guard into main	2026-07-26 22:04:06 +00:00
timothy	3885fd6aea	docs(649): narrow the enumeration's stated guarantees to what it actually proves ... Two limitations the cold review surfaced are now written where the guarantees are described, rather than living only in a review transcript. Both are pre-existing and tracked separately (#663, #664); neither is fixed here. - Head-sha binding detects ONE-WAY movement. An A->B->A force-push round trip restores the expected sha, so the binding holds while the pages came from two states. The record previously read as though the race were closed. - A commit status is repo-GLOBAL, so a success earned on one PR is inherited by any other PR with the same head. Same property that makes the per-sha binding work, read from the other end. Refs #649 Decisions-Edit: yes	2026-07-26 23:29:44 +02:00
timothy	d51255a8ef	fix(632): "could not check" is a third outcome, not a quiet synonym for "nothing to check" ... Cold review's substantive finding. The first draft collapsed an unreadable status response into the graceful-adoption path: `vdesc` came back empty, so `recorded_base` was empty, so the comparison was skipped IN SILENCE — and a later, successful status read could then auto-grant, emitting "merge gate: satisfied" for a comparison that never happened. A transient Gitea hiccup is not evidence that the base is unchanged. The unreadable status response and a PR with no resolvable `.base.ref` now both fall through to a human `ask`, leaving exactly one benign silent case: a verdict that predates #632 and could not have carried the field. The emptiness check is done in SHELL before jq sees it, same jq-1.6 rule as the rest of this file. Also from review: the graceful-adoption test asserted only that the decision lacked the issue tag, so it would have passed for a base-specific ask or deny whose wording omitted it — the failure mode most likely to appear when someone edits these messages. It now asserts on the word "base". Recorded rather than fixed, because fixing it would be worse: docs-only PRs exit before this check, since that carve-out short-circuits the gate earlier. It does not auto-grant — it passes through to an ordinary permission prompt — so the exposure is a missing warning on a merge a human is already confirming, not a silent merge. The record now says so instead of implying the deny is unconditional. Mutation-verified: collapsing the unreadable case back into graceful adoption, skipping the check on a missing live base, and dropping the mismatch deny each redden their own test and nothing else. Refs #632	2026-07-26 23:27:22 +02:00
timothy	00e623c066	fix(632): bind a review verdict to its BASE branch, not only to its head sha ... #622 made `review-verdict/h10` a per-sha required status, so a new commit cannot inherit an old verdict — the required context is simply absent on the new head. Retargeting a PR's base reaches the same end from the opposite direction: the head sha and the status both hold still while the merge-base, and therefore the effective diff the verdict was formed against, changes underneath them. #622's record claimed the invariant holds "by construction"; this was the documented exception, and an unrecorded exception is how a guarantee degrades into a habit. `post-review-verdict.sh` now records the base branch in the status description as a trailing `(base: <ref>)`, and refuses to write a status at all if the base moved between reading the PR and posting — the same TOCTOU window the head check already covers, which the head check cannot see because retargeting does not move the head. `pretooluse-merge-consent.sh` reads the field back and denies when it no longer matches the PR's live `base.ref`. Two choices are load-bearing, and each is pinned by a test rather than left to a comment: - The comparator is `base.ref`, NOT `base.sha`. `base.sha` tracks the base branch's tip, which moves whenever anything merges to `main` — comparing it would invalidate every open verdict on every unrelated merge, converting a rare-event guard into a permanent merge deadlock. A base that merely advances is out of scope by design: rebasing onto it moves the head sha, which the per-sha binding already covers. - The field goes in the status DESCRIPTION, not the verdict comment. The comment body is parsed by `scripts/check-review-verdict.sh`, whose grammar had three false-opens in its history (#629); nothing parses the description, so this adds a field without reopening that surface. Scope is stated honestly rather than overclaimed: this is DETECTION on the hook path only. A commit status carries no base of its own, so the server-side required check cannot see a retarget, and a merge driven through the Gitea UI or API is unaffected. That is the accepted exposure — base changes are rare, manual, and this is a two-account repo — but it now fails loud in the one place that evaluates consent, instead of living only in a doc. Verdicts posted before this change carry no `(base: …)` and get NO opinion rather than a deny; denying would block every in-flight PR the day it lands, and the window closes on its own since verdicts are per-head and short-lived. Verified by mutation, six mutants, each killed by its intended test: remove the hook's deny; compare base.sha instead of base.ref; drop graceful adoption; stop recording the base; drop the TOCTOU guard; accept a PR with no resolvable base. The positive controls matter more than usual here — the test PR is deliberately non-docs (a docs-only PR short-circuits the whole gate and would never reach the base check) and the rest of the gate is unstubbed, so "the hook denied" alone proves nothing. Refs #632 Decisions-Edit: yes	2026-07-26 23:13:50 +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	b255b7ffdc	test(648): close the mutation gaps round 5 found — two tests passed for the wrong reason ... Round 5 could not break the predicate itself: 28,930 real runs of the script across 14,465 crafted --version strings on bash 3.2.57 and 5.3.15 produced zero fail-opens, and `{1,9}` is honoured on bash 3.2, so round 4's bound is not void on the authoring Macs. What it did find is that two of round 4's changes were unpinned, and the tests that looked like they covered them did not. Reverting BOTH the first-line slice and `[[:blank:]]`→`[[:space:]]` together left the whole suite green. The four filler cases are all killed by the SEPARATOR restriction alone, so they attributed the fix to the wrong layer. Added three cases carrying the literal word `version` (`jq\nversion\n9.9` and friends), which satisfy the separator rule and can only be stopped by confining the parse to line one with a newline-free blank class. The CR-strip test was worse: vacuous through two independent mechanisms. `str.splitlines()` also splits on `\r`, so a per-line view dropped the stray CR; and `subprocess.run(text=True)` translates `\r` to `\n` outright, so even a raw-string check on stdout was unfalsifiable. The mutant demonstrably emits `... = jq-1.6<CR> (parsed 1.6; ...)` at the byte level while the test reported green. Added `run_bytes()` and a bytes comparison. Both gaps are now mutation-verified: reverting either change reddens exactly its own test. Also records the operational edge this parser acquires in the follow-up: it is strictly fail-closed by design, so once the floor mode gates the required check, a jq wrapper that prints a banner line would deadlock merges. The fix there is to widen the accepted forms, never to relax fail-closed. Decisions-Edit: yes	2026-07-26 22:21:07 +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	63fa81fbb5	docs(648,649): the jq contract + the shared PR-file enumeration record ... Adds docs/ci-cd.md "The jq contract" (1.6 floor, the three divergent constructs, and the deliberate pin-vs-floor asymmetry with its merge-deadlock reason), plus two decision records: ci.jq-version-contract and ci.shared-pr-file-enumeration. ci.script-tests-job stops restating the three jq rules and points at the new record. Also corrects the script-tests preflight description: it is now two steps (git presence, then jq VERSION via scripts/jq-preflight.sh --expect 1.6), not one. A literal NUL byte had crept into the ci-cd.md paragraph describing jq 1.6's NUL truncation — which git treats as a binary file. Replaced with the literal text. Decisions-Edit: yes	2026-07-26 22:21:07 +02:00
timothy	e4c0db7702	Merge pull request 'fix(510): one watermark resolver for all four attachment points' (#655) from fix/510-watermark-resolution-policy into main	2026-07-26 20:16:34 +00:00
timothy	c0376dcbce	Merge pull request 'fix(644): stop seven SPA list loads truncating silently — one shared pager, bounded media pickers' (#656) from fix/644-spa-paging into main	2026-07-26 20:10:04 +00:00
timothy	bc1a37ff01	fix(510): pin the blank-guard's is-Custom discriminator, verified by mutation ... Round-4 review found the fall-through tests, while now falsifiable, still did not pin the whole guard. Two gaps, both closed and both verified by running the mutation rather than by asserting the test would catch it: 1. All three fall-through tests used only " ", so narrowing IsNullOrWhiteSpace to `image == " "` would have passed while breaking fall-through for null and "" -- and null is the form the API actually persists. Parameterized over null, "" and " ". 2. Nothing pinned the guard's `ImageSource is Custom` clause. This is the sharper of the two: a ChannelLogo watermark's Image is NORMALLY blank (the API writes Image = null for every non-Custom source), so dropping the discriminator would send every playout-item ChannelLogo watermark down the fall-through path instead of resolving the channel's own logo -- with a fully green suite. Added Blank_Image_ChannelLogo_Playout_Item_Watermark_Should_Win_And_Not_Fall_Through, which distinguishes the two levels by watermark Id so a fall-through is observable even though both resolve to the same cached path. Mutation results (each mutation applied on its own, then reverted): drop `is Custom` from the guard -> 1 failure, and the new test is the ONLY test that catches it IsNullOrWhiteSpace -> == " " -> the null and "" parameterized cases fail Negative control re-measured on the final 32-case fixture: 19 fail against the origin/main resolver. The 13 that pass both ways pin deliberately preserved behavior plus the positive control, which the record now states explicitly along with the mutation table. This round's lesson, recorded in the record: a test's NAME is not evidence it pins what it claims, and a whole-file revert cannot show that a test aimed at a specific clause actually reaches that clause -- only mutating the clause can. Gates: 2661 tests green across 4 projects, 225/225 script tests (the gate I skipped before the last push), decisions-validate OK, format exit 0, no BOMs. refs #510	2026-07-26 21:53:01 +02:00
timothy	d189d17157	fix(510): quote the record's rule scalar — unquoted " #653" was a YAML comment ... CI's script-tests job (test_frontmatter_reader_matches_pyyaml_on_every_real_record) caught that the dependency-free hand parser and PyYAML disagreed on this record's `rule:` field. Cause: `rule:` was an unquoted plain YAML scalar containing " #653)", and in a plain scalar " #" starts a comment. PyYAML therefore truncated the value at "...(the song-progress overlay," and silently dropped the rest of the clause -- including the scope carve-out naming #653, which is the whole point of that sentence. The generated catalog row was built from the truncated value, so the active-decisions view was quietly missing it. Same trap as fix(631) (a record TITLE whose unquoted " #" was a comment). The `signals:` and `mechanics:` fields were already single-quoted, which is why only `rule:` diverged; `title:` is safe because its "(#510)" has no space before the "#". Single-quoted `rule:` and regenerated the catalog, which now carries the full sentence. Worth recording: I ran scripts/decisions_validate.py locally but NOT scripts/tests, so the gate that catches exactly this class was the one local check I skipped. Touching the decisions corpus means running the pytest suite, not just the validator -- the validator reads through the hand parser and so cannot see a hand-parser-vs-PyYAML divergence by construction. Local: 225/225 script tests pass, decisions-validate OK. refs #510	2026-07-26 21:38:09 +02:00
timothy	daedf003e5	fix(644): round-3 review — split truncated/incomplete picker hints, F2 out-of-list gaps, F3 abort warns, F4 aria wiring, F5 FillerPresetsScreen tests ... Addresses the round-3 cold re-review's five low-severity findings on #644's client-side paging fix: - F1: `loadPickerOptions` (RerunCollectionsScreen, PlaylistsScreen) returned one `truncated: boolean` for two different conditions — a real Class B cap hit vs an unconverged Class A `loadAllPages` load — so an incomplete multi-collection load rendered the self-contradictory "Showing the first 47 of 47 — use search to narrow." Replaced with a `hint: 'incomplete' | 'none' | 'truncated'` discriminator and distinct copy per value; 'incomplete' matches the wording already used by the Class A list-load warn Badge. - F2: mirrored the out-of-list current-selection injection (RerunCollectionsScreen/PlaylistsScreen's `selectedInList` prepend) into FillerPresetsScreen and ScheduleItemInspector's rerun-collection picker, so an id outside the loaded page still renders as selected instead of misrepresenting the stored value as "(none)". - F3: gated the `console.warn` on an incomplete Class A load with `!signal?.aborted` in the `multi` branches (RerunCollectionsScreen, PlaylistsScreen) and SchedulesScreen.loadAllRerunCollections, so a superseded/aborted load (Retry, or a type switch mid-load) no longer logs a false warning. - F4: added `Select`'s `ariaDescribedBy` prop and wired the truncation/incomplete hint span to it via `useId()` in RerunCollectionsScreen and PlaylistsScreen, so screen readers announce the hint (FillerPresetsScreen already routed it through `Row help=`). - F5: added FillerPresetsScreen.test.tsx (previously untested) covering the Class B single-request guarantee, the truncation hint's totalCount>100/<=100 boundary, and the F2 injection; added the two assertions the re-review found missing anywhere in the suite — the Class A `incomplete` warn Badge actually rendering, and a screen-level seqRef stale-overwrite race — to RerunCollectionsScreen.test.tsx. Updates docs/spa-conventions.md §3b and the spa.list-completeness-vs-bounded-pickers decision record to describe the hint discriminator. Decisions-Edit: yes	2026-07-26 21:29:53 +02:00
timothy	edf8be4b5e	fix(510): re-review round — make two review-added tests actually falsifiable ... Re-review of the previous fix commit found that two tests added to close round-1 findings could not fail. Both verified before fixing: 1. Missing_But_Named_Custom_Playout_Item_Watermark_Should_Not_Fall_Through gave the channel-level fallback the SAME missing custom path as the playout-item watermark, so a wrongly-widened guard would have fallen through to a fallback that also resolved to None -- the assertion held either way. The fallback is now an independently resolvable ChannelLogo whose cached file exists, so a fall-through returns it and fails the test. Added the matching positive control (blank -> falls through and DOES return that logo), so the pair shows the guard distinguishes blank from unresolvable instead of both landing on None. 2. Deco_With_One_Valid_And_One_Missing_Watermark... asserted a filtered list length while the routing claim the decision record cited it for lives in FFmpegLibraryProcessService.CanUseFFmpegNativeWatermark, which the test never called. It now calls the real predicate. Also, three wrong claims of my own: 3. The Resource arm comment said "nothing in the app writes a Resource watermark to the database". False -- CreateWatermarkHandler and UpdateWatermarkHandler persist whatever ImageSource the request names, so a Resource watermark IS creatable through the API, always with Image = null. That is precisely why the new null guard is load-bearing, so the comment was arguing for its own removal. 4. "One resolver and no per-caller policy" contradicted the surviving playout-item blank-Custom fall-through documented a few lines later. Reworded in both the record and the XML docs: one resolver, and exactly one piece of per-caller policy which lives in the CALLER. 5. The record's "12 of 18 new tests fail pre-fix" was stale. Re-measured against the final fixture: 19 of 29. The other 10 pass both ways by design because they pin preserved behavior, which the record now says explicitly rather than leaving the gap to be read as weakness. Removed the vacuous generated-URL test rather than keeping it with an honest comment -- an empty list trivially contains no URL, so it implied coverage it never had. Its assertion is folded into the sibling test that has a real arrangement. Gates: 2772 tests green across 5 projects, dotnet format exit 0, no BOMs, decisions-validate OK, live-E2E re-run against this binary (0 changed pixels, nameplate absent, warning emitted). refs #510	2026-07-26 21:29:11 +02:00
timothy	ee66cb7459	fix(505): address cold-review findings — retag on tonemap, subtitle scale, anamorphic ... Independent cold review (Codex, no implementation role) found no Blocker and three real defects, all fixed here: HIGH — HDR was re-tagged bt709 only when the profile had NormalizeColors on. The colorspace filter sat behind desiredState.ColorsAreBt709, so an operator with normalization disabled got tonemapped SDR pixels still tagged bt2020 and the player converted them a second time. The guard is now "tonemapped || (ColorsAreBt709 && ...)". Deliberately NOT fixed by hoisting usesVppQsv out of the guard, which would force bt709 on scale-only non-HDR chains that legitimately opted out. MEDIUM — image subtitles stopped being resized. The subtitle canvas is scaled only when the video chain contains a recognized scale filter, and that predicate listed the QSV filters only; swapping ScaleQsvFilter for ScaleVaapiFilter left a 4K HDR + PGS source with a 720p video and a source-size subtitle overlay. VaapiPipelineBuilder already listed ScaleVaapiFilter; QsvPipelineBuilder does now. MEDIUM — anamorphic HDR now falls back to the software tonemap. ScaleQsvFilter is handed the SAR VideoStream calculates (it has a 0:0 fallback); ScaleVaapiFilter multiplies by ffmpeg's runtime `sar`, which differs when the decoded frame leaves SAR unspecified. Rather than ship a graph nobody has run, exclude anamorphic -- which leaves those sources exactly where they were before this change. LOW — tests now pin the exact validated graph as an ordered substring (the prior assertions would have passed with setFormat off, hwdownload dropped, or the wrong tonemap output format), assert against the vpp_qsv OPTION rather than a bare "tonemap=1" substring, and cover NormalizeColors=false, anamorphic and image subtitles. Each of the three fixes was negative-controlled: reverting it fails exactly one test, and no others. The remaining LOW (deriveDevice's defaulted bool is a future-call-site trap) is recorded as an accepted residual rather than fixed, since a named factory would push this diff into the VA-API pipeline for no behavior change. The record is 86 prose lines, over the 60-line ceiling. Declining to cut: every bullet is a distinct measured finding, which docs.corpus-size-signal names as a legitimate decline. Decisions-Edit: yes	2026-07-26 21:16:05 +02:00
timothy	1a7f15fb27	fix(510): address independent review — Resource null guard, honest routing claim ... Two independent reviews (cross-family Codex + cold Opus) both returned BLOCKED. Findings, all verified against source before acting: 1. Resource arm could throw ArgumentNullException (Codex, Medium). Making the channel/global Resource arm reachable exposed that CreateWatermarkHandler and UpdateWatermarkHandler write `Image = null` for EVERY non-Custom watermark, so an API-created Resource watermark reached Path.Combine(folder, null). Added the blank/null guard the arm never had. This was live at the playout-item level too, not just newly-reachable code. 2. "Routing is unaffected" was false (Codex, Low but sharp). The predicate is unchanged, but CanUseFFmpegNativeWatermark also tests Count == 1, and dropping an unresolvable watermark shortens the list. A deco with one valid and one missing permanent watermark now routes ffmpeg-native where it previously routed to the graphics engine. Intended, but observable -- so it is documented and pinned by a test rather than claimed away. 3. "Exactly one resolver" over-claimed (Opus, High). True of the selector, not the application: the song-progress overlay is built as a WatermarkOptions directly by the streaming and troubleshooting handlers, unchecked, and can still hand ffmpeg a nonexistent -i. Pre-existing; scoped the claim in the record and channels.md and filed #653. 4. Undeclared crash->degrade change (Opus, Medium). Channel/global Custom had no blank-image guard, so a cleared image hit ImageCache's fileName[..2] and threw out of stream startup. Now declared in the record and tested. 5. Contradictory rule text (Opus, Medium) -- the catalog one-liner said "always no bug" while the body documents the playout-item fall-through exception. Qualified; catalog regenerated. 6. History was wrong in both the record and the XML docs: the three precedence levels did NOT all check every source -- channel/global had no Resource arm and threw. Corrected. Tests: 30 in the fixture now (was 18). New coverage for the preserved blank-Custom fall-through (to channel AND to global), the complement case (missing-but-named must NOT fall through), null/blank Resource, and the valid+missing routing case. 17 of 24 failed against the pre-fix resolver before this round; the fixture stays mutation-sensitive. Also: hoisted the mock-filesystem Initialize() out of its loop so a multi-file case cannot silently seed only the last file, and marked the generated-URL test honestly as redundant-by-construction rather than claiming independent coverage. The decision record is now 81 prose lines, over the 60-line ceiling. Declared as a legitimate decline per docs.corpus-size-signal: the length is the review findings above, each a distinct fact, not redundancy. refs #510 #652 #653	2026-07-26 21:12:13 +02:00
timothyandClaude Opus 5	94182cdd53	fix(644): split loadAllPages by list class; bound media-library pickers to one page ... Cold adversarial review of fe342a6a found the blanket loadAllPages-everywhere fix dangerous for the three getLibraryBrowseItems pickers (RerunCollectionsScreen, PlaylistsScreen, FillerPresetsScreen): paging Episode/Song/Image/Movie/MusicVideo to completeness can mean ~200 serial requests against a 20k-row library, each more expensive than the last, to populate a <select> with thousands of <option> nodes. - Class A (bounded-by-construction lists: rerun collections, multi-collections, playlists) keep loadAllPages. Class B (media-library pickers) now fetch ONE bounded page and surface truncation via a `Showing the first N of M` hint wired to the real totalCount, instead of paging to completeness or truncating silently. - loadAllPages: reports `{ items, complete }` instead of just `T[]` so a caller can no longer mistake a defensive empty-page break for a full list (F4); accepts an optional AbortSignal so a superseded loop stops issuing further page requests (F2); baseParams is now required via a conditional rest-tuple whenever the loader's params type has a field beyond pageNum/pageSize (F6); pushes into the accumulator instead of re-spreading it every page (F7). - MultiCollectionsScreen/RerunCollectionsScreen/SchedulesScreen: add a seqRef + AbortController guard around the list/bootstrap loads so a stale loadAllPages loop can't resolve after a newer one and resurrect deleted rows (F3); log and surface an incomplete load rather than rendering it as whole. - docs/spa-conventions.md §3b rewritten for the Class A / Class B split; new decision record docs/decisions/records/spa/list-completeness-vs-bounded-pickers.md (spa.list-completeness-vs-bounded-pickers), catalog regenerated. - Tests: paging.test.ts covers null/undefined totalCount, null page, a short-but-non-empty page, a page-2 rejection, the complete:false flag, and cancellation (asserting fetch call COUNT stays put after abort), plus a compile-time @ts-expect-error pinning the F6 typing fix. Screen-level tests pin a real second HTTP request for a >100-item Class A list (MultiCollectionsScreen) and exactly one /library/browse request plus the truncation hint for a Class B picker (RerunCollectionsScreen). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-26 21:04:55 +02:00
timothy	18c4f4e0b2	docs(505): correct the #498 record's escape-hatch claim, which was wrong the dangerous way ... The qsv-decode-encode-split record told HDR-on-QSV users to set QsvPreferNativeDecoder OFF "to keep GPU tonemap". Measured on the Intel host, vpp_qsv=tonemap=1 is a silent no-op on pre-Gen11 graphics, so following that advice did not preserve GPU tonemapping -- it disabled tonemapping entirely and shipped untonemapped HDR. Prose correction only; the decision itself (the QsvPreferNativeDecoder toggle) is unchanged and stays active. The software-tonemap half of the bullet was always accurate and stands. Decisions-Edit: yes	2026-07-26 21:00:49 +02:00
timothy	41e2870113	fix(505): tonemap QSV HDR through OpenCL; vpp_qsv=tonemap is a silent no-op ... #505 asked to route the #498 native-decode path through TonemapQsvFilter to move HDR tonemapping off the CPU. Measured on the Intel host (jazz: FFmpeg 8.1.2 / iHD 25.1.4 / UHD 630) against real HDR HEVC Main10, that filter is a SILENT no-op: a graph ending in vpp_qsv=tonemap=1 returns a frame byte-identical (same md5) to the same graph with no tonemap at all, with no warning and no error. QSV VPP tonemapping needs Gen11+; pre-Gen11 iHD ignores it. So the issue's premise was inverted, and the branch it wanted to extend was already broken: the existing DecoderHardwareAccelerationMode == Qsv path shipped untonemapped HDR whenever QsvPreferNativeDecoder was off -- which is exactly the escape hatch #498/#523 recommend. Prod was unaffected (native-decode is the default and took the working software branch). Tonemap on the GPU via OpenCL instead, the route VaapiPipelineBuilder already uses and the one Jellyfin uses. The scale has to run first, in scale_vaapi: tonemapping full-size is slower than the software path it replaces (15.5s wall for 12.5s of content, below realtime), while scale-first cuts total CPU ~60% (35.6s -> 14.1s) and lands at the no-tonemap wall-clock floor. A QSV surface maps to neither OpenCL nor VA-API, so the gate requires software frames: the QSV decoder and deinterlace_qsv both fall back to the software tonemap, slower but correct. TonemapQsvFilter is deleted -- a filter that silently does nothing is worse than no filter. Also fixes output tagging: the first end-to-end run was correctly tonemapped yet still announced bt2020 primaries, because SetPixelFormat's usesVppQsv predicate ("did a hardware filter strip color info") listed only the QSV filters. Both new filters are now in it. Validated end to end on jazz with the exact generated command: exit 0, YAVG 26.39 (software reference 26.6, untonemapped 44.3), and ffprobe reports bt709 space/transfer/primaries. fixes #505	2026-07-26 20:55:05 +02:00
timothy	9cbe70e486	fix(510): one watermark resolver for all four attachment points ... WatermarkSelector resolved watermarks in two places with two policies. The three precedence levels (playout item, channel, global) existence-checked every image source and degraded to None; the deco path had its own copy of the same switch that returned whatever path it computed, unchecked. So one channel could disagree with itself about whether an on-screen bug rendered, based only on how the watermark was attached. #502 deferred this here but scoped it to ChannelLogo. It was never ChannelLogo-only: the deco path skipped the existence check for Custom and Resource too. Extract one ResolveWatermark used by all four sites. Severity is not cosmetic. A dead LOCAL path is not harmlessly skipped -- CanUseFFmpegNativeWatermark hands a single permanent watermark to ffmpeg as a bare -i argument and excludes only URLs, so the deco path could hand ffmpeg a nonexistent input file. The generated-initials nameplate was real: a live-E2E on a real transcoded frame confirmed it composited via the deco path (/iptv/logos/gen is on ArtworkController, which has no auth filter, so the container-internal self-fetch succeeded). The #502-era comment claiming "it has never rendered here" was wrong, and the new record says so. It is still removed: serving it means an HTTP fetch inside stream startup, which graphics.channel-logo-caching (#525) eliminated for logos, and it depends on #1's hardcoded localhost. Reviving it by caching the image instead is #652. Measured blast radius on prod: 0 Deco rows, 0 DecoWatermark rows, all 43 channels have logo artwork -- no rendered output changes. Preserved deliberately: a playout-item Custom watermark with a blank image still falls THROUGH to the channel/global watermark; unifying resolution must not change which watermark wins. Routing is untouched. Strict improvement: the channel and global arms previously threw NotSupportedException on a Resource watermark; they now resolve it. The default arm still throws so a new image source fails loudly. Tests: 18 new cases including a positive control and 8 deco-vs-channel parity cases. 12 of the 18 fail against the pre-fix resolver, which is what proves they are load-bearing rather than vacuous. fixes #510	2026-07-26 20:54:00 +02:00
timothy	fe342a6a0b	fix(644): page SPA list loaders to completeness instead of inflating pageSize ... Seven call sites (rerun-collections, multi-collections, library/browse) requested pageSize far above each endpoint's server-side MaxPageSize=100 clamp and took the single response page as the whole list, so rows past 100 silently vanished with no error or truncation indicator. Extract the loadAllRerunCollections pattern from SchedulesScreen (#634) into a shared, generic web/src/api/paging.ts::loadAllPages helper that pages against totalCount with an empty-page defensive break, and refactor SchedulesScreen plus the seven over-cap call sites in RerunCollectionsScreen, MultiCollectionsScreen, PlaylistsScreen, and FillerPresetsScreen to use it. Server caps are unchanged (api.search-allitems-paging precedent: client pages, server stays bounded). Document the convention in docs/spa-conventions.md §3b.	2026-07-26 20:32:06 +02:00
timothyandClaude Opus 5	fefd11dffe	fix(620): signal corpus size per RECORD; the aggregate becomes an unthresholded trend ... Squashed from 7 commits (4 review rounds) to keep the rebase onto #621 tractable; the full round-by-round history is on PR #642. corpus was 5658/5600 — over budget and warning again — 3h35m after #619 put it at 5228, with nobody consolidating anything. So this does NOT re-baseline. An aggregate over a monotonically growing corpus can only ratchet; that is the "permanently red, therefore no signal" state #542 re-baselined away from, and growth is not even a smooth rate to plan against (the corpus FELL from 5089 to 5042 across four days, then gained 427 in one evening as two large records landed). Replaces it with a per-record prose ceiling (default 60), non-blocking, naming each record over it — not monotonic, so it can go red AND green, and it points at a file. The aggregate is still printed as an unthresholded trend notice, with record prose and non-record scaffolding reported separately because they are not the same unit. The GENERATED catalog is no longer counted at all: it gains one row per record and no consolidation can shrink it, which made the metric partly a record COUNT in a line-count costume. The calibration test took FOUR versions, and the failures are the durable lesson: v1 true by construction (`max(under) <= 60 < min(over)` over lists built by that test) v2 a gap WIDTH — a ceiling of 200 also sits in a wide gap, so it passed v3 fraction band + "clear air" vs the nearest record above — hostage to an unrelated record: one ordinary 62-line addition reddened it with the ceiling correctly placed, and the only remedy was to RAISE the ceiling. That is this very treadmill, as a hard failure in what #631 makes a blocking job. v4 `p90 <= ceiling <= p95` — the property stated directly and scale-free. Two rules recorded: a guard test must depend only on the thing it guards, and a threshold over a growing population must be expressed in that population's own terms. Candidates: all over-ceiling records assessed, each actioned or declined with a reason. The largest (scan.libraryfolder-unique-identity, 230 lines) is a legitimate DECLINE — a dozen-odd distinct traps whose only copy that is. Nothing pruned, so no archive or supersession was required. An automated redundancy metric is explicitly rejected. Also: `--budget` is accepted but announces its retirement rather than no-opping silently; the dead `budget_ok` parameter is gone; and five "untresholded" typos are fixed, one of which was propagating into the generated catalog row and MemPalace's per-key drawer. Refs #620 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-26 13:57:01 +02:00
timothyandClaude Opus 5	37fd30dce7	fix(631): quote a record title whose unquoted # was a YAML comment ... Third latent defect surfaced purely by running scripts/tests in CI — and this one was already red on origin/main, where test_frontmatter_reader_matches_pyyaml_on_every_real_record fails. `api/paging-zero-based.md` carried an UNQUOTED plain scalar title ending "... (#616, #633)". In YAML a space followed by `#` starts a COMMENT, so PyYAML reads the title as truncated at "(#616," while the hand-written dependency-free reader keeps the whole line. PyYAML is correct per spec; the record's title was genuinely losing "#633)" for any spec-compliant consumer, which includes MemPalace's ingest. Fixed by quoting the scalar; both parsers now agree byte-for-byte. Asked "instance or class?" before fixing: swept every frontmatter field of all 179 records under both parsers and compared. This is the ONLY divergence in the corpus, so a one-line fix is the right scope — no lint rule needed, and #621's structural guard already makes a parse FAILURE loud (this was a parse-to-different, which no structural check can see). 216 passed under jq 1.8.2 and jq 1.6. Refs #631 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-26 13:27:05 +02:00
timothyandClaude Opus 5	bb1809fbf0	fix(647): the jq-1.6 fail-open was ALSO in review-verdict.yml — the enforced gate ... Round-4 review caught the miss that matters most: #643 fixed the `jq -e`-on-empty fail-open in .claude/hooks/pretooluse-merge-consent.sh, but the SAME construct sits in .gitea/workflows/review-verdict.yml — and that is the copy that runs on the CI runner, where jq is 1.6, and that feeds the branch-protection-required review-verdict/h10 status. Reproduced: `printf "" | jq -e '.statuses | type == "array"'` exits 4 on jq 1.8.2 (guard fires, correct) and 0 on jq 1.6 (guard passes). So on a transient API error `statusjson` is empty, the guard lets it through, `existing` reads "", and the job posts `pending` — or for a bot/docs-only PR an exemption `success` — over a possibly-existing human verdict. That is precisely what the comment three lines above it says must never happen. The hook version was harmless in practice because it runs on a dev Mac with jq 1.8. This one is live. Fixed identically, with a comment naming why the sibling fix missed it, and the same hardening applied to the changed-files read in the same workflow. Also from round 4: - LOW, reproduced — an ARRAY-valued `.status` dodged the closed allow-list. `index` is polymorphic: with an array argument it does SUBSEQUENCE matching, so `[...,"renamed",...] | index(["renamed"])` is truthy while `.status == "renamed"` is false — the row passed the allow-list AND skipped the `previous_filename REQUIRED` clause. The same `git mv code -> docs/` dodge the closed set exists to block, one type away. Now requires `.status` to be a string first; mutation-verified. - The record now carries all THREE jq rules rather than the one, and notes that the durable fix is pinning/preflighting the runner's jq version rather than patching constructs one at a time (tracked on #647). 200 tests pass under BOTH jq 1.8.2 and jq 1.6. Refs #647, #643, #631 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-26 13:24:20 +02:00
timothyandClaude Opus 5	f4473926d4	fix(643): close the status-string dodge, pin the allow-list anchors, fix two doc claims ... Round-3 review (MERGEABLE, all findings Low/Nit) — applied anyway, because each is the over-claim or unpinned-anchor family this PR keeps hitting. LOW — the `renamed => previous_filename REQUIRED` clause matched `.status` by exact lowercase string, so any other value took the `else true` branch: a row with `"Renamed"`, or with no status at all, validated fine and silently dropped its SOURCE path, letting `git mv ErsatzTV/Program.cs -> docs/a.md` read as docs-only. `.status` is now checked against a closed set and an unknown value fails closed. Two things that fix taught me, both caught by my own positive control rather than by review: 1. The first predicate was WRONG in a way that gated everything: inside `[...] | index(.status)`, jq's `.` is the ARRAY, so `.status` was null and every row failed. `$s` is now bound from the row before the context switches. A security check that rejects everything looks identical to a working one from the failing side — only test_gitea_real_status_values_are_accepted caught it. 2. The set includes BOTH `changed` and `modified`. Live Gitea 1.25.4 emits `changed`, but a closed allow-list built from the wrong vocabulary is worse than the hole it closes: it would gate every genuine docs-only PR. The property wanted is "reject what we don't recognise", not "enumerate one version exactly". LOW — three allow-list anchors had no test at all: dropping `^` from the `docs/` alternative (`ErsatzTV/docs/Program.cs` would exempt), dropping `$` from `.md` (`x.md.cs` would exempt), and dropping the non-empty-list guard. Since the round-3 `..` finding WAS an anchor subversion, they are now pinned; all three mutation-verified. NIT — docs/ci-cd.md called this job "a checkout plus a pure-stdlib pytest run", which the same file contradicts 450 lines later and which this PR's own record names as the bug that turned the job red on its first CI run. Also replaced a frozen "111 tests" with an explicitly indicative figure — the suite is ~190 after rebasing onto main, and a frozen count has rotted four times this session. 198 passed under both jq 1.8.2 and jq 1.6. Refs #643, #631 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-26 13:24:20 +02:00
timothy	54c875414c	fix(643): validate every field the extraction consumes; fix a vacuous test ... Re-review of the previous fix commit found it incomplete and its test vacuous. Both reproduced before fixing. MEDIUM — the validation domain did not match the CONSUMPTION domain. `chunk` emits `(.previous_filename // empty)` for EVERY row regardless of status, but the guard validated that field only when `.status == "renamed"`. So a row marked `modified` — or Gitea's distinct `copied` — carrying a newline in previous_filename was still exempted. Verified EXEMPT for both statuses against the previous commit. Now validated whenever present, with the `renamed` => REQUIRED clause kept on top. MEDIUM — test_newline_in_previous_filename_is_also_rejected asserted the right outcome without exercising the mechanism: its payload's second segment was `ErsatzTV/Program.cs`, which the allow-list rejects on its own, so the test passed with the newline guard entirely removed. That is why the hole above went unnoticed — the same filter-hides-the-defect trap the guard itself is about. Payload changed to a segment that PASSES the allow-list, so the test now discriminates, plus parametrized regressions for modified/copied/added. LOW — `..` components rejected. The allow-list anchors `^docs/`, so `docs/../ErsatzTV/Program.cs` matched it (reproduced). Git will not produce such a path, but this guard exists to fail closed on unexpected 2xx shapes. Added a positive control (a legitimate docs->docs rename still exempts) so the tightened row schema cannot be satisfied by never exempting anything. Severity calibrated in the record: the docs-only exemption ends in `decide allow ""`, a passthrough to the normal permission prompt, NOT an auto-grant. Every bypass here downgrades a mechanical deny/ask to a human prompt; none can silently self-merge. Real, worth fixing, but not what an earlier framing of #643 implied. All mutation-verified; 123 passed under BOTH jq 1.8.2 and jq 1.6. Refs #643, #631	2026-07-26 13:24:20 +02:00
timothy	c046add10a	fix(643): close two more fail-opens in the docs-only enumeration, found by cold review ... An independent cross-family review of the jq-1.6 fix found two further ways the docs-only exemption can fire over an incomplete file list — both reachable with NO transport error, so neither had anything to do with the original bug. 1. HIGH — a path containing a newline. `chunk` flattens paths into newline-delimited text before the allow-list grep, so a filename of "safe.md\ndocs/Program.cs" splits into two lines that BOTH match the allow-list, while the real single path ends in .cs. Git permits newlines in filenames and the reviewer reproduced the bypass against this hook. Now rejected outright at the row-schema guard, on both `filename` and `previous_filename`: no docs path contains a control character, so failing closed costs nothing. 2. HIGH — a short page read as the last page. `n < 50` assumed the server's page size is the 50 we requested, but Gitea caps `limit` at the server-wide MAX_RESPONSE_ITEMS (default 50, configurable) and may return fewer. A 30-row docs page followed by a page of code completed the enumeration over a PARTIAL list. Only a validated EMPTY page may now terminate it; the page<=40 cap still fails closed, and the cost is one extra request. 3. MEDIUM — the enumeration was not bound to one head. Paging is several round-trips, so a force-push between them assembles a list belonging to no single commit: page 1 from head A plus a short docs tail from head B, with B's code page never read. The head sha is re-read after enumeration and the exemption refused if it moved. All three mutation-verified: reverting each fix reddens exactly its own test and nothing else. A positive control (short page then empty page) pins that the stricter terminator still exempts a genuinely docs-only PR, so "never terminate early" cannot be satisfied by never exempting anything. 118 passed under BOTH jq 1.8.2 and jq 1.6. The record now states the generalisable lesson: every defect here was an exhaustiveness failure in an enumeration whose completeness is load-bearing. When a security decision depends on having seen ALL of something, the termination condition must be positive and explicit, never inferred from a proxy. Refs #643, #631	2026-07-26 13:24:20 +02:00
timothy	5f068a2488	fix(631): close a fail-open in the merge-consent gate that only CI could see ... Running scripts/tests in CI for the first time turned up a real defect in the gate itself, not just in the harness. `jq -e` over EMPTY input exits 4 on jq >= 1.7 but 0 on jq 1.6 — verified against both binaries, not inferred. The docs-only pagination guard in pretooluse-merge-consent.sh leaned on that exit status to reject a transport failure. On jq 1.6, which the CI runner ships: page 2 errors -> gq returns empty -> jq guard wrongly PASSES -> n is empty so [ "$n" -lt 50 ] errors into false -> the loop walks PAST the failed page -> page 3 legitimately returns [] -> files_complete=yes over a PARTIAL list -> the docs-only exemption fires over unread pages that may be pure code. That is the very defect the guard's own comment describes, reintroduced one layer down by a jq version difference. Fixed by rejecting an empty body explicitly rather than inferring it from jq's exit status. The same hardening is applied to the review-verdict status read, which fell through to `vstate=""` -> deny (fail-CLOSED, so never a hole) but would have surfaced the wrong message. Why it survived: the existing transport-failure test asserts the right thing but can only observe the bug where jq is 1.6, so it passes on a developer Mac with the bug fully present — and the suite had never run anywhere else. The new test removes that dependency by shimming ONLY jq 1.6's empty-input exit status, so it pins the property on any host. Mutation-verified: revert the fix and it goes red on jq 1.8.2. The shim is deliberately narrow (no `-n`): a broader first version swallowed the `jq -n` calls `decide` uses to build its JSON, so the hook emitted nothing and every decision read as a passthrough — the verifier manufacturing the exemption it was meant to disprove. test_jq16_shim_actually_reproduces_the_quirk now pins the shim itself, `-n` case included. Suite: 113 passed under jq 1.8.2 AND under jq 1.6. Refs #631	2026-07-26 13:24:20 +02:00
timothy	73577f484f	ci(631): install pyyaml too — the suite is not pure-stdlib, and CI proved it ... First CI run went red on `ModuleNotFoundError: No module named 'yaml'` at collection: test_migration_equivalence.py imports scripts/migrate_decisions_split.py, which uses PyYAML by design. It passed locally only because this machine already had PyYAML. The original claim came from reading decisions_lib/decisions_validate/build_catalog and the test files themselves — which is precisely the set that does NOT import yaml. Replaced with an AST import scan over all of scripts/, which gives the complete third-party set: pytest and yaml. (etv_client is imported only by scripts/scripted-schedules/entrypoint.py, a container entrypoint pytest never collects.) This does not contradict the dependency-free decisions READ path: decisions_lib._read_frontmatter is hand-written exactly so validation runs where nothing is installed. The one-shot WRITE path is allowed PyYAML, and its test needs it. Verified the way it should have been the first time: a clean venv with pytest alone reproduces CI's collection error; adding pyyaml gives 111 passed. Comments, ci-cd.md and the decision record all corrected to drop the "pure stdlib" claim. Refs #631	2026-07-26 13:24:19 +02:00
timothy	c0f4a52d7a	ci(631): preflight jq/git in script-tests, and reflow the header comment ... test_post_review_verdict.py and test_merge_consent_exemption.py exec the real post-review-verdict.sh and pretooluse-merge-consent.sh, which shell out to jq ~26 times. Those tests shim `curl` on PATH but NOT `jq`, and no existing small-lane job proves jq is present on that image — so a missing jq would land as ~20 opaque assertion failures rather than a diagnosis. Checks rather than installs: #390 deliberately removed run-time apt-get from CI, so the correct fix for a genuine miss is baking the tool into the runner image, and the error message says so. Refs #631	2026-07-26 13:24:19 +02:00
timothy	69d8d3ccfe	ci(631): run scripts/tests in CI as its own job, not inside the flake-covered decisions-guard ... Nothing executed scripts/tests/. No workflow and no Husky hook invoked pytest: decisions-guard runs decisions_validate.py and build_decisions_catalog.py directly (the code, never its tests) and the `test` job is dotnet test only. The 111 tests guarding the decision corpus, the #610 migration-equivalence harness, the merge-consent exemption logic and the #622 review-verdict poster were enforced only by whoever remembered to run pytest locally. Adds a `script-tests` job to pr-checks.yml rather than the step inside decisions-guard the issue proposed. decisions-guard is covered by ci.decisions-lifecycle-flake, a standing instruction that a lone `decisions lifecycle` red is a known flake and must not be investigated — so a genuine pytest regression there would surface as exactly the red every session is told to wave through. A gate inherits the credibility of the job it lives in. Runs unconditionally: the suite executes the real post-review-verdict.sh and pretooluse-merge-consent.sh, so a `scripts/**` path filter would miss a .claude/hooks/** edit. ~10s, pure stdlib + pytest. Negative control verified locally (broken test -> exit 1); CI-red measurement follows on a scratch PR per the Done-when. Refs #631	2026-07-26 13:24:19 +02:00
timothy	eb339084f9	Merge pull request 'fix(621): make an unparseable decision record loud instead of silently invisible' (#641) from fix/621-record-parse-guard into main	2026-07-26 11:23:12 +00:00
timothyandClaude Opus 5	9949703585	docs(621): state the mutation count precisely — it depends on which mutation ... The record said "10 tests go red"; on the rebased base a full short-circuit reddens 11. Both numbers are real and measure different mutations: neutering only the scan loop leaves the empty-wing check live (10), while returning [] from the whole function disables that too (11). The reviewer flagged the phrasing as ambiguous before the count also drifted. Now states the mutation alongside the number, since a bare count is exactly the kind of frozen figure this session has watched rot four times. Refs #621 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-26 12:05:09 +02:00
timothy	3d720a6bc1	fix(621): exempt by exact path, not basename — my "no basename exemption" claim was false ... Re-review found the basename exemption was not actually removed: the code still dropped any wing-root README.md, and the docstring defended that with "no such file exists today; the catalog README lives in TOPIC_DIR". That is false — docs/decisions/archive/ README.md exists and parses to []. So the rule text in the record, the catalog row and ci-cd.md all asserted something the shipped code contradicted. It was also a live hole, not just wrong prose: dl.active_files() globs RECORDS_DIR.rglob with NO filter, so a future docs/decisions/records/README.md would be a corpus source while exempt from the guard — verbatim the hole this PR cites as the reason to stop using _NON_DECISION_FILES, reintroduced one directory over. Now exempt by exact RELATIVE PATH ({archive/README.md}), with a test pinning that the same basename in the ACTIVE wing is still faulted. Mutation-verified. DISAGREEING with one review finding, deliberately. It asked for the empty-wing anti-vacuity check to be symmetric across both wings. The semantics differ: zero ACTIVE records means the scan measured nothing and any clean result is vacuous, while zero ARCHIVED records is a normal state — nothing superseded yet, true of any young repo and every fresh clone before the first supersession. Making it symmetric fails a correct corpus (it reddened 7 tests). The asymmetry stays, with the reason in the code and a test pinning it so nobody "fixes" it back. 127 passed. Refs #621	2026-07-26 12:03:43 +02:00
timothy	98b3e8715b	fix(621): close three exemption holes and pin the guard's own wiring, per cold review ... An independent cold-context review found the guard could be disabled or bypassed in four ways, one of them silently. HIGH — the guard's only wiring was untested. Deleting `wing_faults=record_wing_faults()` from main() left the ENTIRE suite green (120 passed) while a real block-scalar record vanished again with `decisions-validate: OK`. Every existing test either called the collector directly or handed validate() a hand-built list, so nothing pinned that main() ever invokes it — the #609 "prints OK while doing nothing" defect, one level up, inside the fix for that same defect. Independently reproduced before fixing. Now pinned by a monkeypatched sentinel test. MEDIUM — a record could escape by BASENAME. `_NON_DECISION_FILES` is a set of TOPIC-dir names ({README, migration-map, retrieval-eval}) and was applied to the wings too, so a genuine record at records/docs/retrieval-eval.md was skipped. Not hypothetical: the path<->key rule forces key `docs.retrieval-eval` to that exact filename, and dl.active_files() applies that filter only to the TOPIC_DIR glob — so the file would be a corpus source while exempt from the guard. Now nothing is exempt by basename. MEDIUM — the archive exemption was by LOCATION but justified by IDENTITY. Anything directly in archive/ was skipped, so a new unparseable archive/foo.md would vanish in the one directory left unwatched. Now exempt only if it really is a #610 stripped index: one keyless record with a known generated heading. LOW — parse-to-WRONG. `rule: >-` with an UNINDENTED continuation containing a colon parses to one VALID keyed record whose rule is literally ">-", plus a junk key, silently truncating the real value. PyYAML rejects that input, so the hand reader is more permissive than the writer. Now faulted via an unknown-frontmatter-key check. LOW — an absent or empty wing reported clean. Now loud: refusing to certify a corpus from a scan of nothing. Doc corrections, all measured rather than asserted: "all 167 records use single quoted lines" was false (117 of 168 rule: values are unquoted plain scalars — the invariant is single-LINE); the archive shape in the record, ci-cd.md and the rule: field said `archive/<area>/` where the code deliberately uses archive/** ; the mutation count is 10, not five; and the "converts ANY reader limitation" claim is softened to the class it actually covers. 125 passed. Refs #621	2026-07-26 12:03:43 +02:00
timothy	0f565b1f7e	fix(621): make an unparseable decision record loud instead of silently invisible ... A file under the record wings that the dependency-free frontmatter reader cannot parse returned [] and simply vanished from the corpus: decisions_validate.py printed OK, build_decisions_catalog.py --check said "up to date", the record was absent, and nothing anywhere errored. Reproduced end-to-end with a YAML block scalar (`rule: >-`), which is the natural thing to reach for on this corpus's very long rule: values and which parsed fine under PyYAML before #610's dependency-free reader. An EXISTING record disappearing was already loud (the no-vanish diff check). The hole was a NEWLY ADDED record, which that check structurally cannot see — no base state to diff against — so the author's own PR looks clean. Hence a per-PATH check, not a per-construct or diff-driven one: asserting "this path yields exactly one keyed record" turns any present or future reader limitation from silent to loud in one move. Adds record_wing_files/record_wing_faults to decisions_validate.py, surfaced through validate() as ERRORS (a file in the record wings that is not a record is a mistake by definition) and reported first, since a file that failed to parse leaves every downstream check silently evaluating an incomplete corpus. The five top-level stripped legacy archive files are exempt — they are generated "Records formerly in this file" indexes, keyless by construction, and are what keeps older date-based pointers resolvable. _read_frontmatter is deliberately NOT extended to accept block scalars; rationale in the new docs.record-wing-parse-guard record. 8 tests, mutation-verified: with the check neutered 5 go red, restored all 119 pass. A live-corpus positive control asserts the wings are non-empty so a clean result can never be vacuous. Refs #621	2026-07-26 12:03:43 +02:00
timothy	214fad2dcd	fix(633): document the 0-based paging contract on the OpenAPI parameters ... `api.paging-zero-based` says `pageNum` is 0-based across `/api/v1` and every wrapper of it. That was true of the MCP tool catalog and the docs, and not true of the generated OpenAPI document: all 24 paging parameters across the 12 paged operations were emitted with no `description` at all, so a consumer reading only `v1.json` — the intended contract, and what generated clients surface to their users — had to infer the base from `default: 0`. That is the same inference that cost #487 a verification pass on the MCP side, where the description was present but wrong. Annotates each `[FromQuery]` paging parameter with `[Description]` (`System.ComponentModel`), the mechanism `parentId` already used in ImagesController, and regenerates `v1.json`. `pageSize` states the endpoint's OWN cap, because the caps genuinely differ — 100 typical, 200 auto-tune members, 1000 search/all-items — and the record forbids documenting one global number; it also states that the offset derives from the effective (capped) size, so an over-large `pageSize` narrows the page instead of widening the offset. The generated TypeScript client covers DTOs only, not query parameters, so it is unchanged; `endpoint-index.md` carries summaries, not parameter descriptions, so it is unchanged too. Pinned by OpenApiPagingContractTests against the in-process generated document. The test NAMES the expected set of 12 paged operations rather than only filtering for parameters called `pageNum`: a filter cannot see an endpoint that should page and doesn't, which is exactly how two MCP tools escaped the equivalent check in #616. Set equality is asserted in both directions, and the caps are pinned per endpoint so a description naming the wrong cap fails — a wrong justification outlives a wrong line. Mutation-verified both ways: dropping one `[Description]` reddens the description test, and making one endpoint stop exposing `pageNum`/`pageSize` under those names reddens the set-equality test. Refs #633 Decisions-Edit: yes	2026-07-26 11:10:22 +02:00
timothyandClaude Opus 5	50bcd7b0c7	fix(629): strip raw HTML blocks, and state where the hardening stops ... 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 ... 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 ... 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