653 Commits

Include renames

Author	SHA1	Message	Date
timothy	cb7da865b6	Merge pull request 'docs: permit subagents explicitly, and make claiming an issue a check rather than a label' (#682) from docs/claim-protocol-and-subagents into main	2026-07-27 16:04:13 +00:00
timothy	1d76a088c6	Merge pull request 'feat(578): artist typeahead covers music-video and song credits; album_artist stops 404ing' (#676) from feat/578-artist-typeahead-source into main	2026-07-27 06:09:30 +00:00
timothy	b83e965994	docs: make subagent use explicit, and turn "claim an issue" into a check rather than a label ... Two rules that were implied but not enforceable, both demonstrated by ersatztv#649 being implemented TWICE in parallel to completion on the same day. Subagents. The kickoff's HARD CONSTRAINTS already require parallel disjoint slices, mandatory independent review from a cold brief, and a named model/effort per dispatch — none of which is satisfiable without delegation. But nothing said so outright, and a generic client preamble of the form "do not use the Agent tool unless the user requested it" reads as a prohibition. Now stated plainly in both CLAUDE.md (always loaded) and the kickoff (pasted per session), with what to delegate and what to keep inline. Claiming. `in-progress` prevents duplicate PICKUP, not duplicate WORK — the record already said so, but step 3 told you to apply the label and nothing else. It now requires four checks first: an open PR whose body says `fixes #N`, a remote branch naming the number, a claiming comment predating the label (exactly what select-queue.sh's CLAIM? flag raises and deliberately leaves unresolved), and a fresh git fetch. Each fails differently; all four are cheap. Staleness. The second half of the #649 collision was reading origin/main once, at branch time, and not again across hours and four review rounds. A branch on a stale base computes its diff against that base, so `git diff origin/main` shows other sessions' merged work as DELETIONS and pushing it reverts them. Re-fetch before every push, rebase when it moved. process.parallel-session-claim carries the incident, including what worked: the merged implementation was better in one respect and the discarded branch's test coverage was salvageable, so diff the two before discarding yours.	2026-07-27 07:44:25 +02:00
timothyandClaude Opus 5	27867e03cf	fix(651): make the stated invariant true on Playlists; pin the predicate's endpoints ... The review's headline finding was in my prose, not my code: spa-conventions and the round-8 commit both claimed an unbindable id "surfaces as 'no selection' with Save disabled" and that regressions "assert zero writes are reachable". True on RerunCollections and FillerPresets. False on PlaylistsScreen in all three respects — `draftFromItem` nulled the id but KEPT `selectedName`, so the row read "Cool Movie" over a null draft; Save had no selection check; and clicking it did issue the PUT with `mediaItemId: null`. Only the server's `ReplacePlaylistItemsHandler` 422 stood there, and the DB would have persisted it (`PlaylistItemConfiguration` marks all four FKs `IsRequired(false)`). Rather than weaken the claim, made it true: a dropped id now clears its label, and Save is gated on every item having a selection, with a visible count as the reason. `playlistGroupId` was the same class on the same screen — seeded from the wire into `AddPlaylistDialog`, re-parsed with a bare `Number()`, and POSTed as an entity reference — so "every path by which an id from the wire becomes editor state" was not literally true. Now filtered from the group options and normalized on submit. Added `selectionId.test.ts`. The predicate had become the single point of failure for eleven call sites across three screens while being exercised only indirectly; nothing pinned the inclusive endpoints, so a `>` for `>=` slip passed the entire suite. Verified by mutating each comparison. Also documented why `0` and negatives are accepted — the contract is bindability, not existence — because every other id check in this repo uses `id > 0` and the next reader would otherwise "fix" the inconsistency. Two of my assertions were vacuous, the eighth of that shape on this branch: one clicked a button it had just asserted disabled (a restatement of `toBeDisabled()`), and one asserted a POST count on a path that never attempted a save. The first is deleted; the second now actually attempts the write, which makes it fail against the unguarded parent. Corrected claim: all five round-8 regressions do fail against their parent, but on their load-bearing assertions (`getByText('A selection is required')`, `queryByText('Bogus Collection')`) — not on the write-count ones, which were passengers. Follow-up filed as #677 (ScheduleItemInspector's unguarded ingresses; list-backed pickers dropping malformed options silently). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-27 04:25:14 +02:00
timothyandClaude Opus 5	39c4e8df0a	fix(651): review round 8 — put the selection-id predicate at the boundary, not the site ... Round 7 added the int32 check inside `isSearchPickerOption` — the place the defect was found — which left every other door into editor state open. A malformed successful response carrying `1.5` or `2147483648` still entered `draft` through list-backed options and through the selection restored from the detail read, keeping Save enabled and sending a value the API cannot bind, while the identical value arriving via SearchPicker was correctly rejected. The predicate now lives once, in `web/src/api/selectionId.ts`, and sits on every path by which an id from the wire becomes editor state. The class crosses all three screens, not just the one the finding named, so all three are covered: - RerunCollectionsScreen: `toPickerOptions` (3 list branches) + `draftFromRerun` - PlaylistsScreen: `toPickerOptions` (3 list branches) + `draftFromItem` (4 id fields) - FillerPresetsScreen: `draftFromPreset` (5 id fields) + the collection-family browse options - pickers.tsx: `isSearchPickerOption` now delegates rather than carrying its own copy An unbindable id is treated as ABSENT, never coerced — rounding 1.5 to 1 would submit a DIFFERENT record — so it surfaces as "no selection" with Save disabled and a visible reason; an option that cannot be selected safely is dropped rather than rendered. Five regressions assert zero writes are reachable via each previously-unguarded path. Also corrects two of my own test descriptions, per the review: the padded-ETag test is a regression guard rather than a round-7 defect demonstration (Headers strips outer whitespace before the app sees it), and the late-settlement test guards the abort/race COMPOSITION — what it actually fails is an abort-only implementation whose fetch ignores its signal, which is why its stub ignores `init.signal`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-27 04:25:14 +02:00
timothyandClaude Opus 5	e605e4006a	fix(651): review round 7 — treat "no usable token" as one class, not three values ... HIGH: the fail-closed gate rejected `null` but not the adjacent values. `Headers.get('ETag')` returns `''` for an empty or whitespace-only header, which PASSED the gate and produced an editable draft; `updateRerunCollection`'s `ifMatch ? … : undefined` then dropped the empty string as falsy and sent no `If-Match`, silently overwriting a collaborator — the exact class the gate exists to make unreachable, reached through the value next door. Absent, empty and whitespace are now one case ("no usable concurrency token"), normalized by a single `usableEtag` helper that returns the TRIMMED token or null, so `etagRef` can only ever hold something that will actually be sent. Tested across four blank shapes asserting zero PUTs are reachable, plus a padded ETag that must be trimmed and USED rather than dropped. MEDIUM: the deadline abandoned the wait without cancelling the work, so each Retry stacked another live connection. It now aborts via an AbortSignal (threaded through `getRerunCollectionWithMeta`) AND clears its timer on settlement and unmount. Both halves are kept deliberately: aborting cancels the work, while the rejected race stops the UI waiting even if the abort never propagates — cancellation and giving-up are not the same guarantee, which the late-settlement test proves by using a stub that ignores its signal. MEDIUM: `Number.isFinite` accepted ids the API cannot bind — `1.5` and values outside int32 rendered, committed through `onSelect`, and would fail server-side on `selectedId`. Validated as an int32 integer. MEDIUM: a malformed or failed page was reported as "No matches", telling the user the library is empty when the request actually failed and giving no hint that reopening retries. Failures now surface as a distinct alert. MEDIUM: `spa-conventions.md` still mandated the deleted "never let a refresh clear an id it failed to name" guard and said "the client guard stays" — contradicting the initialize-once bullet 20 lines below it. Rewritten to state that the guard is gone and must not be rebuilt, with the reason (it only ever preserved a list-seeded value that is null in production). Grepping the DELETED TERMS across all docs — the lesson from round 6's stale `rule:` — also caught two stale `signals:` tokens on the record that the rule fix had missed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-27 04:25:14 +02:00
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	fc3ede09bc	docs(578): the retracted claim survived in 9 places, including the record title and rule ... Comment- and docs-only; verified no non-comment line changed in any .cs. I reported last round that I had "classified every surviving hit". That was false, and the false confidence is the expensive part: a confidently-stated "I checked everything" stops anyone else checking. The retracted wording survived in nine places, two of them the record's title and rule: — and the catalog copies rule: verbatim, so the generated entry point and the record disagreed semantically while docs/decisions.md said the correct thing. Root cause of the miss, because it will recur otherwise: I built the sweep term list from the DELETED MECHANISM's vocabulary (LIKE, superset, keyspace, anchor, over-match) and never added the RETRACTED CLAIM's own words. "no predicate", "bound on work", "index entries", "no gap" and "holds in memory" were never grepped. After a retraction the subject list has to include the words of the thing being retracted, not just the thing already deleted. Second, worse: my first attempt at this round's sweep printed nothing for every term and I nearly read that as "all clear". zsh does not word-split an unquoted $FILES, so grep received one giant non-existent path — and the `|| echo "(none)"` never fired because the pipeline's exit status was sed's. Same failure shape as the bug arc itself: a check reporting success while examining nothing. Re-run with a proper array plus a control term ("SongMetadata" -> 42 hits) so an empty result is distinguishable from a broken grep. Fixed all nine, replacing "no predicate" with the seekable-cursor-vs-residual distinction already written correctly elsewhere: - handler: the "real bound on work" claim, the short-page rationale - SearchFieldValuesQueryShapeTests: "ANY predicate" + "reads exactly n index entries", and added what the test can and cannot pin (a SQL string, not a plan / visibility work / payload I/O) - GetSearchFieldValuesHandlerTests: "no gap between what the engine looks at and what it hands back", and the current-behaviour comment - record title, rule:, attempt-5 table row; api-conventions - regenerated docs/decisions/README.md so catalog and record agree again Tenth item, the same overclaim one level down and it survived the first retraction: the row bound was said to cap what the process holds in memory. It does not — payload width is unrestricted and one JSON array can contain arbitrarily many strings, each of which may enter the in-memory distinct set. It caps logical rows returned/materialized and round-trip count, nothing about bytes. Added as a third struck-through bullet next to the other two retractions.	2026-07-27 03:10:36 +02:00
timothy	5f73cd4482	docs(578): scope the bound to LOGICAL ROWS — physical work is not bounded, and I claimed it was ... Comment- and docs-only. Verified: the diff for both .cs files contains no non-comment line. HIGH (claim). "LIMIT n reads exactly n index entries and n rows" is false, and the bounded-physical- work reading it implied is false with it. Two mechanisms, both retracted explicitly in the record rather than quietly reworded: - MySQL purge lag. Deleted clustered-index records survive until purge runs and a range scan still traverses them, so returning 2,000 VISIBLE rows can touch far more index records. Deletion history therefore STILL affects physical work — precisely what attempt 4's failure was supposed to have made irrelevant. Attempt 5 removes the LOGICAL dependence on Id distribution; it does not make physical work independent of deletion history. - Unbounded row width. Artists/AlbumArtists are unrestricted TEXT/longtext and both SQLite and InnoDB spill to overflow pages, so a row count implies neither a byte count nor a page-read count. The guarantee is now stated as exactly what it is: at most 20,000 LOGICAL rows returned/materialized, and at most 10 round trips (11 for artist). The 392 KiB measurement is labelled one data point on one library whose credits average ~20 B/row, with an instruction to re-measure rather than extrapolate for long credit lists or MySQL over a network. Also noted: the query-shape test pins the SQL STRING — it cannot pin a plan, MVCC visibility work or payload I/O, and on MySQL using the index to satisfy ORDER BY is an optimizer choice, not a semantic guarantee. Precision fix, and the reviewer is right that the sloppy version misleads: "any predicate defeats LIMIT" is wrong, since the query's own cursor is a predicate. The distinction is a SEEKABLE PREDICATE ON THE ORDERING KEY (positions the scan, never discards a row) versus a RESIDUAL predicate (discards rows the engine already produced, so LIMIT bounds survivors and says nothing about how many were produced). Restated in the handler, the record and api-conventions. MEDIUM. docs/decisions.md still advertised "a superset LIKE pre-filter that may over-match but never under-match" for list-valued columns. That is the documented entry point for convention lookups, so a maintainer starting there was told to preserve exactly what round 4 deleted, in direct contradiction of the linked active record. Fixed. LOW. Two test comments read as current: "however many non-matching rows" (false past the ceiling — now bounded and cross-referenced to the test that pins that boundary) and the ampersand case "widens to the bare anchor" (no prefix predicate exists; kept because it is the input shape that broke the old scheme, now labelled as such). Fifth consecutive round of stale text, so this sweep was done by grepping the subject across handler, tests, record, decisions.md, api-conventions.md and the endpoint description: LIKE, ESCAPE, pre-filter, superset, over-match, under-match, anchor, keyspace, window, candidate, row cap, index entries, 392, and every deleted constant name. Every surviving hit is now either current-and-correct or explicitly framed as history.	2026-07-27 03:10:36 +02:00
timothy	373956fcee	fix(578): delete the SQL predicate — LIMIT only bounds work when there is nothing to discard ... BLOCKER. Attempt 4 bounded the Id KEYSPACE, and keyspace is not rows. Delete 20,000 historical rows, put one song at Id 20001, query artist?q=que: the walk burned all ten windows on empty ranges and returned [] for a table containing exactly one row. Capacity fell linearly with deletion ratio and no ratio was safe — one placed gap hides the next match. My record called that "heavily fragmented" and the endpoint description said loss happens "on a very large library"; the one-row example disproves both. Option A. The query now carries NO predicate at all — no LIKE, no LOWER, not even IS NOT NULL: SELECT Id, Artists AS Payload FROM SongMetadata WHERE Id > @AfterId ORDER BY Id LIMIT @Batch That is the whole fix, and it is the point. LIMIT truncates what survives a predicate, so with any predicate present it bounds the OUTPUT and says nothing about the WORK; the engine may evaluate and discard arbitrarily many rows first. Stripped to a bare primary-key range, LIMIT n reads exactly n index entries and n rows — independent of sparsity, deletion history or where the gaps fall. All selectivity moves into memory. A short page can now only mean exhaustion, which is precisely what it could not mean while a predicate was present. Four attempts, four wrong quantities: the result (a fixed budget the over-matching pre-filter starved), candidates returned (a no-match query must evaluate every eligible row before returning an empty page), keyspace width (above), and finally actual rows. The record carries the table; it is worth more than the code. Deleting the predicate deletes a whole bug family with it: the JSON-escape reasoning, the narrow-only-on-verbatim-ASCII rule, the exhaustive Unicode sweep that proved it sound, the ESCAPE '/' portability workaround, and the may-over-match-never-under-match invariant that turned out to be conditional on something untrue. SearchFieldValuesPrefilterSupersetTests is deleted entirely; the one assertion worth keeping — that the SQL has no predicate — moved to the query-shape suite, which pins the SQL string exactly so "just a cheap filter" fails a test instead of silently unbounding the walk. Measured cost of no server-side narrowing, on a seeded 20,000-song library (in-memory SQLite): worst case (no match, full walk) 20,000 rows / 10 round trips / 391.9 KiB / 119ms SQL, ~40ms warm end-to-end. Empty q, dense and non-ASCII prefixes all stop on page 1 at ~39 KiB and ~40ms. Judged acceptable for a debounced typeahead against a local file. If it ever is not, the answer is #669, not reintroducing selectivity — the record says so explicitly. Also fixed: - Round-trip count was advertised as 10; it is at most 10 for album_artist and 11 for artist, which also runs its EF query. The MAX(Id) probe is gone with the keyspace scheme, so there is no extra scalar call. - The duplicated-formula ceiling test is deleted rather than rewritten. It re-implemented the loop's arithmetic and would have passed through an off-by-one or a stall in the real loop; the dense integration tests carry that coverage. Its MaxVisited >= Window assertion was a style constraint in correctness clothing. - Stale text swept by grepping the mechanism nouns rather than re-reading: candidate/keyspace/ pre-filter/superset/row cap/LIKE/ESCAPE and the removed constant names, across handler, tests, record, api-conventions and the endpoint description. The two surviving "pre-filter" mentions are deliberate history. Test comments that rendered escaped non-ASCII as literal characters (which contradicted the raw-storage assertion in the same file) now show the escape text. New test List_Valued_Walk_Reads_Live_Rows_Regardless_Of_Id_Density reproduces the one-row killer and fails against attempt 4.	2026-07-27 03:10:36 +02:00
timothy	fbc7b2a1dd	fix(578): bound the Id KEYSPACE — LIMIT cannot bound a query that matches nothing ... BLOCKER. Round 3's ceiling counted LIKE-positive candidates, which is not the quantity that needed bounding. To return an empty page the engine must first evaluate every eligible row, so a no-match query came back with rows.Count == 0, ended the walk having counted zero against the ceiling, and had already inspected the entire table. Round trips and materialized rows were bounded; database work was not. Worse, a dense widened prefix could materialize 20x the candidates and make 10x the round trips of round 1 — a regression dressed as a bound. Third time bounding the wrong quantity: revisions 1-2 bounded the RESULT (a fixed LIMIT budget the widened pattern starved), revision 3 bounded the CANDIDATES, and neither bounds what the database LOOKS AT. Now the Id range is closed on both sides — `Id > @AfterId AND Id <= @AfterId + @Window` — so each round trip is a primary-key range scan of known width. The LIKE still decides what comes back; it no longer decides how much gets looked at. The walk advances by the WINDOW, never by what returned (an empty page means "nothing matched in this stretch", not "exhausted"), and there is deliberately no LIMIT in the SQL — the window caps the row count, and a LIMIT would only restore the illusion that it is doing the bounding. One indexed SELECT MAX(Id) up front stops the walk burning windows on empty keyspace. Result: at most 10 round trips and 20,000 rows inspected for any q, matching or not. New test `List_Valued_Walk_Cannot_Inspect_Past_The_Bound_When_Nothing_Matches` covers the case every earlier bound test missed — they all gave the pre-filter something to return. With a narrow pattern and a needle at row 30,000, revision 3 returns the needle (right answer, unbounded work) and this revision returns []. Paired with a positive control at row 20,000 inside the bound. The ceiling's remaining false negative (20,000 "zzz" rows then "éclair" at 20,001 -> []) is kept and still pinned: it is the documented bounded-best-effort contract at a rarer threshold, not a defect. Stale/false comments, several of them repeat offenders: - The handler still used "Zulu"/"Éclair" as the truncation example, where both orderings pick "Zulu". I had corrected the test and left the comment. Now "Zulu"/"apple" in both. - "leans entirely on the row cap" described a mechanism that no longer exists. - The escape example rendered as literal Édith/É/é rather than the escape TEXT (Édith, É, é) in both the handler and the record, contradicting the explanation it was giving. - "every accented artist was unsuggestable" overstated it: exactly three of the nine pinned cases fail 1b78dc9e (those where query and stored casing differ, so the escape texts diverge); the other six pass. The record now says what the test comment already said. - The record claimed the ordinal switch left response SETS unchanged. False — ordering runs before Take(limit), so with "Zulu"/"apple", empty q and limit=1 the comparer changes which value survives. - The_Scan_Ceiling_Is_A_Whole_Number_Of_Batches was a style preference wearing a correctness costume (a 20,500 ceiling is perfectly safe — the final window clamps). Replaced with the invariant that actually matters: the walk's own arithmetic terminates and lands exactly on the ceiling.	2026-07-27 03:10:35 +02:00
timothy	a37847e509	fix(578): a fixed row budget turns an over-matching prefilter into a false negative ... BLOCKER 1. The invariant a883e5f0 established — "the pre-filter may over-match, it must never under-match" — is only sound while the candidate set is not truncated. It was truncated. A non-ASCII or JSON-escaped prefix collapses the pattern to the bare `%"%` anchor, so every row becomes a candidate, and `ORDER BY Id LIMIT 1000` then spent the whole budget on rows that could not match. Seed 1000 songs by "zzz", put the only "éclair" in row 1001, ask for album_artist?q=é: a883e5f0 returns [], while 1b78dc9e returned "éclair" because its (separately broken) tighter pattern kept the candidate set small. Neither revision was correct — the old one under-matched at the pattern, the new one under-matched at the cap. Widening a predicate under a fixed budget starves it. So the budget is gone. Candidate rows are now walked keyset-paged on Id (`Id > @AfterId … ORDER BY Id LIMIT @Batch`), continuing past non-matching candidates and stopping on the first of: enough distinct exact matches for `limit`, a short page (source exhausted), or a 20,000-candidate-row ceiling in 2,000-row batches. The bound is on effort; it no longer silently decides the result, and the lossy case needs 20,000 rows that already passed the pre-filter before it bites. BLOCKER 2. The endpoint description and the record's rule claimed ordinal matching/dedup/ordering endpoint-wide. False for EF-backed fields: the database runs LOWER/DISTINCT/ORDER BY/LIMIT before any ordinal code, so `genre?q=é` still misses a stored "Éclair" on SQLite. Both are now scoped to the final in-memory stages, and the underlying gap is referenced as #668 rather than described as fixed. #669 (normalized SongArtist table) is referenced as the follow-up for the scan cost. Accuracy corrections to my own claims, all verified by re-running the mutations: - Only THREE of the nine Unicode cases fail 1b78dc9e (é/édith/BJÖRK — where query and stored casing differ, so the escape texts diverge); the other six pass it. The comment said all nine. They stay as continuity coverage, now labelled as such rather than as regression guards. - Ordering_Is_Best_Effort used "Zulu"/"Éclair", where DB and ordinal orderings BOTH pick "Zulu" — it could not demonstrate the divergence it claimed. Now "Zulu"/"apple", which actually diverges: ordinal ranks "Zulu" first, the DB ranks "apple" first, and limit=1 returns ["apple"]. The record sentence was false and is corrected. - The record printed literal "Édith"/"é" where it needed to show the escape TEXT (Édith, é), contradicting the very explanation it was giving. - Corrected the cost claim: the leading wildcard forces scan ACCESS, but each page stops once it has filled @Batch, so a dense query finishes early — it is not necessarily a full table scan. - The Unicode sweep is labelled a PROOF OBLIGATION: it is revision-independent and passes every revision, which is correct for what it is but must not read as regression coverage. One process note: the new record's frontmatter had a lone apostrophe inside a single-quoted YAML scalar ("SQLite's"). decisions_validate.py's hand parser accepted it; scripts/tests caught it.	2026-07-27 03:10:35 +02:00
timothy	1641ca8305	fix(578): the LIKE prefilter under-matched every accented artist; make the superset provable ... Review of 1b78dc9e found the pre-filter's correctness claim was false, and the claim was in the decision record as well as the code. F1 (high). The pattern JSON-encoded the whole query prefix on the reasoning that the stored text escapes non-ASCII, so encoding the prefix the same way would line up. It does not: SQL LOWER() lowercases the *escape text* (`É` -> `é`); it cannot case-fold the codepoint that escape denotes. So `q=é` built `%"é%`, the stored `Édith Piaf` never matched, and the row was discarded before the in-memory filter could accept it. Every accented artist — Beyoncé, Björk, Sigur Rós, Édith Piaf — was silently unsuggestable, which in a music library is the common case. The invariant that was missing, now stated in the code: the SQL pre-filter is an OPTIMIZATION. It may over-match; it must never under-match. Correctness lives in the in-memory filter. So the pattern now narrows only on the leading run of characters the JSON writer stores verbatim and stops at the first character it cannot prove — `q=Beyoncé` still narrows on `beyonc`, `q=é` narrows on nothing and leans on the row cap. Soundness rests on two facts now asserted by exhaustive computation rather than argued: no non-ASCII codepoint in U+0080..U+10FFFF OrdinalIgnoreCase-equals a printable ASCII character (false for InvariantCultureIgnoreCase, which folds ~190 — the choice of Ordinal is load-bearing), and the exact set of ASCII the encoder escapes. F1b. `UseRequestLocalization` honours Accept-Language, so the culture was caller-controlled and `ToLower()` plus the default linguistic `StartsWith(string)` let a header change the answer. Comparison is now OrdinalIgnoreCase and ordering StringComparer.Ordinal throughout — including the shared FilterSortTake that state/video_dynamic_range/content_rating also use. Sets unchanged, order now ordinal rather than culture-dependent. F2. The merge comment asserted an exactness the code does not have: sources truncate by their own ordering (DB collation / primary key), not the merge's, so a dropped value can outrank a survivor. Comment and record now say best-effort, exact only below the truncation points. F3/F4. The cap now rides `ORDER BY Id` rather than the JSON column: MySQL sorts TEXT by only max_sort_length bytes, so the old ordering was not deterministic there, and sorting the whole matching set was avoidable work. What the cap still does NOT bound is the scan — a leading-wildcard LIKE cannot seek an index — so that cost is now documented as accepted, with a normalized `SongArtist` table named as the follow-up candidate rather than left implicit. Every clause above is covered by a test verified to FAIL when that clause is mutated (old pattern builder: 5 red; culture chain: 3 red; cap=3 / cap=limit / ORDER BY json / no cap: red each). F5. Converted to a proper supersession. The old record did not merely hold a stale fact — it recorded song/music-video credits as an "intentionally-uncovered gap" and album_artist as unsupported, and this reverses that call, which `docs.decision-lifecycle` says is never a line-edit. `api.search-field-values` is archived with its original prose restored, and `api.search-field-values-sources` replaces it carrying the whole endpoint contract.	2026-07-27 03:10:35 +02:00
timothy	cd6f36185c	feat(578): artist typeahead covers music-video and song credits; album_artist stops 404ing ... `GET /api/v1/search/fields/{name}/values` sourced `artist` from `ArtistMetadata.Title` only — entity artists — so the free-text credits that `LuceneSearchIndex` also writes to the `artist` field (`MusicVideoArtist.Name`, `SongMetadata.Artists`) produced no suggestions, and `album_artist` 404'd outright. `MusicVideoArtist` turned out to be a real entity table, so it just joins the existing server-side pipeline as a `Concat` — one bounded `UNION ALL` + `LOWER(...) LIKE ... LIMIT` on both providers. `SongMetadata.Artists`/`AlbumArtists` are the hard case: EF 9 maps them as primitive collections (one JSON array per row in a single column), and neither provider can project the elements server-side — SQLite needs the SQL APPLY operator it lacks, Pomelo MySQL 9.0.0 has no primitive-collection support at all. Both failures are now pinned by a test, so a provider upgrade that fixes them shows up as a red rather than as a stale workaround. For those columns the handler pre-filters on the raw JSON (`LOWER(col) LIKE '%"<encoded-prefix>%' ESCAPE '/'` — a deliberate superset, since it matches a row and not an element), caps the rows at 1000 with an `ORDER BY` that makes the truncation deterministic, then splits and exact-filters in memory. Provider portability is by construction rather than by trusting `LIKE`: the prefix is JSON-encoded before matching (which is how it is stored, and which makes the pattern pure ASCII, so SQLite's ASCII-only `lower()` and MySQL's Unicode-aware `LOWER()` agree) and lowercased in C#, so the match is correct under a case-sensitive MySQL collation as well as a case-insensitive one. The escape character is `/`, never `\`: `ESCAPE '\'` is not a portable SQL literal. An empty `q` stays supported for these fields — the row cap already bounds it, and a non-empty-`q` carve-out would make one group of fields behave differently for the same client code. Docs: new `api.search-field-values-list-columns` record (additive sibling — the base record's rule still reads true; only its body's "intentionally-uncovered gap" claim was stale), plus `api-conventions.md`, `spa-conventions.md` §12 and the regenerated `v1.json`. Decisions-Edit: yes	2026-07-27 03:10:35 +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