3694 Commits

This Branch

This Branch

All Branches

Author	SHA1	Message	Date
timothy	7ca058f83b	fix(668): review round 2 -- remove a second false comment, harden a vacuous assertion ... Re-review of the round-1 fix commit returned MERGEABLE with three findings, all about claims rather than behaviour. All three applied. 1. A stale FALSE parenthetical survived round 1. The docstring on Unicode_Fold_Agrees_With_The_Ordinal_Filter still claimed it catches "one that stops filtering the extras out". It does not. Mutation-verified: delete the Where in FilterSortTake and all EIGHT cases stay green, because each is either a positive SQL alone returns or an ASCII-query negative SQL alone rejects. The same mutation turns the new over-match test RED, so the pair does cover both directions -- but only the corrected wording says so. This is the same species of error round 1 fixed, one paragraph above it; swept by subject this time. 2. Unicode_Fold_Over_Match_Is_Discarded_By_The_Ordinal_Filter asserts an EMPTY result, so it passes vacuously if the fold never runs. Its premises are now asserted explicitly (the query is non-ASCII, and ToUpperInvariant maps ſ to S), so a fold that quietly stopped mapping ſ would fail rather than go green for the wrong reason. 3. The comment on IsSqlite overstated its enforcement. ProviderStaticsWiringTests parses the composition roots for ASSIGNMENTS only; nothing mechanically stops a read of TvContext.IsSqlite here. The real reason stands -- such a read would falsify that test's prose exemption while the test stayed green -- so the comment now says that instead of implying a guard that does not exist. Decisions-Edit: yes	2026-07-27 21:02:24 +02:00
timothy	ce215be590	chore(668): arm the Decisions-Edit trailer, which the earlier commits voided ... The two preceding commits both END with: Refs #668 Decisions-Edit: yes `Refs #668` has no colon, so git does not recognise it as a trailer -- and a single non-trailer line in the final paragraph voids the WHOLE block, taking the valid `Decisions-Edit: yes` with it. Confirmed with interpret-trailers: the pair parses to nothing, while `Decisions-Edit: yes` alone (or `Refs: #668` with a colon) parses fine. `%(trailers:key=Decisions-Edit,valueonly,unfold)` -- exactly what scripts/decisions_validate.py reads -- returned empty, so the `decisions lifecycle` job failed for a real reason, not the known flake. The branch is already pushed, so amending is out (process.pr-routine-sequence). The guard accepts the trailer on ANY non-merge commit in the merge-base range, so this empty commit carries a well-formed one. It is deliberately empty rather than bundled onto an invented change. Decisions-Edit: yes	2026-07-27 20:55:37 +02:00
timothy	ac67c9ee74	fix(668): review round 1 -- make two guards actually guard ... Independent review found both new guard tests weaker than they read. 1. A false docstring. It claimed the SQL stage "genuinely returns 'ſweet' for q='S'". It does not: 'S' is ASCII, so ContainsNonAscii is false and the fold branch is SKIPPED. Those three negative cases exercise the ASCII fast path, which is worth pinning but is not what the comment said -- and the consequence was that NO test drove a row through the fold for the ordinal filter to discard, i.e. the harmless over-match direction the whole design rests on was untested. Comment corrected and Unicode_Fold_Over_Match_Is_Discarded_By_The_Ordinal_Filter added (stored "Sword", q="ſ" -> fold runs, SQL pattern S%, SQLite returns the row, filter drops it, response empty). 2. Unicode_Fold_Escapes_Like_Wildcards could not fail if the %/_ escaping it names were deleted -- the in-memory filter masks the over-match, so the counts stay right. The escaping's real role is preventing LIMIT crowding, so Unicode_Fold_Escaping_Prevents_Limit_Crowding pins that instead. Verified by mutation: with the %/_ replaces removed the new test fails while the original two still pass. Also: the crowding residual in the decision record was attributed to MySQL alone; the SQLite fold shares it in principle, so "no accepted loss" is narrowed to mean no unreachable VALUE rather than a guaranteed count. And a comment says why the provider check is derived per-context instead of reading TvContext.IsSqlite (that static is scoped host-only by ProviderStaticsWiringTests, and reading it here would falsify the exemption). Refs #668 Decisions-Edit: yes	2026-07-27 20:50:48 +02:00
timothy	05542946ad	fix(668): reach accented facet values via a registered Unicode fold on SQLite ... SQLite's LOWER() folds ASCII only -- lower('Édith') is 'Édith' unchanged -- so the EF-sourced facet fields UNDER-matched any stored value whose prefix carries an uppercase non-ASCII character. An under-match is unrecoverable: no later stage can reintroduce a row SQL never returned. Adds a SECOND, ADDITIVE query taken only when the provider is SQLite and q contains a non-ASCII character: raw Dapper SQL folding through etv_upper(), a SqliteConnection.CreateFunction scalar implementing ToUpperInvariant. Every other case -- all-ASCII q, and MySQL for all q -- runs the existing EF query byte-identically. MySQL needed no change and gets none: verified on MySQL 8.4 that its LOWER() is Unicode-aware and its ci collation makes the predicate OVER-match, which the existing ordinal filter already discards. The fold is ToUpperInvariant because OrdinalIgnoreCase equality is a strict SUBSET of invariant-uppercase equality, so the SQL stage yields a superset of the final filter's matches and can never under-match. Note OrdinalIgnoreCase is NOT "invariant-upper then ordinal": ToUpperInvariant('ſ') is 'S', yet "ſweet".StartsWith("S", OrdinalIgnoreCase) is false. Tests pin that. No migration, no model change; both provider snapshots are untouched. Refs #668 Decisions-Edit: yes	2026-07-27 20:36:28 +02:00
timothy	61aa8a902a	test(668): red-first pin for accented values on EF-sourced facet fields ... Stored 'Édith' is unreachable from q=é and q=É on SQLite, because SQL LOWER() folds ASCII only. The stored-LOWERCASE pair is pinned alongside it and passes today, so the fix must supplement that path, not replace it. Red: both uppercase cases return []. Refs #668	2026-07-27 20:08:39 +02:00
timothy	aa1f504e02	Merge pull request 'fix(684): key the pageSize guard registry on identity, not source position' (#686) from fix/pagesize-guard-line-churn into main	2026-07-27 17:39:39 +00:00
timothyandClaude Opus 5	689451161e	fix(684): review round 2 -- drop a false exhaustiveness claim I introduced ... The L1 fix (name every class-b shape) collided with the M1 fix (move SmartCollectionDialog into class-b) in the same commit: the comment claimed "exactly three shapes, which is the whole list" while the registry 80 lines below already held four. That is the same false-exhaustiveness defect this PR exists to remove -- a reader adding a fifth class-b site would conclude theirs cannot be class-b despite rendering a real totalCount hint, and reach for search-bounded or deviation. The operative rule is now stated first and the shape list is explicitly illustrative: a site qualifies by RENDERING a totalCount-backed hint, not by resembling a listed shape. Also, both non-blocking review items: - the deviation prose said the tracking issue goes "in the note", while enforcement is on the structural `issue` field -- it now points at the mechanism that actually binds, and says why the note scrape was rejected; - the UNREGISTERED report prints every position sharing an identity, so it now says "identity seen at:" rather than implying all of them are unregistered. A positionless key cannot know which occurrence is excess; the candidate set is the honest answer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-27 19:17:07 +02:00
timothyandClaude Opus 5	fc8353c75c	fix(684): key the pageSize guard registry on identity, not source position ... The #650 guard pinned every pageSize call site by absolute line:column, making the registry a function of every OTHER file in the repo. The guard was BORN RED. #651 moved AutoTuneScreen.tsx up ten lines and FillerPresetsScreen.tsx down seventy-two, and merged to main BEFORE the guard's own PR (#675) did -- so the registry, authored against a pre-#651 base, was stale the instant it landed. Its own merge run was CANCELLED, so nothing reported it; the red first surfaced on the next push (#676's merge, which touches no web/src file and is not the cause). One ordering accident, not a recurring pattern -- but the exposure is general, because every PR is green against its own base and the breakage exists only in the merge result. Identity is now (file, kind, value). New/removed/changed sites all still fail. The MULTISET comparison is preserved, so a shared identity must be discovered exactly as many times as it is registered. The scanner's positional pageSizeSiteId is untouched: pageSizeScan.test.ts asserts real AST positions against fixed fixtures, the opposite case, with no churn to remove. The one case this costs is stated rather than implied: a same-identity substitution within one file (delete a registered site, add a different unreviewed one with the same kind and value token) now passes. Narrow, and caught only incidentally before. Named in the guard and the record because "costs no coverage" is a claim that outlives whoever made it. Failure reports still print the discovered line:column -- identity and diagnostics need not share a format, and a bare id was useless in a file holding two such sites. Registry reconciled with #651: Playlists/RerunCollections lost their bounded windows to the shared searchLibraryPickerOptions, now registered in its place. Adds a 'search-bounded' class for that shape. Adds a 'deviation' class rather than laundering a live defect into a compliant-looking label. Reconciling the registry surfaced a §3b violation (#685, filed): CollectionsScreen's AddItemsDialog degrades to an unfiltered whole-type window on an empty query and surfaces nothing. Both existing labels would have been false, and either would have made the guard vouch for behaviour that does not exist. Deviation entries must name a tracking issue, enforced by a structural field -- a #\d+ scrape of the note passed with the reference deleted, because notes legitimately cite historical issues. Corrects SmartCollectionDialog to class-b: it does render a totalCount badge, which is class-b's defining evidence. fixes #684 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-27 19:08:26 +02:00
timothy	ac0f65c743	Merge pull request 'docs(649): narrow the base-ref headline to what the checkout actually binds' (#683) from docs/649-narrow-base-ref-headline into main	2026-07-27 16:33:23 +00:00
timothy	c794a48462	docs(649): narrow the base-ref headline to what the checkout actually binds ... The record's bold sentence said the base-ref checkout means the workflow "cannot be rewritten by that same PR to weaken its own judgment". That is true of the SCRIPTS the job executes and false of the workflow itself: Gitea resolves a `pull_request` workflow definition from the PR's own head, so a PR editing `review-verdict.yml` runs its rewritten copy — which can delete the checkout outright, or just post `review-verdict/h10=success` and stop. Branch protection requires the context, not an author, and carries `required_approvals: 0` (#672). The scoping already existed further down, under "What is deliberately NOT claimed". That is not good enough for this particular sentence: it is bold, it is the paragraph a reader resolving this record from the catalog lands on, and someone who stops there leaves with the opposite of the truth. A caveat only works if it is reached. So the headline now says what the checkout binds (the scripts, from the already-reviewed base) and a following paragraph states the head-resolution hole directly, with the superseded claim quoted so a reader who remembers it can see it was retracted rather than wonder whether two records disagree. The later paragraph loses its duplicated opener and points at it instead. Docs only; no behaviour change. Same failure class the rest of #649 kept turning up — a claim stronger than the code — reached this time through prose rather than a test. Refs #649, #672 Decisions-Edit: yes	2026-07-27 18:30:44 +02:00
timothy	aeff810cad	Merge pull request 'test(649): cover the review-verdict status read and the bot-path guards' (#673) from test/649-workflow-body-coverage into main	2026-07-27 16:22:47 +00:00
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	d751f5e01d	Merge pull request 'fix(650): two at-cap list truncations, and a completeness guard that keys on the defect' (#675) from fix/650-at-cap-truncation into main	2026-07-27 06:08:13 +00:00
timothy	400e30a278	docs: record the Decisions-Edit trailer for the parallel-session-claim rationale ... The previous commit rewrote the rationale prose of process.parallel-session-claim (adding the ersatztv#649 double-implementation incident) without this trailer, and CI's decisions-lifecycle gate correctly rejected it. Worth recording WHY it passed locally and failed in CI: I ran decisions_validate.py BEFORE `git commit` in the same command chain, so it inspected the working tree. The trailer check reads COMMITTED history, so the one rule that can only fail after committing was the one I validated before committing. Run the decisions validator after the commit, not before it. Decisions-Edit: yes	2026-07-27 08:03:18 +02:00
timothy	7ed0a59c56	test(649): cold-review fixes — the never-overwrite test skipped the case its docstring called sharpest ... Four gaps, all found by mutation rather than reading. The never-overwrite test used only a NON-EXEMPT file list, so "an exemption posted over a human BLOCKED verdict" — the scenario its own docstring named as the sharp one — was asserted nowhere. Moving the short-circuit to after classification, gated on non-exempt, survived the whole suite while turning a human rejection into a green required status for both a docs-only PR and a bot PR. Now parametrised over non-exempt, docs-only-exempt and bot-exempt file lists. The structural emptiness pin is REPLACED by a real jq-1.6 behavioural test. Its stated justification — "no behavioural test can catch this on a dev machine" — was simply false: this file already imports _JQ16_SHIM for pr-changed-files.sh, so the runner's quirk is reproducible here. The structural version was also weaker than it looked, stripping only FULL-LINE comments, so leaving the literal as a trailing comment on the surviving guard satisfied it while the real check was gone. The behavioural test catches that mutant and needs no comment-stripping. The status-read stub now returns DECOY contexts either side of the verdict row, so dropping `select(.context == $c)` is caught. First attempt gave the decoys `status: success`, which triggers the same short-circuit as a real verdict — the mutation still produced an identical outcome and survived. `pending` decoys make mis-selection observable. DOCS_ONLY's `^` anchor is now covered alongside its `$`: losing it exempts ErsatzTV/docs/Evil.cs, a C# file, and is fail-OPEN. Two remaining survivors are documented in-file as behaviourally equivalent, not gaps: `first` -> `last` (the combined endpoint returns one row per context by contract, so a two-row fixture would test a fiction), and the garbage-response test defending the type guard only by redundancy.	2026-07-27 08:01:36 +02: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
timothy	2a2dcacd58	test(649): cover the review-verdict status read, and the guards that only fire on the bot path ... Additive tests for properties #666 shipped correctly but left unguarded. No behaviour change. The stub's status read was hardcoded to "no verdict yet", so two whole branches of the classify step were unreachable from any test. Four mutations survived the full suite because of it — including re-introducing the literal ersatztv#647 fail-open, and overwriting an existing human verdict. The stub now models a transport error, a garbage body, and an existing verdict. `test_an_EMPTY_enumeration_is_not_exempt_even_for_a_BOT` needs the bot author to test anything: with a non-bot, the blank line an empty list produces already fails DOCS_ONLY, so the `count -eq 0` guard never decides the outcome. On the bot path it is the ONLY thing between an enumeration that read nothing and an unattended success. Verified by mutation — `grep -c .` -> `grep -c ''` grants a bot PR success while every other test stays green. Same short-circuit shape as the PROTECTED/DOCS_ONLY disjointness this file already documents. Two anchors were also unguarded: `grep -qxF` (author `ova` is a substring of `renovate`) and DOCS_ONLY's `$` (`evil.mdx` reads as docs-only). Five of the six mutations are caught behaviourally. The sixth — dropping the shell emptiness check — cannot be caught locally: `jq -e` over empty input exits 4 on jq 1.8 so the guard still fires on a dev Mac, and 0 on the runner's 1.6 where it is the actual bug. A structural assertion closes that gap, with comments stripped first, since a raw substring search is satisfiable by moving the guard into a comment while deleting the real one — verified. refs #649, #672	2026-07-27 07:40:00 +02:00
timothy	31f2a927a2	Merge pull request 'feat(651): library-browse pickers resolve by search, not a 100-row window' (#678) from feat/651-searchable-pickers into main	2026-07-27 03:47:06 +00:00
timothyandClaude Opus 5	66c8500e94	fix(651): pre-merge asks — an empty filter loop asserts nothing; fix "1 item need" ... FIX 1: `PlaylistsScreen.test.tsx`'s group-id test filtered POSTs and asserted inside a `for` loop over the result. On the fixed build that array is empty — the unmatched value leaves the select at '', so Create is disabled and jsdom won't dispatch its onClick — so ZERO assertions ran and the title claimed more than the body proved. A future change that re-enabled Create and POSTed `playlistGroupId: null` would still have passed. Added the unconditional `expect(posts).toHaveLength(0)` before the loop. Worth noting this is the ninth instance of the shape on this branch, and the sibling strengthening in the *same commit* got it right (`RerunCollectionsScreen.test.tsx` uses an unconditional `toHaveLength(0)`) — so the lesson didn't generalize even one file over. The rule is: an assertion inside a loop over a filtered collection proves nothing until the collection's length is asserted. FIX 2: "1 item need a selection" — the noun was pluralized, the verb wasn't, and singular is the common case. My test used `/need a selection/i`, which matches both the right and wrong grammar, so nothing could catch it; it now asserts the exact string '1 item needs a selection'. FIX 3: two comments about clicking an already-disabled button read as contradictory policy. They're not — on the parent the Playlists button was ENABLED, so there the click genuinely discriminates, while the rerun button was disabled on both sides, making it a restatement of `toBeDisabled()`. Both comments now say which case they are and why. Added to #677: the row label falls back to "(no X selected)" on empty `selectedName` regardless of `selectedId` (the mirror image of the bug fixed here), and an all-unbindable group list disables Create with no reason shown. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-27 05:19:49 +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	b93a7d33ff	docs(578): record the update-openapi.sh incremental-skip trap that made my own check vacuous ... Verifying the regenerated OpenAPI artifacts, I re-ran the pipeline against an already-built tree and got a clean git diff — which I nearly reported as "artifacts confirmed". It was a no-op. When the project is already built and unchanged, MSBuild skips the document-generation work but still runs RenameOpenApiFiles (AfterTargets), whose Move then fails with MSB3680 "ErsatzTV.json does not exist" — nothing produced it. The script exits non-zero correctly, but I had piped it (`./scripts/update-openapi.sh 2>&1 | tail -2`), so the shell reported tail's 0 and the failure was invisible. A clean diff after a regeneration that never regenerated proves nothing. Caught it with a positive control: tamper all three artifacts, re-run, see which get restored. v1.d.ts came back (npm run generate:api is unconditional) while v1.json and endpoint-index.md stayed tampered. A `touch` on a compiled source then made the real regeneration run and restore all three byte-exact, which is the verification that actually means something. CI is unaffected — the api-docs job restores into a clean tree, so generation never skips. This is a local-dev hazard only, and it is the same shape as the bug arc this branch is about: a check that reports success without examining anything, exactly what LIMIT was doing to the row bound.	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	17c25e75fa	fix(650): replace node:fs/path/url with import.meta.glob in the pageSize guard ... The gate is npm run typecheck (tsc -b --pretty false, project-mode) — the prior fix for the 3 node:*-unresolvable errors deferred a decision rather than resolving it, per instruction not to change tsconfig unilaterally. Evaluated and rejected: adding "node" to tsconfig.app.json's `types` (makes Node globals type-visible to production browser code, dissolving a deliberate separation the repo documents) and a file-local `/// <reference types="node" />` (empirically leaks Node's ambient `setTimeout` into the whole tsc -b program, breaking 3 unrelated window.setTimeout mocks — confirmed by trying it). Adopted instead: Vite's `import.meta.glob('/src/**/*.{ts,tsx,mts,cts}', { query: '?raw', import: 'default', eager: true })`, resolved natively by Vite/vitest at transform time — no node:fs, no node:path, no node:url, no tsconfig change, no cross-project references, and the guard test stays exactly where it is (`src/api/`). `vite/client` types (hence `import.meta.glob`) were already wired in via `src/vite-env.d.ts`. `isScannableSourceFileName` is unchanged and still the single place the glob's results are filtered through — the extension set moved into the glob literal, but discovery still runs every matched file through this same named, tested predicate rather than a second copy of the logic. Verified the discovery mechanism swap changes nothing observable: a side-by-side comparison test (old fs-based walk vs new glob-based walk, both run against the current repo, not committed — temporary) found byte-identical results — 19 call sites, 136 scannable files, zero set difference in either direction. Re-ran both required plants against the new mechanism: 1. Duplicate at-cap literal appended to builder/ChannelBuilder.tsx (an already-registered file): caught — `UNREGISTERED (1) ... + builder/ChannelBuilder.tsx:2058:54:literal:100`. 2. New file with a `https://` string (no false match), a shorthand `{ pageSize }`, and two literal pageSize values in one ternary's two branches on a single line: all 3 real sites caught at distinct columns — `+ screens/_fakeDefectPlant.ts:4:30:shorthand:pageSize`, `+ screens/_fakeDefectPlant.ts:8:22:literal:111`, `+ screens/_fakeDefectPlant.ts:8:48:literal:222`. Also re-verified the .mts/.cts discovery pin still fails when the extension set is narrowed back to `.ts`/`.tsx` only (2 of 14 predicate cases fail, as before). All three plants reverted after verification; registry/predicate otherwise untouched. Full local gate: `npm run lint` clean, `npm run typecheck` (tsc -b) clean — zero errors, including the 3 node:* ones this commit resolves — `npx vitest run` 110 files / 1078 tests passed (re-run three times; two runs hit transient vitest worker-pool timeouts from overlapping background processes on this machine, unrelated to the code — a clean sequential run passed in full both before and after).	2026-07-27 02:40:31 +02:00
timothy	5b46214774	fix(650): fix 3 tsc -b never-callable errors in gate helpers (project-mode typecheck) ... npm run typecheck (tsc -b --pretty false, the real gate — npx tsc --noEmit was silently checking nothing meaningful due to the root tsconfig.json's empty `files: []`) flagged 3 errors: `let x: (() => void) | null = null` gate-release helpers, reassigned only inside a Promise executor, narrowed a later `x?.()` call to `never` under tsc -b's project-mode control-flow analysis. Not reproducible under a bare `tsc --noEmit` invocation. Fixed by adopting the no-op-initializer pattern already established elsewhere in this repo (api/libraries.test.ts's `releasePost`): declare as `let x = () => {};` instead of `(() => void) | null = null`, dropping the now-unnecessary optional chain at each call site. Same runtime behavior, no `as any`/`@ts-expect-error`, no change to what any test asserts. 3 call sites fixed: releaseStrandedAppend (F3 test), and releaseB0 in both the inverse-settlement-order test and the round-4 hook-level single-flight test. Verified: `npm run typecheck` no longer reports these 3; `npm run lint` clean; `npx vitest run` 110 files / 1078 tests still pass. 3 more typecheck errors remain (node:fs/node:path/node:url unresolvable in src/api/pageSizeCallSites.guard.test.ts, the only file under src that needs real Node fs access) — deliberately NOT fixed here. @types/node is already a devDependency and physically installed, but tsconfig.app.json (covering all of src) has no "node" in its `types` array, and a file-local `/// <reference types="node" />` was tried and reverted: under tsc -b's single-program compilation, it leaked Node's ambient `setTimeout` (returning NodeJS.Timeout) into the whole app project, breaking 3 unrelated window.setTimeout mocks expecting the DOM signature (confirmed by trying it — TS2345 in ChannelsScreen.test.tsx/LibrariesScreen.test.tsx/ PlayoutsScreen.test.tsx). The clean fix is a real project-config change (either widen tsconfig.app.json's types, accepting Node globals become type-visible in browser app code too, or move this one file into tsconfig.node.json's project alongside the e2e specs, which would also need a "references" wire-up for its cross-project import of pageSizeScan.ts) — left for the coordinator to choose per their explicit instruction not to make this call unilaterally.	2026-07-27 02:08:19 +02:00
timothy	937ee92a3f	fix(650): close two test-adjacency gaps — hook-level single-flight pin, predicate-level glob pin ... Fourth cold cross-family review: no runtime correctness finding this round (single-flight held against synchronous throws, generation overlap, StrictMode, unmount, and separate hook instances). Two test-gap findings remained, both the same shape as prior rounds' review feedback: asserting on something ADJACENT to the mechanism rather than the mechanism itself. 1. ChannelBuilder.test.tsx's page-0 single-flight test asserted on the disabled BUTTON — during a page-0 refresh, `loadingMore` disables the native button, so `fireEvent.click` never reaches `loadMore` at all. Deleting `busyRef.current = true` at libraryBrowse.ts's generation-change effect would leave that test green while direct hook calls could still overlap page 0 and page 1. Added a hook-level test that calls `result.current.loadMore()` directly (via `renderHook`, bypassing the button/DOM layer) while a gated page-0 fetch is in flight, and asserts no page-1 request is ever issued. Verified: removing `busyRef.current = true` at that line makes the new test fail with `expected [ +0, 1 ] to deeply equal [ +0 ]` (a page-1 request was issued when the guard should have ignored the direct call); restored, green again. 2. pageSizeScan.test.ts's `.mts`/`.cts` tests exercised the SCANNER'S PARSING, not the guard's file-discovery glob — reverting the glob back to `.ts`/`.tsx` left both those tests green (the scanner still parses a `.mts`/`.cts` filename as plain TS regardless of extension) AND the real-source guard green (this repo has no committed `.mts`/`.cts` file for the reverted glob to miss). A prior verification planted a real `.mts` file and watched the guard notice it, which proved the behavior exists today but pinned nothing going forward. Extracted the inline glob condition into a standalone, exported `isScannableSourceFileName` predicate and added 14 parametrized cases asserting it BY FILENAME (no filesystem involved) — `.ts`/`.tsx`/`.mts`/`.cts` accepted, their `.test.*` and `.guard.test.ts` variants excluded, non-TS extensions rejected. Verified: reverting the predicate's regex back to `.ts`/`.tsx` only makes 2 of the 14 cases fail (`isScannableSourceFileName('*.mts')` and `'*.cts'` both wrongly return `false`); restored, green again. Nothing else changed — the reviewer confirmed no further findings (loader async/throw handling, generation sequencing, the unconditional rollback, StrictMode/unmount behavior, the disabled-during-refresh UX, type-only node exclusion, computed-name exactness, wrapped-forwarded-call rejection, and all existing test labelling were all confirmed correct as-is). Full local gate: lint clean, tsc clean, full vitest run 110 files / 1078 tests passed, re-run twice for stability (no flakes).	2026-07-27 01:29:15 +02:00
timothy	1c86a1c1fc	fix(650): enforce single-flight in useLibraryBrowse; close scanner false negatives ... Third cold cross-family review (BLOCKED) found the append/page-0-refresh races were being fixed one interleaving at a time — round 1 fixed page-0-settles-first, round 2's compare-and-set rollback fixed the duplicate-append case but introduced a permanently-skipped page, and the reviewer found the exact mirror of round 1's fix (page-1-settles-first, erasing page 1 with no cursor reset). Direction from the review: stop enumerating orderings, make the overlap structurally impossible. SINGLE-FLIGHT (web/src/builder/libraryBrowse.ts): a new `busyRef` guard is true from the moment ANY fetch (a page-0 refresh OR an append) for the current query generation is issued until it settles. `loadMore` checks it SYNCHRONOUSLY and returns immediately (ignored, not queued) if a fetch is already in flight — including a page-0 refresh, not just a prior append, so a "Load more" click that lands while a query change is still resolving is a no-op rather than starting a second, overlapping request. With overlapping fetches eliminated by construction, the append-failure rollback no longer needs the round-2 compare-and-set: single-flight guarantees nothing else could have moved `pageRef` since a given fetch started, so it now always rolls back and retries the exact page that failed, unconditionally. Visual feedback (the button showing loading/disabled during a page-0 refresh, not just an append) is set via `queueMicrotask(() => setLoadingMore (true))` rather than a bare synchronous call in the generation-change effect — `react-hooks/set-state-in-effect` flags the latter; a microtask-deferred call resolves before any human-perceptible input, satisfies the lint rule (the same reason `.then()` callbacks elsewhwere in this hook aren't flagged), and keeps the actual correctness guarantee (the ref check) perfectly synchronous regardless. TESTS REWRITTEN, not just added — the round-2 "HIGH-2" hook test explicitly asserted the NEXT request after a failed page 1 (following an overlapping page 2 success) should be page 3, i.e. it blessed page 1's permanent loss. Replaced with two hook-level tests: single-flight ignores a synchronous double `loadMore()` call (only one fetch issued), and a failed page is retried as the SAME page number. Replaced the round-2 component-level "HIGH-1" test (which drove the now-impossible overlap through the DOM) with one asserting the click during a pending page-0 refresh is ignored, and that once free, the correct page-1-then-page-2 sequence completes with both pages' rows present. Verified all three new/rewritten tests against the prior committed hook (7b1ae48b0): the two single-flight-specific tests fail as expected (`[0, 1]` requested when only `[0]` should have been); the retry-semantics test happens to pass against 7b1ae48b0 too (compare-and-set and unconditional rollback coincide in the non-overlapping case) but is kept because it is the correct "retry as page 1, not page 3" pin the review asked for, replacing the one that asserted the wrong thing. SCANNER (pageSizeScan.ts) — closed three documented false-negative classes: - Transparent TS wrappers around the initializer (`pageSize: 100 as const`, `100 satisfies number`, parenthesized) are now unwrapped before the NumericLiteral/Identifier check. - Non-Identifier property names: a quoted string key (`'pageSize': 100`) or a statically-resolvable computed key (`['pageSize']: 100`) are now accepted; a computed key that isn't a literal correctly stays unresolved. - `.mts`/`.cts` are no longer silently excluded from the guard's file discovery glob (tsconfig.app.json's `include` covers all of `src`; no such files exist in the repo today, but the glob shouldn't hide one if it ever does). 10 new fixture tests in pageSizeScan.test.ts pin each case (plus a rejection test confirming a forwarded call wrapped in `as` still doesn't match, and one confirming an unresolvable computed key stays unmatched). TEST LABELLING: relabeled the URL/M-3 and `??`/M-4 fixtures as CONTRACT fixtures rather than regression pins — a round-3 review found round 1's plain literal regex already handled those two exact inputs correctly on its own; only the combined multi-case fixture (and the string-contains-text, template-interpolation, same-line-identity, JSX, and destructuring fixtures) actually fail against round 1. Labeled the guard test's 4 tests as BASELINE assertions (they all pass on clean b90f8a3b) rather than implying they prove this round's specific fixes — pageSizeScan.test.ts's fixtures are what actually regression-pin the scanner. No server-side/C# change. Full local gate: lint clean, tsc clean, full vitest run 110 files / 1063 tests passed (re-run twice, stable).	2026-07-27 01:29:15 +02:00
timothy	ca99bedb1a	fix(650): rewrite the pageSize guard on the TS compiler API; fix two append-ownership races ... Second cold cross-family (Codex, BLOCKED) re-review of b90f8a3b found the regex/bracket-tracking guard scanner still defeated in five ways, and two new High-severity races introduced by the F3/F4 fixes. Addressed as a further follow-up (b90f8a3b left untouched). GUARD REWRITE (per the review's explicit direction — stop patching the regex, use the compiler): - New `web/src/api/pageSizeScan.ts`: `scanPageSizeSites` parses each file with `ts.createSourceFile` and walks the real AST for `pageSize` PropertyAssignment/ShorthandPropertyAssignment nodes inside an ObjectLiteralExpression. This eliminates categorically (not case-by-case): - M-3: comments and string/template CONTENTS are never revisited as code, so a `'https://...'` string can't be misread as an unterminated string that swallows the rest of the file. - M-4: an object literal nested in a ternary, `??`, or JSX expression container is still found — the walk visits every descendant node regardless of the syntactic context above the ObjectLiteralExpression. - M-5: template-literal interpolations are real AST children, not opaque text. - L-7: a type literal (`type P = { pageSize: 100 }`), an interface PropertySignature, and a destructuring ObjectBindingPattern (parameter or nested) are structurally different node kinds from ObjectLiteralExpression — excluded by kind, not by a preceding-character heuristic a stray `{`/`(`/`,` could fool. `getLineAndCharacterOfPosition` gives exact line+column (fixes M-6 identity granularity) instead of the prior line-only identity. - `pageSizeCallSites.guard.test.ts` now imports the shared scanner; identity is `file:line:column:kind:value`, compared as a MULTISET (count, not membership) in both directions. - Both directions (unregistered / stale) are computed and folded into ONE thrown Error so a failure always shows the complete picture in one run, addressing the line-churn "second direction never renders" concern. - New `pageSizeScan.test.ts`: a FIXTURE test (inline source strings, no repo scan) pinning the exact discovered set for every case the review named — comment-in-string, string containing the literal text `pageSize: 100`, template interpolation, ternary, `??`, JSX container, same-line duplicates, parameter/nested destructuring, a type literal, an interface property, a forwarded call expression, a React dependency array. This is what actually protects the scanner going forward — the guard test alone only ever proved today's snapshot of real call sites, never the scanner's handling of input classes it hadn't happened to encounter yet. - Re-verified both original plants (a duplicate at-cap call in an already-registered file, and a new file with both a literal and a shorthand site) against the rewritten scanner; both still fail with the new combined-direction message. Also verified a run with BOTH directions simultaneously non-empty renders both in one report. HIGH-1 (ChannelBuilder.tsx useLibraryBrowse, now web/src/builder/libraryBrowse.ts): `reqId` identifies a query GENERATION, not an individual fetch — a page-0 refresh and a "Load more" append can be outstanding simultaneously under the same reqId (query changes while an append is in flight for the new generation). Whichever settled first used to clear `loadingMore`, letting a second click fire an out-of-order/duplicate page fetch. Fixed with a per-fetch `fetchId` plus a `loadingFetchIdRef`/`loadingFetchReqIdRef` pair: only the fetch that OWNS the currently-displayed spinner can clear it; a same-generation page-0 refresh leaves a same-generation append's spinner alone, while a page-0 refresh for a NEW generation still retires an abandoned OLDER-generation append's spinner (preserving the original #650 F3 fix). Reproduced the exact interleaving from the review in a new test (gate B's page-0 and page-1 fetches independently, click "Load more" while B's page-0 is still in flight) and confirmed it fails without the fix (button re-enables while the append is still pending). HIGH-2 (same file): the append-failure rollback mutated whatever `pageRef.current` currently held, rather than the specific page THIS fetch requested — under an overlapping-append race, a later page's success followed by an earlier page's failure could roll the cursor back past already-appended progress, corrupting a retry into refetching a duplicate. Fixed with a compare-and-set guard (`if (pageRef.current === pageNum)`) so the rollback only fires when nothing has advanced the cursor since. Since this overlap is UI-unreachable once HIGH-1's single-flight disabling is wired up (verified empirically: two synchronous fireEvent.click calls in RTL only produce one request, since act() flushes the disabling render between them), the regression test drives `useLibraryBrowse` directly via `renderHook` (now exported) to force the exact interleaving and confirms it fails without the fix (page 2 gets duplicated, page 3 never requested). Extracted `useLibraryBrowse` (plus `loadCollections`/`loadLibraryItems`/ `BrowseState`/the media-type const arrays) into a new non-JSX module `web/src/builder/libraryBrowse.ts` — exporting a hook from a .tsx file tripped `react-refresh/only-export-components`; this also makes the hook importable by `renderHook` without pulling in the whole screen component. No server-side/C# change. Full local gate: lint clean, tsc clean, full vitest run 110 files / 1051 tests passed (one LibrariesScreen.test.tsx flake reproduced under full-suite parallel load, confirmed pre-existing and unrelated — passes in isolation, never touched that file).	2026-07-27 01:29:15 +02:00
timothy	2d049e9a28	fix(650): follow-up — per-occurrence guard identity, shorthand pageSize detection, and three UI defects ... Cold cross-family (Codex) review of 9763fdca found real defects; addressed as a follow-up rather than amending that commit. MUST FIX, addressed: - F5: pageSizeCallSites.guard.test.ts collapsed call-site identity to `file:value`, so a SECOND at-cap call in an already-registered file was invisible (verified: appending a duplicate `getLibraryBrowseItems({ mediaType: 'Movie', pageSize: 100 })` to ChannelBuilder.tsx passed all 4 guard tests before this fix). Identity is now `file:line:kind:value` — a bracket/quote-tracked scan resolves each occurrence's exact line, so a duplicate on a new line is a new, unregistered identity. - F6: the guard now also detects the ES6 shorthand property form (`{ ..., pageSize }`), not just `pageSize: <value>`. Implemented as a bracket-stack scan that distinguishes an object-literal `{` (real risk) from a block-statement `{` or an array `[` (false positives from things like `useCallback` dependency arrays and `const pageSize = 100;` inside a function body) by inspecting the token immediately preceding each `{`. Six real shorthand sites are now registered (the two inside loadAllPages itself, ChannelBuilder's two per-kind fan-outs, and two genuine user-adjustable pagers in LogsScreen/BlockPlayoutTroubleshootingScreen). Object SPREAD and positional-argument pageSize (api/search.ts's api.search-allitems-paging precedent) remain a documented residual gap, written down in the test file's own header comment, not silently absent. - F2 (TraktListsScreen.tsx): an incomplete load with zero accumulated rows rendered BOTH "List may be incomplete" and the unsupported "No Trakt lists yet." claim. The zero-row empty state now branches on `incomplete` first. - F3 (ChannelBuilder.tsx useLibraryBrowse): changing the query/library while a "Load more" append was in flight stranded the button in its loading/disabled state forever (the stale append's own `finally` no longer matched the current request id, and the superseding fresh fetch never cleared `loadingMore` either). `finally` now clears `loadingMore` whenever the settling request is still the CURRENT one, regardless of whether that particular request was an append. - F4 (ChannelBuilder.tsx useLibraryBrowse): one rejected per-kind request in an append's `Promise.all` wiped every already-loaded row via `items: []` with no way back. Append failures now preserve state, surface the error inline next to a still-present "Load more" button, and roll the page cursor back so a retry re-requests the same page instead of skipping it. Both new UI fixes are pre-existing defects in the 'library' source that 9763fdca's loadCollections fix newly made reachable from 'collections' too. Verified all four fixes against negative controls: reverted each in turn and confirmed its dedicated test fails with the expected message, then restored. DO NOT FIX (filed as timothy/ersatztv#665 instead, bug+frontend+priority:low): - F1: loadCollections/loadLibraryItems sort each fetched page independently, so appended pages are only locally sorted, not globally sorted across the accumulated list. - F7: an overclaiming totalCount can leave "Load more" clickable after every kind is actually exhausted (no auto-loop; a user click is still required each time). Not touched (reviewer confirmed correct as-is): Trakt sequence/abort/unmount handling, the Class-A vs Class-B incomplete-copy distinction, and the `lists.length` footer count.	2026-07-27 01:29:15 +02:00
timothy	ad4ac6c7e0	fix(650): page Trakt lists to completeness and report real totals in loadCollections ... Two SPA list loads requested EXACTLY the server's pageSize cap (100), truncating identically to #634/#644's over-cap defect but invisible to that fix's manual "pageSize above the cap" grep: - TraktListsScreen requested pageSize:100 and rendered BOTH the truncated page AND the real totalCount, so 101 lists showed as "101 lists" over a 100-row table. Trakt lists are bounded-by-construction (Class A), so this now pages to completeness via the shared loadAllPages helper, surfaces an "incomplete" badge if a page ever comes back short of totalCount, and passes an AbortSignal from the effect cleanup. - ChannelBuilder's loadCollections (fanning out per collection kind) reported the truncated merged.length as totalCount, so canLoadMore's `items.length < totalCount` comparison was permanently false and "load more" could never fire. It now sums the real per-kind totalCount, mirroring the existing loadLibraryItems pattern in the same file, and canLoadMore is no longer gated to the 'library' source only. Also found and fixed a third at-cap site not named in #650: ChannelBuilder's SeasonsDialog (TelevisionSeason browse scoped to one show) reads pageSize:100 but never read the response's totalCount. No real show has 100+ seasons, so this stays a single bounded page (Class B) rather than paging to completeness, but now surfaces a "Showing the first N of M seasons" hint instead of silently truncating if a show somehow exceeds the cap. Codifies the missing completeness guard as an enumerating allow-list vitest test (web/src/api/pageSizeCallSites.guard.test.ts): scans every `pageSize:` call site in the SPA and diffs it against a hand-reviewed registry in both directions (unregistered site = new defect risk, stale entry = registry rot), with anti-vacuity floors on files-scanned and sites-discovered. Verified the guard actually fails on a planted defect and a planted stale entry before finalizing it. No server-side change: the client pages, the server stays bounded (api.search-allitems-paging precedent).	2026-07-27 01:29:15 +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	8dcd4f3602	Merge pull request 'fix(632): bind a review verdict to its BASE branch, not only to its head sha' (#667) from fix/632-verdict-base-ref into main	2026-07-26 21:58:49 +00:00
timothy	e960d5b918	test(649): make the POST-wiring assertion unable to opt out or accept the wrong host ... Re-review found the verifier could disable itself two ways, both of which look like coverage: - it was guarded by `if url_file.exists()`, so deleting the recorder in the stub turned the whole assertion into a no-op and every test stayed green; - it compared only the URL SUFFIX, so a POST to the right path on the wrong HOST or the wrong REPO passed — which is exactly the class the assertion was added to catch. It now requires the URL to have been recorded whenever a status was posted, and compares the full URL against the env the job was given. Mutation-verified three ways: wrong host, wrong repo, and deleting the recorder each redden the suite. Refs #649	2026-07-26 23:38:25 +02:00
timothy	ed8de77e10	fix(632): validate status ROWS, not just the top-level array — the same swallow one level down ... Re-review caught my previous fix claiming more than it delivered. "Every unreadable input asks" was false: validating only that `.statuses` is an array left `{"statuses":[1]}` passing the guard, after which `.context` on a number errors and the `|| true` on the extraction turned that error into an empty description — straight back onto the graceful-adoption path the guard exists to distinguish from. The identical swallow-the-error shape I had just fixed a few lines up, surviving one level deeper. The validation domain now matches the CONSUMPTION domain: every row must be an object with a string `.context` and a `.description` that is absent or a string. The extraction drops its `|| true` and asks explicitly instead, since a swallowed error there is indistinguishable from a benign "no base recorded". Both guards are load-bearing, for DIFFERENT shapes — established by mutating them together and separately rather than assuming the pair was redundant: - a non-string `.description` is caught ONLY by the row validation (jq -r renders the object as JSON, the sed finds no `(base: …)`, and it silently reads as a legacy verdict); - a scalar row is caught by EITHER, so with the validation weakened the extraction guard is what still asks. Also noted rather than changed: this is the third read of the same status endpoint in a worst-case hook run. Sharing one snapshot would close a narrow same-run disagreement window, but the other two branches derive different decisions from a failed read, so threading a shared response through them changes pre-existing logic rather than #632's. Recorded in place so it is not rediscovered as an oversight — every `decide` exits immediately, so the reads cannot produce one self-contradictory message. Refs #632	2026-07-26 23:38:14 +02: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