679 Commits

Include renames

Author	SHA1	Message	Date
timothyandClaude Opus 5	980da6db00	chore: ignore .codex/, and stop shipping a plaintext credential in docs ... .codex/ is generated by `codex exec` as a machine-local mirror of the .claude hooks. It is deliberately NOT tracked even though .claude/ is (17 files): its config.toml embeds a plaintext Gitea credential and absolute /Users paths, so committing it would leak the credential and would not be portable anyway. Ignoring it also unblocks scripts/refresh-shared-checkout.sh, which refuses on a dirty tree. Separately, docs/handoffs/rest-api.md carried the same credential inline; it now references $ETV_GITEA_BASICAUTH like every other doc. NOTE this does not purge git history — the literal appears in 12 earlier commits and is still recoverable there. Refs: #698 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-30 21:59:27 +02:00
timothyandClaude Opus 5	57e33f9937	chore(698): drop a trailing blank line at EOF ... Nit from review round 6 (git diff --check). No behaviour change. Refs: #698 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-30 21:02:41 +02:00
timothyandClaude Opus 5	fe00e0d71f	fix(698): compare the recorded base exactly, never parse it out ... Review round 5 returned BLOCKED with one High, and it needed no forgery and no #697 — just a branch name. `main)evil` IS A VALID GIT BRANCH NAME (`git check-ref-format --branch 'main)evil'` succeeds). A genuine human verdict earned while head H targeted it is written `(base: main)evil)`. Truncating at the first `)` yields exactly `main`, which matches a PR that has since been retargeted onto `main`, so the verdict is inherited over a completely different diff. I had asserted the opposite in a code comment one commit earlier — that a `)` in a branch name "mismatches — safe direction". That was generalised from `feat/foo)bar`, which does mismatch, and is false for EVERY branch whose name starts with the target base. Two attempts at extracting this value have now been defeated (`##` last-marker by an appended marker, `#` first-marker by this), so the lesson is the shape, not the off-by-one: do not parse a value out of user- or attacker-influenced text when you can compare against the exact expected literal instead. The description must now END with the literal `(base: <this PR's base>)` AND contain exactly ONE marker — the marker count kills the append trick without having to decide which occurrence is authoritative. Pure shell (`${#}` arithmetic), no truncation to abuse. Verified across all six shapes, including a PR that legitimately targets `main)evil` (accepted) and `(base: )` (rejected). Absent markers remain accepted, since verdicts predating #632 carry none. Mutation-verified: restoring the truncating parse reddens only the new paren test, while the appended-marker, matching-base and legacy tests stay green. 385 tests pass. Note for the record: pytest has never executed inside the review sandbox in any of the five rounds, so the suite has only ever been run here. Refs: #698 Decisions-Edit: yes Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-30 20:51:57 +02:00
timothyandClaude Opus 5	e7bae06385	fix(698): review round 5 — a human verdict formed against ANOTHER base is no longer inherited ... Round-4 cross-family review returned BLOCKED with a single Medium; the three round-3 items were confirmed actually fixed. THE SHA-BINDING WAS ESCAPABLE THROUGH THE HUMAN PATH, not the exemption path. The short-circuit identified a human verdict by creator + `Review-verdict:` prefix and then exited before looking at the base. So: earn a GENUINE `success` on head H while it targets a scratch base with a benign diff, then retarget H onto `main`, where its diff carries unreviewed code. Creator real, prefix real, status inherited — a green required check over code nobody reviewed. `post-review-verdict.sh` has recorded the reviewed base in the description since #632; this gate simply never read it. The merge-consent hook did compare it, but that is advisory and covers only its own path: a merge through the Gitea UI or API sees nothing but the status. The gate now rejects a verdict whose recorded base differs from the PR's. An ABSENT base is deliberately NOT a mismatch — verdicts predating #632 carry none, and re-deriving over one would un-approve a genuinely reviewed head. Only present-and-different is rejected, which is exactly the escape. Tests: the mismatch case, plus two positive controls (matching base still short-circuits; a legacy no-base verdict still short-circuits) so the check cannot pass by blanket rejection. Mutation-verified: removing the check reddens only the mismatch test. Also from round 4: sharpened the docstring of test_the_classify_step_runs_without_SHELL_ERRORS. It catches guards that die NOISILY; it is not a general liveness check, since a clean mutation like hardcoding n_protected=0 emits nothing. The branch-discriminator test is the actual liveness guard. Claiming otherwise would have made a cheap net look like a strong one. And fixed a dangling decision key I had just introduced: the base-in-description convention belongs to `release.verdict-status-check`, not the `ci.verdict-records-base` I invented — the breadcrumb hazard our own retrieval rules warn about. 382 tests pass. Refs: #698 Decisions-Edit: yes Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-29 21:44:44 +02:00
timothyandClaude Opus 5	d4c600149d	fix(698): review round 4 — the PROTECTED guard was DEAD; define before use, fail closed, fix prescriptive docs ... Round-3 cross-family review returned BLOCKED with 3 Mediums. The first was serious and self-inflicted. THE PROTECTED GUARD WAS A NO-OP. Round 3's `count_matching` / `count_not_matching` helpers were defined AFTER the classification chain that calls them, so `count_matching` was `command not found` on every run, `$( )` yielded an empty string, `[ "" -gt 0 ]` errored, and the `elif` was simply skipped — the protected-path check never executed at all. Confirmed by direct execution before fixing. Three "protected path" tests stayed GREEN throughout, because a protected path is also not a manifest and not docs-only, so the job still reached `pending` down a different route. Asserting the STATE could not distinguish a working guard from a dead one. The mutation battery missed it too: I had mutated the predicates, not their reachability. Fixed three ways: * helpers are defined immediately after `gh()`, before any use; * the three counts are evaluated ONCE at TOP LEVEL and validated numeric, because `exit 1` inside `$( )` leaves only the subshell and, with the substitution sitting in a conditional, `set -e` never fires either — so a grep error had been silently reading as "no match". A non-numeric result now aborts with nothing posted, and an absent required check blocks the merge; * the helpers return a non-numeric sentinel instead of trying to `exit`. Verified: an invalid regex now exits 2 and posts NOTHING (previously it classified and posted). Renaming the helper at its definition turns six tests red. TESTS, aimed at the failure mode rather than the symptom: * assert the DISCRIMINATOR (the job's `Decision:` reason line), not the outcome — when several branches yield the same verdict, the verdict cannot tell you which ran. A first draft of this test asserted the status description and failed against a WORKING guard, because for `pending` the description is constant; * a cheap stderr sweep for `command not found` / `integer expression expected` / `unbound variable` across four representative PR shapes. Each of those makes an `if` condition merely false while the job exits 0 and posts a plausible status, so this catches a whole family of silently-skipped guards. DOCS. The record and ci-cd.md still PRESCRIBED the here-string that round 3 removed — following them would have reintroduced the temp-storage failure. Both now prescribe counting, define-before-use, top-level evaluation and numeric validation. The workflow's measurement paragraph still said the npm manifests "are included" three lines above the note saying they are excluded; corrected. 379 tests pass. Refs: #698 Decisions-Edit: yes Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-29 21:13:56 +02:00
timothyandClaude Opus 5	d8bd1dcba9	fix(698): review round 3 — count instead of matching, re-read before the POST, fix stale docs ... Round-2 cross-family review returned BLOCKED: 2 High + 3 Medium. HIGH — here-strings traded one fail-open for another. `grep -q… <<< "$data"` fixes the SIGPIPE inversion, but bash materialises a large here-string via temporary storage, so it fails when temp space is full or unwritable — and since these sit inside `if`/`!`, that failure flips the predicate exactly as SIGPIPE did. It did NOT reproduce on my bash 3.2, DID on the reviewer's Linux bash 5.x, and CI is Linux; the disagreement is itself the argument for a construct that cannot fail either way. Path predicates now COUNT with `grep -c`, which drains stdin (no early exit, no SIGPIPE) over an ordinary pipe (no temp file), and grep's status is read honestly: exit 1 means "zero matches", a legitimate answer, while >1 is a real error that FAILS THE JOB rather than silently reading as "no match". `set -e` does not catch these on its own — they sit in command substitution inside a conditional. Verified correct under 171KB input AND an unwritable TMPDIR. The description test became a `case` prefix match, removing another pipeline from a security predicate. New record `ci.grep-q-pipefail-inversion` covers the whole class. HIGH — a human verdict landing mid-run was still overwritten, and the code claimed otherwise. The job read statuses once, classified over several round-trips, then posted: a reviewer posting BLOCKED in between had it replaced by an exemption `success`, turning an explicit rejection into a merge. Added a re-read immediately before the POST which refuses to write over a human verdict found then. The heading no longer says "never overwrite" — it cannot promise that, since there is no compare-and-set on Gitea's status API. Remainder tracked as #706. MEDIUM — documentation was stale in three places, all mine. The record's frontmatter `rule:` still listed the npm manifests (I fixed the body and forgot the frontmatter, so the canonical rule AND the generated catalog were wrong); docs/ci-cd.md still said `edited` was absent from `types:`, contradicting a section I had just updated; and the workflow header still implied the `edited` re-run settles the ABA race. All corrected to say detection, not atomicity. TESTS. 373 pass. New: a mid-run human verdict via a status stub that returns nothing on the first read and BLOCKED on the re-read, and large-input regression tests for the ADVISORY hook, which had none — the copy with less authority is the one that quietly keeps a bug (#649's whole point). Mutation-verified: reverting the hook predicate, the workflow predicates, or the pre-POST re-read turns exactly the intended tests red while every positive control stays green. Refs: #698 Decisions-Edit: yes Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-29 20:53:36 +02:00
timothyandClaude Opus 5	bafb487eaa	fix(698): review round 2 — close a PRE-EXISTING SIGPIPE inversion, narrow the bot allow-list, correct two overclaims ... Cross-family adversarial review returned BLOCKED with 3 Blockers + 2 Highs. Two were verified independently before acting on them. BLOCKER 1 (pre-existing on main, not introduced here). `producer | grep -q…` inverts under `set -o pipefail`: grep exits at its first match, the producer takes SIGPIPE and exits 141 once the list exceeds the pipe buffer, so a MATCH is reported as a FAILED pipeline. Reproduced at 1901 paths / ~171KB, inside the enumerator's 2000-file cap: * `A.cs` + 1900 docs paths -> docs_only=yes, status 141 (a C# PR exempted) * `.gitea/…` + 1900 docs -> the PROTECTED guard MISSES entirely The second reaches a green required status on a PR rewriting the gate, with no retarget, no bot account and no forged status. This construct shipped with the original docs-only check (review-verdict.yml:257 on main), so it was live. Fixed in both the enforced gate and the advisory hook by using here-strings, which have no writer to signal. New record `ci.grep-q-pipefail-inversion`. BLOCKER 2 (mine). `web/package.json` / `web/package-lock.json` are removed from BOT_MANIFESTS. renovate.json enables only nuget/github-actions/dockerfile, so npm is unmanaged here and the entry bought nothing — while package.json `scripts` are EXECUTED by CI (npm ci, npm run build). It widened an exemption onto a code-execution path for no benefit. BLOCKER 3 + HIGH (documentation was wrong, code unchanged). The claim that `edited` made the retarget residual "non-durable" is retracted: runs are not serialized, so a stale run can post `success` after the reclassifying run posts `pending`. The ABA transition is narrowed and observable, NOT closed. Likewise the provenance check asks "posted by a user credential", not "posted by a reviewer" — ETV_STATUS_AUTH is basic auth, so a #697 forgery gets a non-null creator AND an attacker-chosen description and is preserved as human. Both now stated at full strength. TESTS. 4 large-input cases crossing the pipe buffer, each paired with a large-input POSITIVE control so "large lists now fail closed" (a deadlock) cannot pass as a fix. Verified by mutation: reverting the here-strings turns all three negatives red while the control stays green. Two of my own weak tests fixed — the "base advances" case called head_moves_to(SHA) with the already-current sha (a duplicate positive control, now a structural assertion that the comparator is .base.ref and never .base.sha), and the arity test counted five arguments without checking the fifth was the base. The whole class was invisible because every previous test used a handful of short paths: a guard whose behaviour depends on a buffer threshold needs a test that crosses it. Refs: #698 Decisions-Edit: yes Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-29 20:35:29 +02:00
timothyandClaude Opus 5	f523fc535d	fix(698): bind the base, constrain the bot exemption by content, re-derive unattributable successes ... The `review-verdict/h10` exemption path decided from mutable or unattributed PR state, and a machine-written `success` was never revalidated. Three routes, one root cause, so one change. Route 1 (reproduced live as probe PR #703, closed unmerged): `/pulls/{n}/files` diffs against the PR's LIVE base, so retargeting moves the answer without moving the head sha. A PR opened into `main` and retargeted mid-run enumerated docs-only and was granted `h10=success` while its diff against `main` carried a C# file; retargeting back reclassified nothing. `scripts/pr-changed-files.sh` now takes the expected base branch as a REQUIRED 5th argument (optional would be a silent opt-out) and checks it before and after paging; the workflow passes it from the `pull_request_target` payload, which a retarget cannot rewrite, and `edited` is in `types:` so a retarget reclassifies. A pinned two-sha diff would close route 1 outright but Gitea 1.25.4 cannot serve one: `compare/{base}...{head}` returns no `files`, and a `--depth=1` fetch of the two shas has no merge base. Measured, not assumed. The residual window is stated in the code and the record rather than papered over. Route 2: `pull_request.user.login` is the PR's immutable CREATOR while its head is not, so pushing code onto an open Renovate branch kept the exemption. The bot exemption now also requires EVERY path to be a dependency manifest — a set measured across all 11 Renovate PRs this repo has had, not guessed. Route 3: the never-overwrite short-circuit exited on ANY `success`, so a forgery obtained once was inherited forever. It now fires only for a status positively identified as a human verdict (non-null `.creator.login` AND a `Review-verdict:` description — measured: user-posted statuses carry a creator, Actions-posted ones carry null). Written in the positive direction so an unrecognised shape is re-derived rather than trusted. The two exemptions are composed, not chained: as an `elif` chain a Renovate docs-only PR lost the docs-only exemption. Caught before commit and pinned by a test. Tests: 17 new cases in scripts/tests/test_pr_changed_files.py, each verified by mutating the clause it covers (8 mutations, 8 kills). Both records trimmed under the 60-line prose ceiling so the corpus tail-boundary check stays calibrated. Does NOT close the class: anyone who can POST a status directly can still impersonate a verdict — that is #697, deliberately left open. Refs: #698 Decisions-Edit: yes Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-29 19:52:00 +02:00
timothy	ceef16081d	docs(672): make the self-test gap discoverable (signals + section pointer) ... Re-review of the round-2 head returned MERGEABLE with one LOW: the record's `signals:` field did not mention the self-test gap. That field is the semantic discovery surface -- it is what the MemPalace mirror matches on -- so the most operationally dangerous property of this change ("a gate edit goes live only on merge, having never run") was unreachable by anyone searching for it. Someone asking "how do I test a change to review-verdict.yml" would have found nothing. Adds three signal phrases and points the record's one-line reference at a section rather than at a ~1050-line file. No behaviour change; frontmatter and prose only. Verified the frontmatter still parses under PyYAML rather than the validator's hand parser, per #674 -- an apostrophe in a single-quoted scalar is exactly what that hand parser cannot see. Refs: #672 Decisions-Edit: yes	2026-07-28 23:17:14 +02:00
timothy	20b7171fba	fix(672): review round 2 -- correct a stale rule: field, document the self-test gap ... Re-review of the fix commits returned MERGEABLE (nine trigger mutations all caught, every prior finding verified against independent sources) with four low-severity findings. All four are addressed here. F1: `release.verdict-status-check`'s `rule:` frontmatter still said "A `pull_request` workflow auto-passes the two exempt classes". Round 1 past-tensed that record's BODY and left its `rule:` stale -- which is the exact failure mode the previous commit cites as the reason to put limitations in `rule:` in the first place. The catalog row mirrors this field verbatim and it mirrors again per-`key:` into MemPalace, so a stale `rule:` propagates further than a stale paragraph. F2: same record, "is what makes the rollout self-hosting" -> past tense. It described #630 and now reads as a live property. F3, the one that matters operationally: base resolution cuts BOTH ways. A change to `review-verdict.yml` is no longer exercised by its own PR -- the PR runs the version already on `main` -- so an edit goes live only ON MERGE, repo-wide, having never run. A broken edit merges green and then breaks the gate for every subsequent PR, and the PR that would repair it is gated by the same broken workflow. The recipe for verifying one safely (scratch base + throwaway PR + probe-named context) now lives in docs/ci-cd.md, which is where an operator looks, rather than in the record. F4: the sibling-workflow guard globbed `*.yml`, so a workflow added as `.yaml` would be silently unscanned. Latent today, which is when it is cheap. The record lost its meta-justification paragraph to the 60-line prose ceiling. Fifth trim this session; the operational recipe moving to ci-cd.md is better placement anyway, but it was forced rather than chosen. ersatztv#688. Refs: #672 Decisions-Edit: yes	2026-07-28 23:03:15 +02:00
timothy	b2a5c72bfe	docs(672): widen the residual to the real inventory (#697, #698) ... Cross-family review established the residual is materially larger than the previous commit said, and that saying "docker-build.yml / ETV_STATUS_AUTH" understates it. Gitea injects `GITEA_TOKEN` into EVERY job automatically, defaulting to read/write. So the set of workflows that can POST `review-verdict/h10` is not a short list to audit -- it is all of them, plus `workflow_dispatch` (1.24+ loads the definition from the selected branch) and `push`-triggered ones. A collaborator's own write-scoped API token is a route with no workflow at all, because branch protection binds the required CONTEXT, not its issuer. Recorded in #697. The same review found three defects in the exemption path itself, none of them introduced here and none closed here: a retarget race that enumerates a docs-only diff against a scratch base while the enumerator revalidates only head.sha, a Renovate-PR hijack (the exemption reads the immutable PR creator, not who pushed the head), and an inherited `success` that short-circuits before any PR/base/author/file check. Filed together as #698, since they share one root cause -- the gate trusts state it cannot attribute. Also drops the claim that the three properties are "pinned by tests". The tests pin the workflow's SHAPE; no in-repository test can establish status-authority isolation, and the sibling-workflow guard added in the previous commit catches only a workflow that names the context in plain text. Trimmed to the 60-line prose ceiling for the third time in this session. That is ersatztv#688 -- the ceiling is now deleting rationale two reviewers asked for. Refs: #672 Refs: #697 Refs: #698 Decisions-Edit: yes	2026-07-28 22:41:48 +02:00
timothy	35a8ea8aef	fix(672): review round 1 -- pin the trigger set exactly, sweep the stale claims ... Cold review found the first cut of the test satisfiable by a still-vulnerable config, and two prose claims that outran the evidence. The test asserted "pull_request_target present, pull_request absent". Adding `workflow_dispatch:` or `push:` ALONGSIDE it kept that green, and both are ref-resolved with secrets, so either one restores an equivalent self-supplied-definition path. Enumerating those two would have the same hole one trigger later, so the assertion now pins the whole set: exactly {pull_request_target}, nothing else. Verified by mutation -- adding `workflow_dispatch` now reds. Adds the guard that would have caught the residual below rather than only the instance: no workflow OTHER than review-verdict.yml may reference `review-verdict/h10` in executable lines. Scoped honestly in its docstring as a drift guard, not a security boundary -- a workflow can still write the status through an indirection a text scan cannot see. `release.verdict-status-check` item 4 still asserted, in the present tense, that a PR editing review-verdict.yml is judged by its own edited copy. That is now false for this workflow, and it is the record a reader resolving the gate from the catalog actually lands on. Past-tensed, with the surviving residual named. The probe count said three. There were four; the omitted one is the only one with a negative result, which is what turns an honest partial into an overclaim. Corrected in both the record and ci-cd.md, along with what was NOT measured (`reopened`/`ready_for_review` firing under the new trigger). Also records two operational consequences a maintainer will otherwise hit cold: retargeting a PR onto `main` leaves it statusless until its next push (`edited` is not in `types:`), and the required contexts carry a literal `(pull_request)` suffix, so repeating this fix on docker-build.yml would rename them and deadlock merges unless branch protection is edited in the same operation. Trimmed the record back under the 60-line prose ceiling -- for the second time this session, which is ersatztv#688 reproducing, not a defect here. Refs: #672 Refs: #697 Decisions-Edit: yes	2026-07-28 21:49:22 +02:00
timothy	8b73234d78	docs(672): record that the fix closes the route, not the class (#697) ... Probing rather than reasoning turned up a second instance of the same vulnerability class while this fix was in review. `docker-build.yml` also triggers on `pull_request`, so it is head-resolved too, and it carries `ETV_STATUS_AUTH` (`REGISTRY_USER:REGISTRY_PASSWORD`) for the #420 revalidation read. Basic auth is not scoped: an account that can read commit statuses can write them. Confirmed with a scratch PR that POSTed a probe-named context using those credentials and succeeded — so a PR rewriting `docker-build.yml` can still post `review-verdict/h10=success` for its own head. That workflow cannot take the same fix. It builds and tests the PR's code, so it must resolve from the head; `pull_request_target` there would be the real footgun. It needs a read-only status identity instead. Filed as #697. The `rule:` field carries the limitation, not just the body, because the predecessor record's documented failure was exactly a reassuring sentence in the position a catalog reader stops at. Also trims the record to the 60-line prose ceiling. Adding it at 62 lines pushed p90 past the ceiling and reddened the blocking `script-tests` job — which is ersatztv#688 reproducing live, not a defect in this change. Refs: #672 Refs: #697 Decisions-Edit: yes	2026-07-28 21:41:26 +02:00
timothy	cf907f0988	fix(672): trigger the verdict gate on pull_request_target scoped to main ... `review-verdict.yml` writes `review-verdict/h10`, the status branch protection requires on `main`. It triggered on `pull_request`, and Gitea resolves a `pull_request` workflow DEFINITION from the PR's own head commit — so a PR editing this file ran its own rewritten copy and could post `h10=success` for itself. `PROTECTED` did not help (the rewrite defines it too), and neither did branch protection, which requires the context rather than an author and carries `required_approvals: 0`. The #649 base-ref checkout binds the SCRIPTS this job runs to the base. It cannot bind the job definition. Only the trigger can. Measured on this instance (Gitea 1.25.4) with three throwaway scratch PRs rather than inferred from GitHub, since the premise is a claim about someone else's software: A. A PR rewriting the workflow posted its own probe context and the real `review-verdict/h10` was never written — the base definition never ran. B. Under `pull_request_target` the identical rewrite was ignored: the BASE definition ran and posted `h10=pending`, on `opened` and `synchronize` alike, with `secrets` still available. C. With `branches: [main]`, a PR into a non-main base produced no run and no status at all. The probes only ever posted probe-named contexts, never a forged `h10`. `branches: [main]` is half the fix, not a refinement: base resolution means the BASE branch supplies the definition, so without it the rewrite simply moves to an attacker-pushed base — and a status forged there is inherited by any later real PR with the same head sha (#663). `pull_request_target` is safe here only because this job never checks out or executes head-supplied code; the base-ref checkout is what makes the trigger usable, so the two are one decision. Rejected `required_approvals: 1` as the cheaper fix: Gitea forbids approving your own PR and this is effectively a single-maintainer repo, so it would deadlock every PR rather than gate the dangerous ones. Three mutations confirm the new test discriminates rather than merely passing: reverting to `pull_request`, dropping the `branches` filter, and re-adding `pull_request` alongside the safe trigger each go red with a distinct message. It parses the YAML instead of substring-matching because `pull_request` is a prefix of `pull_request_target`. Refs: #672 Decisions-Edit: yes	2026-07-28 21:28:38 +02:00
timothyandClaude Opus 5	2249a806c9	fix(671): review round 4 -- fix the chapter-title entity interpolation ... Final cold review of 34eee753b: the commit's own changes were confirmed correct, but it flagged a real pre-existing bug in the exact block I had just edited, and I was adding the first-ever tests for that method without covering it. `Playouts/Mapper.GetDisplayTitle`'s Song arm interpolated `{s}` -- the `case Song s` ENTITY -- into its chapter-title branch instead of `{t}`, the composed title. Song has no ToString() override, so a chaptered song rendered as the literal "ErsatzTV.Core.Domain.Song (Chapter 3)" in the playout guide, troubleshooting, media-item info and channel states. The sibling MusicVideo and OtherVideo arms are correct only because they happen to name their lambda `s`. Pre-existing on main; fixed here because it is one token inside the block this branch already touches. Two tests pin it; reverting renders the type name. Also: completed the guard on that arm (`Optional(s.SongMetadata).Flatten()`, the other half of the sibling pattern I claimed to have copied), added the new mechanism to the record's `mechanics:`, added the symptom tokens a future session would actually search for (ArgumentNullException, Artists, primitive collection, chaptered song) to `signals:`, restored the remedy sentence an earlier trim dropped, and trimmed to 59 prose lines for margin under the 60-line ceiling. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-28 20:27:08 +02:00
timothyandClaude Opus 5	34eee753b2	fix(671): review round 3 -- sweep Artists by FIELD, correct the archaeology ... Cold review of 572737a29. Findings taken; two are corrections to my own claims. CORRECTION: "a regression this branch INTRODUCED" was wrong, and I verified the reviewer's counter-claim against origin/main before accepting it. That handler already included SongMetadata AND already routed Song there, so GET /api/v1/playlists/{id}/items was ALREADY a live 500 for a null-Artists song. This branch only made the same throw reachable on a second path. The record said so twice; both are fixed, because a wrong explanation outlives a wrong line. SWEEP: fixing one site left the mirror standing -- Playouts/Mapper.GetDisplayTitle had the identical unguarded join on a path that also eager-loads SongMetadata, so it too was live, feeding the playout guide, troubleshooting, media-item info and channel states. Guarded, with a unit test; reverting it reproduces ArgumentNullException. LibraryBrowseItemMapper already wrote `Artists ?? []`, so the nullability was known in-tree and these sites were simply unswept. Filed #691 for the remaining SongVideoGenerator dereferences on the playback path. Also: documented that the shared matrix is the RERUN predicate used as a superset for playlists (the playlist write path rejects RemoteStream today); noted the third, inert consumer ReplacePlaylistItemsHandler; added the new mechanisms to the record's `mechanics:` field; and trimmed the record under the 60-line prose ceiling -- it was tipping the corpus p90 above the ceiling and reddening the calibration test in scripts/tests, which passes again. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-28 20:09:07 +02:00
timothyandClaude Opus 5	572737a29e	fix(671): review round 2 -- guard Song.Artists, cover the second consumer ... Cold independent review of 017ef988d. Three findings taken, one filed out. The important one is a regression this branch INTRODUCED. `SongMetadata.Artists` is a nullable EF primitive collection (JSON in one column, not a navigation) that FallbackMetadataProvider leaves unassigned when a song's tags fail to read, and `string.Join` throws ArgumentNullException on a null sequence. The rerun list previously did not load SongMetadata at all, so the throw was unreachable there; adding the include promoted it to a live 500 that would have failed the whole page. Confirmed by reverting the guard: ArgumentNullException, parameter 'values'. The file header claiming every member was guarded was false. The empty case is filtered too, so an artist-less song loses its bare " - ". Second: `GetPlaylistItemsHandler` had no handler-level test at all (its controller tests stub the mediator), so the RemoteStream include added last round was discharged by inspection -- the same method that produced #671. It now runs the same 13-type matrix via a shared SelectionSeedData; removing the include fails that matrix. Third: dropped the dead `(i as Season).SeasonMetadata` include leg -- the Season projection reads Show.ShowMetadata and the scalar SeasonNumber, never SeasonMetadata. Filed #690 for the pre-existing, out-of-scope finding: the paged TotalCount ignores the search query, so the SPA renders empty pages. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-28 19:33:52 +02:00
timothyandClaude Opus 5	017ef988d0	fix(671): review round 1 -- pin exact names, complete the playlist include ... Cross-family (Codex) adversarial review of 8523088ce. All three findings taken: - The name assertion only checked "not a placeholder", so it could not see a missing NESTED include leg: dropping Episode -> Season -> Show still renders "s00e04 - Selected episode", which contains no placeholder marker and passed. Now every type pins its whole expected string; re-removing that leg fails, as verified before restoring it. - Widening the shared switch with a RemoteStream arm put `GetPlaylistItemsHandler` one include short -- it loaded metadata for the other nine types, so playlist RemoteStream names alone would have degraded to "???". - `?? 0` rendered an unloaded Season as "s00", which conventionally means Specials and so fabricated plausible-looking real data; it now renders "s??". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-28 19:00:08 +02:00
timothyandClaude Opus 5	8523088ceb	fix(671): resolve rerun-collection selections through one shared include chain ... The paged list handler eager-loaded nothing, so `ProjectToViewModel` read four unloaded navigations and every row of every collection type projected a null selection. Because the selected id and the display name are read off the SAME navigation, this dropped the id too -- the harm is not an unlabelled badge but an editor that round-trips a null and clears the user's stored selection. The by-id handler loaded metadata for only four of the ten selectable media types: Song/OtherVideo/Image/RemoteStream returned a null-ish selection and Episode/MusicVideo threw an NRE that surfaced as a 500. Fixed at the boundary rather than per call site: - `RerunCollectionQueryExtensions.IncludeSelectionDetails()` is now the single include chain, called by both handlers, joining the existing `ProgramScheduleItemQueryExtensions.IncludeScheduleItemDetails()` precedent (#229). Artwork legs are deliberately omitted -- this projection reads only ids and titles. - The media-item switch was duplicated verbatim for RerunCollection and PlaylistItem; both now call one `ProjectMediaItemToViewModel`, which handles `RemoteStream` (via a new `ProjectToNamedViewModel`, since the existing `ProjectToViewModel(RemoteStream)` returns an unrelated type) and never falls through to null -- an unknown subtype keeps its id and takes a conspicuous name, because throwing would fail a whole paged GET over one bad row. - Every metadata navigation in `MediaItems.Mapper` is now read through `Optional(...).Flatten()`, so an un-included nav degrades to "???" instead of being a latent 500 for whichever caller loads least. Tests enumerate all 13 supported CollectionTypes for both handlers, with the matrix derived from `IsSupportedSelectionType` so a newly-supported type joins it automatically, plus a completeness guard on the set. Each mechanism was removed in turn and confirmed red first. fixes #671 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-28 18:48:24 +02:00
timothy	f2d9c0dc8e	fix(668): review round 5 -- three prose nits, including an off-by-one I filed ... Final sweep confirmed the retracted MySQL over-match claim survives in no file on the branch (only in two immutable commit messages, which stay -- rewriting history would invalidate every sha-bound review verdict). Three nits remained. - The fixture's class docstring said the on-MySQL claim "rests on the server's collation", which is the one thing the decision record says it does NOT rest on. It rests on Unicode-aware LOWER(); the executed comparison bypasses the collation entirely. Reworded. - The record's `rule:` enumerated the covered fields but omitted show_genre, which GetSource and the fold both handle ("genre" or "show_genre"). Added. - My own #688 write-up was wrong twice: the 60-line ceiling warning is NON-blocking by design, and the calibration pytest reds at >=61, not >=60 -- main's p90 is 59, so a 60-line record makes p90 == ceiling and PASSES. The bullet even contradicted itself, since the next sentence relies on 60 passing. Corrected in the PR body and in the issue. Decisions-Edit: yes	2026-07-27 22:21:55 +02:00
timothy	07723e418b	fix(668): review round 4 -- sweep the retracted claim by SUBJECT, not by memory ... Round 3 said "correct the claim everywhere" and missed two places, which is the same mistake in a new coat: I fixed the spots I remembered instead of grepping for the subject. - The class-level summary of the very file round 3 edited still said MySQL's "ci collation OVER-matches instead", contradicting the method docstring forty lines below it. Reworded. - The PR body still carried the retracted over-match story -- and round 3's commit message claimed it had been corrected. It had not. Now corrected, with the measurement table and the retraction stated openly. This time the sweep was `grep -i over-match` across every file the branch touches; the remaining hits are the SQLite-fold invariant and #578 history, which are correct and stay. Also softened two overclaims the reviewer flagged. Round 3 deleted the predecessor's "configuration-incidental, not designed" hedge and replaced it with a firmer statement than the evidence supports: that MySQL cannot over-match is contingent on MySqlConnector fixing the connection collation to utf8mb4_bin, not a property of MySQL. A driver, protocol or prepared-statement change could restore it. The record and the fixture docstring now say "driver-contingent, not a law"; the code is safe either way because the ordinal filter stays regardless. Filed #689 for the source of the copied falsehood -- LibraryFolderDedupeMigrationTests on main carries the same "CI sets ETV_REQUIRE_MYSQL_TESTS=1" sentence. Decisions-Edit: yes	2026-07-27 22:09:26 +02:00
timothy	dda98efcc4	fix(668): review round 3 -- MySQL does NOT over-match; correct the claim everywhere ... Review BLOCKED on a false CI claim I copied from the sibling fixture ("CI sets ETV_REQUIRE_MYSQL_TESTS=1"). Nothing sets it; the doc now says plainly that CI does not arm this lane and points at ersatztv#627. That was the blocker. Chasing the reviewer's second finding then overturned something bigger. It predicted that seeding an unaccented "Edith" would make the in-memory ordinal filter load-bearing on MySQL, since utf8mb4_0900_ai_ci treats é as e. Mutation test says otherwise: with the filter deleted the MySQL test stays GREEN. Measured against a live 8.4 to find out why: LOWER(Name) LIKE 'é%' (literal) -> Édith AND Edith LOWER(Name) LIKE @v (ai_ci variable) -> Édith AND Edith LOWER(Name) LIKE @v COLLATE _bin -> Édith only the EF query, executed -> Édith only The driver binds the pattern with a BINARY collation, so the executed comparison is accent-SENSITIVE and MySQL does not over-match at all. MySQL's correctness rests on its Unicode-aware LOWER(), not on the collation. My earlier probe used a LITERAL pattern -- a different query from the one the code runs -- and I wrote its result into the handler comment, the decision record and the PR body. All three now say what actually happens, and the record carries the lesson: measure the query the CODE runs, not one you type. The "Edith" row stays as a near-miss control, with a docstring that says what it does and does not prove rather than the over-match story it was added for. Also moved EnsureCreatedAsync out of [SetUp]: NUnit skips [TearDown] when [SetUp] throws, so a mid-create failure would strand the database. Decisions-Edit: yes	2026-07-27 21:52:16 +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
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	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	cb7da865b6	Merge pull request 'docs: permit subagents explicitly, and make claiming an issue a check rather than a label' (#682) from docs/claim-protocol-and-subagents into main	2026-07-27 16:04:13 +00:00
timothy	1d76a088c6	Merge pull request 'feat(578): artist typeahead covers music-video and song credits; album_artist stops 404ing' (#676) from feat/578-artist-typeahead-source into main	2026-07-27 06:09:30 +00:00
timothy	b83e965994	docs: make subagent use explicit, and turn "claim an issue" into a check rather than a label ... Two rules that were implied but not enforceable, both demonstrated by ersatztv#649 being implemented TWICE in parallel to completion on the same day. Subagents. The kickoff's HARD CONSTRAINTS already require parallel disjoint slices, mandatory independent review from a cold brief, and a named model/effort per dispatch — none of which is satisfiable without delegation. But nothing said so outright, and a generic client preamble of the form "do not use the Agent tool unless the user requested it" reads as a prohibition. Now stated plainly in both CLAUDE.md (always loaded) and the kickoff (pasted per session), with what to delegate and what to keep inline. Claiming. `in-progress` prevents duplicate PICKUP, not duplicate WORK — the record already said so, but step 3 told you to apply the label and nothing else. It now requires four checks first: an open PR whose body says `fixes #N`, a remote branch naming the number, a claiming comment predating the label (exactly what select-queue.sh's CLAIM? flag raises and deliberately leaves unresolved), and a fresh git fetch. Each fails differently; all four are cheap. Staleness. The second half of the #649 collision was reading origin/main once, at branch time, and not again across hours and four review rounds. A branch on a stale base computes its diff against that base, so `git diff origin/main` shows other sessions' merged work as DELETIONS and pushing it reverts them. Re-fetch before every push, rebase when it moved. process.parallel-session-claim carries the incident, including what worked: the merged implementation was better in one respect and the discarded branch's test coverage was salvageable, so diff the two before discarding yours.	2026-07-27 07:44:25 +02:00
timothyandClaude Opus 5	27867e03cf	fix(651): make the stated invariant true on Playlists; pin the predicate's endpoints ... The review's headline finding was in my prose, not my code: spa-conventions and the round-8 commit both claimed an unbindable id "surfaces as 'no selection' with Save disabled" and that regressions "assert zero writes are reachable". True on RerunCollections and FillerPresets. False on PlaylistsScreen in all three respects — `draftFromItem` nulled the id but KEPT `selectedName`, so the row read "Cool Movie" over a null draft; Save had no selection check; and clicking it did issue the PUT with `mediaItemId: null`. Only the server's `ReplacePlaylistItemsHandler` 422 stood there, and the DB would have persisted it (`PlaylistItemConfiguration` marks all four FKs `IsRequired(false)`). Rather than weaken the claim, made it true: a dropped id now clears its label, and Save is gated on every item having a selection, with a visible count as the reason. `playlistGroupId` was the same class on the same screen — seeded from the wire into `AddPlaylistDialog`, re-parsed with a bare `Number()`, and POSTed as an entity reference — so "every path by which an id from the wire becomes editor state" was not literally true. Now filtered from the group options and normalized on submit. Added `selectionId.test.ts`. The predicate had become the single point of failure for eleven call sites across three screens while being exercised only indirectly; nothing pinned the inclusive endpoints, so a `>` for `>=` slip passed the entire suite. Verified by mutating each comparison. Also documented why `0` and negatives are accepted — the contract is bindability, not existence — because every other id check in this repo uses `id > 0` and the next reader would otherwise "fix" the inconsistency. Two of my assertions were vacuous, the eighth of that shape on this branch: one clicked a button it had just asserted disabled (a restatement of `toBeDisabled()`), and one asserted a POST count on a path that never attempted a save. The first is deleted; the second now actually attempts the write, which makes it fail against the unguarded parent. Corrected claim: all five round-8 regressions do fail against their parent, but on their load-bearing assertions (`getByText('A selection is required')`, `queryByText('Bogus Collection')`) — not on the write-count ones, which were passengers. Follow-up filed as #677 (ScheduleItemInspector's unguarded ingresses; list-backed pickers dropping malformed options silently). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-27 04:25:14 +02:00
timothyandClaude Opus 5	39c4e8df0a	fix(651): review round 8 — put the selection-id predicate at the boundary, not the site ... Round 7 added the int32 check inside `isSearchPickerOption` — the place the defect was found — which left every other door into editor state open. A malformed successful response carrying `1.5` or `2147483648` still entered `draft` through list-backed options and through the selection restored from the detail read, keeping Save enabled and sending a value the API cannot bind, while the identical value arriving via SearchPicker was correctly rejected. The predicate now lives once, in `web/src/api/selectionId.ts`, and sits on every path by which an id from the wire becomes editor state. The class crosses all three screens, not just the one the finding named, so all three are covered: - RerunCollectionsScreen: `toPickerOptions` (3 list branches) + `draftFromRerun` - PlaylistsScreen: `toPickerOptions` (3 list branches) + `draftFromItem` (4 id fields) - FillerPresetsScreen: `draftFromPreset` (5 id fields) + the collection-family browse options - pickers.tsx: `isSearchPickerOption` now delegates rather than carrying its own copy An unbindable id is treated as ABSENT, never coerced — rounding 1.5 to 1 would submit a DIFFERENT record — so it surfaces as "no selection" with Save disabled and a visible reason; an option that cannot be selected safely is dropped rather than rendered. Five regressions assert zero writes are reachable via each previously-unguarded path. Also corrects two of my own test descriptions, per the review: the padded-ETag test is a regression guard rather than a round-7 defect demonstration (Headers strips outer whitespace before the app sees it), and the late-settlement test guards the abort/race COMPOSITION — what it actually fails is an abort-only implementation whose fetch ignores its signal, which is why its stub ignores `init.signal`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-27 04:25:14 +02:00
timothyandClaude Opus 5	e605e4006a	fix(651): review round 7 — treat "no usable token" as one class, not three values ... HIGH: the fail-closed gate rejected `null` but not the adjacent values. `Headers.get('ETag')` returns `''` for an empty or whitespace-only header, which PASSED the gate and produced an editable draft; `updateRerunCollection`'s `ifMatch ? … : undefined` then dropped the empty string as falsy and sent no `If-Match`, silently overwriting a collaborator — the exact class the gate exists to make unreachable, reached through the value next door. Absent, empty and whitespace are now one case ("no usable concurrency token"), normalized by a single `usableEtag` helper that returns the TRIMMED token or null, so `etagRef` can only ever hold something that will actually be sent. Tested across four blank shapes asserting zero PUTs are reachable, plus a padded ETag that must be trimmed and USED rather than dropped. MEDIUM: the deadline abandoned the wait without cancelling the work, so each Retry stacked another live connection. It now aborts via an AbortSignal (threaded through `getRerunCollectionWithMeta`) AND clears its timer on settlement and unmount. Both halves are kept deliberately: aborting cancels the work, while the rejected race stops the UI waiting even if the abort never propagates — cancellation and giving-up are not the same guarantee, which the late-settlement test proves by using a stub that ignores its signal. MEDIUM: `Number.isFinite` accepted ids the API cannot bind — `1.5` and values outside int32 rendered, committed through `onSelect`, and would fail server-side on `selectedId`. Validated as an int32 integer. MEDIUM: a malformed or failed page was reported as "No matches", telling the user the library is empty when the request actually failed and giving no hint that reopening retries. Failures now surface as a distinct alert. MEDIUM: `spa-conventions.md` still mandated the deleted "never let a refresh clear an id it failed to name" guard and said "the client guard stays" — contradicting the initialize-once bullet 20 lines below it. Rewritten to state that the guard is gone and must not be rebuilt, with the reason (it only ever preserved a list-seeded value that is null in production). Grepping the DELETED TERMS across all docs — the lesson from round 6's stale `rule:` — also caught two stale `signals:` tokens on the record that the rule fix had missed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-27 04:25:14 +02:00
timothyandClaude Opus 5	a973fc48e2	fix(651): review round 6 — fail closed on a missing ETag; validate elements, not containers ... HIGH: "the draft is only created alongside the ETag" was not the invariant I claimed. The response can OMIT the header, in which case the draft was still created and the subsequent PUT carried no `If-Match` — the round-4 force-write hole in a new form. The editor now fails closed: no concurrency token, no editable draft (error + Retry/Back). Six tests were leaning on ETag-less detail mocks, which is exactly how this stayed invisible; every single-record GET mock now goes through a `detailResponse` helper that always sets one, and the absent case is tested explicitly — asserting zero PUTs are even reachable. MEDIUM: the detail GET had neither deadline nor recovery affordance, so a never-settling request left a bare spinner with no way out. It is now raced against a 15s deadline, the loading view carries a Back control, and the error view offers Retry. MEDIUM: the malformed-body guard checked the container, not the elements. `[null]` passes `Array.isArray`, reaches `setResults`, and throws on `option.id` during render; a wrong-typed `id` would commit an invalid value through `onSelect`. Each element is now validated, and a malformed payload is treated as a failed attempt so it stays retryable rather than cached. MEDIUM: the decision record's `rule:` — the authoritative string, copied verbatim into the catalog that is the documented entry point — still mandated the machinery round 5 deleted: touched-field hydration, `replaceDraft`, conflict reconciliation. Anyone following it would have rebuilt the rejected design. Rewritten to the initialize-once policy and the catalog regenerated; historical prose no longer says `replaceDraft` "is now" separate. MEDIUM: the replacement Reload test resolved its second GET immediately and returned a non-null selection, so it observed neither a pending reload nor the dirty-selection discard — removing `setDraft(null)` could leave it green. It now holds the reload open, asserts the form is ABSENT while pending, and returns `selectedId: null` to pin the case round 3 showed could resurrect a dirty id over a collaborator's change. Checklist item taken from this: when a mechanism is deleted, the decision record's `rule:` is the single most likely thing left stale, and the one string that propagates. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-27 04:25:14 +02:00
timothyandClaude Opus 5	78cd9e0ebf	fix(651): review round 5 — delete the draft-reconciliation layer instead of extending it ... Took the coordinator's proposal. Rounds 2-4 built and rebuilt a layer that merged a late detail response into a draft the user was already editing; it produced a HIGH finding in three consecutive rounds, three of them cross-user lost updates. Round 5's finding was unfixable in kind: `identityConflicts` had no immutable baseline, so it could not tell "the user changed this" from "the server changed this" — giving both a missed conflict (same type, different id: v2 ETag installed over the user's id, third lost update) and a false one (local type switch: dialog opens spuriously, and "Keep editing" leaves `etagRef` null so the next PUT is a silent FORCE-WRITE). The race is removed rather than refereed. `RerunCollectionEditor` now initializes its draft EXACTLY ONCE from the detail GET and withholds the form until it lands; conflicts are detected at save time by the `If-Match` -> 412 -> Reload path that already existed. Deleted: `touchedRef`, `hydrateDraft`, `hydrateSelection`, `hydrateIdentity`, `identityOf`, `identityConflicts`, `replaceDraft`, `replacePending` and the hydrate/replace load mode. Reload simply sets the draft back to null and re-runs the same load, so the form is unmounted while the replacement is in flight — round 4's MEDIUM-4 becomes structural. Two facts make this lossless rather than a regression. The list row could never have helped: `GetPagedRerunCollectionsHandler` applies ZERO `.Include()`s where `GetRerunCollectionByIdHandler` applies fourteen, and both project through the same mapper, so the list response is a strict SUBSET of the detail one — the id round 1 preserved from it is null in production for every row (#671), and existed only in test fixtures. And FillerPresetsScreen/PlaylistsScreen already worked this way; RerunCollections was the outlier, which is why nearly every finding in rounds 3-5 traced to it. The ETag is now written in the same callback that sets the draft, so `draft != null` implies an ETag and a PUT without `If-Match` is unreachable by construction. MEDIUM-2: a failed search retried every debounce forever — a fresh `{ok:false}` re-ran the effect and the success guard declined it. `ok` (is the held answer authoritative) is now separate from `attemptRef` (have we already tried this exact source+query); only an explicit reopen/focus/edit re-arms a retry. MEDIUM-3: a malformed 2xx body resolves as `undefined` (client.ts swallows the SyntaxError), and `setResults(undefined)` threw on the next render. A non-array is now treated as a failed attempt, and `search` is raced against a 10s deadline since a caller-supplied promise carries no abort signal. Two tests were hiding findings and are fixed: the failed-search test pressed Escape before the unintended retry could fire, and the round-4 conflict test claimed to select id 9 while firing the already-selected id 5. Fourteen tests of the deleted machinery are gone with it, replaced by six asserting the new invariants. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-27 04:25:14 +02:00
timothyandClaude Opus 5	f601d957a6	fix(651): review round 4 — an id never travels without its namespace ... Round 4's through-line: rounds 2-3 made HYDRATION treat {collectionType, selectedId, selectedName} as one value, but the fix was applied to the structure that was named rather than to every structure that carries an id. Three more instances of the same class, plus two independent findings. HIGH-1 + HIGH-2 (one change, per the structural directive): every result, option and cached result set now carries its SOURCE, and identity is compared as (type, id). - `SearchPicker` takes a REQUIRED `source` prop and caches results against (source, query), not query text. Keying on text let the round-3 re-query guard SUPPRESS the new source's request and leave the previous namespace's hit clickable under the new label — a Collection id stored as a SmartCollection id. Results from another source are now hidden outright rather than dimmed: they are not stale, they are wrong. Required rather than defaulted, because a default would silently opt every caller out. - `pickerFor` tags list-backed options with the type they were loaded for, on both RerunCollectionsScreen and PlaylistsScreen, so the previous type's rows stop being selectable the instant the active type changes rather than lingering through the replacement load. HIGH-3: a touched identity contradicting the server's type is a CONFLICT. Pinning the user's edit was right; adopting the response's newest ETag alongside it authorized a Save that silently overwrote the collaborator's type change with no 412. The conflict dialog is raised and the stale ETag kept, so even a forced Save 412s. With round 3's Reload defect this is the second cross-user lost update, so it is recorded as a category: never install a save-authorizing ETag over a local edit the server contradicts. MEDIUM-4: the editor is inert while a Reload is pending. The dialog closes immediately, so an edit typed before the replacement landed was silently erased along with the touched set that protected it. MEDIUM-5: cached search provenance records `ok`, so a transient 500 is retried instead of being cached as an authoritative "No matches" that reopening can never clear. Also: `npx tsc --noEmit` typechecks NOTHING in web/ — the root tsconfig is solution-style ("files": [] + references), so it resolves to zero inputs and exits 0. The real gate, and what CI runs, is `npm run typecheck` (tsc -b). Verified by planting a deliberate type error: --noEmit stayed green, -b caught it. Running the real gate surfaced four genuine errors in tests written earlier this branch (a missing required prop and three `never has no call signatures` from closure-assigned mock variables), fixed with the repo's existing holder-object pattern rather than casts. Recorded in spa-conventions so the next session doesn't repeat it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-27 04:25:14 +02:00
timothyandClaude Opus 5	5b0ba08aab	fix(651): review round 3 — cover the class, not the named instance ... Round 3 found four defects that are all one mistake: each earlier fix enumerated one instance of a class instead of covering the class. Fixed by asking what else is in each class rather than patching the named case. CRITICAL — cross-user lost update. Conflict "Reload" ran through the refresh policy with a touched-set reset. Because a reloaded RemoteStream reports `selectedId: null` under the #671 server gap, the keep-ours-when-the-id-is-missing fallback restored the user's DIRTY selection, the fresh ETag was installed, and the next Save silently overwrote the collaborator's change — with the very edits the user had explicitly asked to discard. "Keep ours when the server omits the id" is a REFRESH policy; a reload is a REPLACE. `replaceDraft` is now a separate function and the mode travels with the load, so the two cannot be confused at the call site. HIGH — `collectionType` and the selection still hydrated apart. `collectionType` says which table an id indexes, so it is part of the same indivisible value as the id and its name; splitting it is the identical bug to splitting id from name. A record retyped server-side mid-load hydrated the new type while retaining the old id, displaying and saving a Collection id as a RemoteStream id. All three fields now resolve as one `Identity` unit: either half touched pins the whole thing, a differing type takes the response's unit whole (null selection included), and only once both sides agree on the type does the id/name rule apply. HIGH — stale results were still committable by pointer. Enter was gated and `onClick` was not: the same defect in another modality. The guard moved into the single `choose()` sink so every commit path is covered, including any added later, and a superseded list is now genuinely inert (`aria-disabled` + dimmed) rather than looking normal and silently no-opping. MEDIUM — reopening after Escape re-queried an already-current result set; the duplicate response reset the cursor the user had since moved, leaving Enter doing nothing. The effect now skips the search when the cached results match the trimmed query, and reopening places the cursor per the ARIA APG instead of swallowing the keypress. Also corrects an overstated justification in FillerPresetsScreen: the render-time id check was dropped because there is no reachable path TODAY, not because "every writer sets both" — the initial load writes the id alone, and a stale resolver can repopulate the label after a clear. Same enumeration error as the findings above; the comment now says what is actually true and what to do if a path appears. Note: an apostrophe I introduced inside the single-quoted `rule:` scalar broke PyYAML while `decisions_validate.py` (hand parser) stayed green — the same trap class as the unquoted `#`, caught only by scripts/tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-27 04:25:14 +02:00
timothyandClaude Opus 5	ba52219a9a	fix(651): review round 2 — hydrate untouched fields, never merge a selection field-by-field ... Re-review of a04d9f0b came back BLOCKED: the F1 merge fixed a visible data loss and introduced a silent one. Four blockers, all web-only. BLOCKER 1: `selectedId` and `selectedName` were coalesced independently, but they are one value. Against a Song response (id resolves, name does not), a user selecting a different song while the detail GET was in flight got the NEW name paired with the STORED id — chip read "New Song", Save wrote 42, no error and no visual cue. Strictly worse than the original defect, which at least cleared the field visibly. The same merge resurrected a deliberate clear and reverted a type switch. Replaced with two rules. The id/name pair resolves atomically (no id in the response -> keep ours whole; same id -> its name may fill ours in; different id -> take theirs whole). And hydration loses every race against the user: a `touchedRef`, fed by a single `edit()` funnel so "touched" cannot drift from "changed", limits the refresh to fields the user has not edited; an explicit conflict-reload clears it, since discarding local edits is its entire purpose. Three interleaving tests hold the detail response open, act as the user, then release it. BLOCKER 2: Enter could commit a result from the previous query — highlight Alpha for "Al", retype "Be", press Enter before the debounce. The highlight now drops on input change rather than when the next response happens to arrive, and every keyboard action is gated on the results matching what is typed. The stale list stays visible (hiding it flickers on each keystroke), it just stops being actionable. BLOCKER 3: Escape closed the popup while focus stayed in the input, where `onFocus` can never re-arm it — the picker was dead until the user blurred and refocused. Typing and ArrowDown now both reopen it. BLOCKER 4: the LCG boundary test recomputed the divisor instead of exercising `lcg`, so all three tests passed with the old `/ 0xffffffff`. Since the recurrence is a bijection mod 2^32, the seed whose first step lands on 0xffffffff is solvable exactly (653637408); the tests now drive the real generator into that state. Also: the rerun #id-degrade and re-save tests did not await the refresh, so they were satisfiable from the initial draft; they now await it and re-read live textContent. The Episode/MusicVideo cases are relabelled as error-path guards — a 500 never reaches the hydrate. FillerPresets' render-time id comparison is dropped as unreachable (every writer sets label and id together, and the one async writer refuses a mismatched id); an unreachable guard is an untested one. #671 is referenced from spa-conventions §3b as the server-side root cause the client guard defends against. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-27 04:25:14 +02:00
timothyandClaude Opus 5	e7e425fa25	fix(651): review round 1 — never clear an unnamed id, complete the Lucene escaping, keyboard-operable picker ... Cold cross-family review of 57aefcdf. Six findings, all web-only. F1 (HIGH, data-loss shaped): RerunCollectionsController.ProjectToResponseModel derives BOTH selectedId and selectedName from the same eager-loaded navigation, and GetRerunCollectionByIdHandler loads media metadata only for Show/Season/Artist/Movie while MediaCollections/Mapper maps RemoteStream through `_ => null`. So opening a RemoteStream rerun collection returned HTTP 200 with a null selection and the edit-load refresh CLEARED a stored id, leaving Save permanently disabled. The refresh now merges instead of replacing, so no path can clear an id it merely failed to name; the label degrades to `#id`. Covered per affected type — RemoteStream, Episode, MusicVideo, Song, OtherVideo, Image — plus a re-save assertion. The read-model gaps themselves are server-side and are NOT touched here. F2: `&` and `|` were missing from the escaped set, so `Rock && Roll` compiled with the boolean operator live. Pre-existing in Auto-Tune's original helper, but propagated to three more pickers — and now fixed for Auto-Tune too, since the helper is shared. The test that claimed to cover "every Lucene special" carried its own hand-copied sample and could not see its own omissions; it is now driven per-character off an exported LIBRARY_PICKER_LUCENE_SPECIALS. F3: a slow edit-load name resolution could relabel a newer selection. The label is now keyed to the id it was resolved for AND refuses to overwrite a label naming a different id — keying the render alone stops the mislabelling but discards the correct new label. F4: searchLibraryPickerOptions clamps pageSize instead of merely defaulting it. A bound a caller can exceed is not a bound. F6: replacing a native <select> with an input+listbox dropped keyboard operability. Full ARIA combobox pattern added — role/aria-expanded/aria-controls/aria-autocomplete, Arrow/Home/End over aria-activedescendant, Enter to commit, Escape to dismiss, options as non-tab-stops, cursor reset on each new result set. F7: both is-mounted tests were unsound. React 19 no longer warns on setState-after-unmount and an unmounted tree renders nothing either way, so the DOM assertion could not fail; the hook re-arm test used rerender rather than an effect cleanup. Now: a hook-module mock proving SearchPicker actually reads the guard and sees false, and a StrictMode double-invoke for the re-arm. Both verified by removing the mechanism and watching them fail. Same for the LCG divisor, which now has a direct boundary test. F5 (FillerPresets collection-family names) is filed as #670, not fixed here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-27 04:25:14 +02:00
timothyandClaude Opus 5	fad6805b91	feat(651): media-library pickers resolve by search instead of a bounded window ... The three `getLibraryBrowseItems` pickers (RerunCollectionsScreen, PlaylistsScreen, FillerPresetsScreen) populated a native <select> from a 100-row window over media-library tables that can hold tens of thousands of rows. #644 made that truncation visible; it did not make the picker usable, and paging to completeness would have been worse than the bug (~200 serial requests, each more expensive than the last). They now resolve by SEARCH through the shared `SearchPicker` over a new `searchLibraryPickerOptions` helper: zero requests on mount or on a type switch, at most ONE bounded request (25 rows) per settled query, nothing below 2 characters. Typed text is compiled via the now-shared `titleContainsQuery` (`title:*<escaped>*`) rather than forwarded raw, since the index's default field does not match bare title words. The current selection renders from the owning record — `selectedName` for rerun collections and playlist items, and for filler presets (which store only an id) a single by-id detail read — so editing an existing record can never lose or fail to name its selection. Class A stays put: bounded-by-construction admin lists still page to completeness via `loadAllPages`, and the collection-family filler-preset types keep their bounded single page (their `query` is a SQL LIKE, which a compiled Lucene query would not match). No server-side cap is raised; this is a web-only change. Folded in from #578: the rule-builder facet typeahead arms on focus rather than on mount (an N-rule tree fired N unrequested lookups), both typeaheads pair their `seqRef` guard with a shared `useIsMountedRef`, and the roundtrip test's LCG divides by 2^32 so `pick()` can no longer index one past the end. Decision record `spa.list-completeness-vs-bounded-pickers` is archived as superseded by the new `spa.library-pickers-resolve-by-search`; spa-conventions §3b rewritten to match. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-27 04:25:14 +02:00
timothy	fc3ede09bc	docs(578): the retracted claim survived in 9 places, including the record title and rule ... Comment- and docs-only; verified no non-comment line changed in any .cs. I reported last round that I had "classified every surviving hit". That was false, and the false confidence is the expensive part: a confidently-stated "I checked everything" stops anyone else checking. The retracted wording survived in nine places, two of them the record's title and rule: — and the catalog copies rule: verbatim, so the generated entry point and the record disagreed semantically while docs/decisions.md said the correct thing. Root cause of the miss, because it will recur otherwise: I built the sweep term list from the DELETED MECHANISM's vocabulary (LIKE, superset, keyspace, anchor, over-match) and never added the RETRACTED CLAIM's own words. "no predicate", "bound on work", "index entries", "no gap" and "holds in memory" were never grepped. After a retraction the subject list has to include the words of the thing being retracted, not just the thing already deleted. Second, worse: my first attempt at this round's sweep printed nothing for every term and I nearly read that as "all clear". zsh does not word-split an unquoted $FILES, so grep received one giant non-existent path — and the `|| echo "(none)"` never fired because the pipeline's exit status was sed's. Same failure shape as the bug arc itself: a check reporting success while examining nothing. Re-run with a proper array plus a control term ("SongMetadata" -> 42 hits) so an empty result is distinguishable from a broken grep. Fixed all nine, replacing "no predicate" with the seekable-cursor-vs-residual distinction already written correctly elsewhere: - handler: the "real bound on work" claim, the short-page rationale - SearchFieldValuesQueryShapeTests: "ANY predicate" + "reads exactly n index entries", and added what the test can and cannot pin (a SQL string, not a plan / visibility work / payload I/O) - GetSearchFieldValuesHandlerTests: "no gap between what the engine looks at and what it hands back", and the current-behaviour comment - record title, rule:, attempt-5 table row; api-conventions - regenerated docs/decisions/README.md so catalog and record agree again Tenth item, the same overclaim one level down and it survived the first retraction: the row bound was said to cap what the process holds in memory. It does not — payload width is unrestricted and one JSON array can contain arbitrarily many strings, each of which may enter the in-memory distinct set. It caps logical rows returned/materialized and round-trip count, nothing about bytes. Added as a third struck-through bullet next to the other two retractions.	2026-07-27 03:10:36 +02:00
timothy	5f73cd4482	docs(578): scope the bound to LOGICAL ROWS — physical work is not bounded, and I claimed it was ... Comment- and docs-only. Verified: the diff for both .cs files contains no non-comment line. HIGH (claim). "LIMIT n reads exactly n index entries and n rows" is false, and the bounded-physical- work reading it implied is false with it. Two mechanisms, both retracted explicitly in the record rather than quietly reworded: - MySQL purge lag. Deleted clustered-index records survive until purge runs and a range scan still traverses them, so returning 2,000 VISIBLE rows can touch far more index records. Deletion history therefore STILL affects physical work — precisely what attempt 4's failure was supposed to have made irrelevant. Attempt 5 removes the LOGICAL dependence on Id distribution; it does not make physical work independent of deletion history. - Unbounded row width. Artists/AlbumArtists are unrestricted TEXT/longtext and both SQLite and InnoDB spill to overflow pages, so a row count implies neither a byte count nor a page-read count. The guarantee is now stated as exactly what it is: at most 20,000 LOGICAL rows returned/materialized, and at most 10 round trips (11 for artist). The 392 KiB measurement is labelled one data point on one library whose credits average ~20 B/row, with an instruction to re-measure rather than extrapolate for long credit lists or MySQL over a network. Also noted: the query-shape test pins the SQL STRING — it cannot pin a plan, MVCC visibility work or payload I/O, and on MySQL using the index to satisfy ORDER BY is an optimizer choice, not a semantic guarantee. Precision fix, and the reviewer is right that the sloppy version misleads: "any predicate defeats LIMIT" is wrong, since the query's own cursor is a predicate. The distinction is a SEEKABLE PREDICATE ON THE ORDERING KEY (positions the scan, never discards a row) versus a RESIDUAL predicate (discards rows the engine already produced, so LIMIT bounds survivors and says nothing about how many were produced). Restated in the handler, the record and api-conventions. MEDIUM. docs/decisions.md still advertised "a superset LIKE pre-filter that may over-match but never under-match" for list-valued columns. That is the documented entry point for convention lookups, so a maintainer starting there was told to preserve exactly what round 4 deleted, in direct contradiction of the linked active record. Fixed. LOW. Two test comments read as current: "however many non-matching rows" (false past the ceiling — now bounded and cross-referenced to the test that pins that boundary) and the ampersand case "widens to the bare anchor" (no prefix predicate exists; kept because it is the input shape that broke the old scheme, now labelled as such). Fifth consecutive round of stale text, so this sweep was done by grepping the subject across handler, tests, record, decisions.md, api-conventions.md and the endpoint description: LIKE, ESCAPE, pre-filter, superset, over-match, under-match, anchor, keyspace, window, candidate, row cap, index entries, 392, and every deleted constant name. Every surviving hit is now either current-and-correct or explicitly framed as history.	2026-07-27 03:10:36 +02:00
timothy	373956fcee	fix(578): delete the SQL predicate — LIMIT only bounds work when there is nothing to discard ... BLOCKER. Attempt 4 bounded the Id KEYSPACE, and keyspace is not rows. Delete 20,000 historical rows, put one song at Id 20001, query artist?q=que: the walk burned all ten windows on empty ranges and returned [] for a table containing exactly one row. Capacity fell linearly with deletion ratio and no ratio was safe — one placed gap hides the next match. My record called that "heavily fragmented" and the endpoint description said loss happens "on a very large library"; the one-row example disproves both. Option A. The query now carries NO predicate at all — no LIKE, no LOWER, not even IS NOT NULL: SELECT Id, Artists AS Payload FROM SongMetadata WHERE Id > @AfterId ORDER BY Id LIMIT @Batch That is the whole fix, and it is the point. LIMIT truncates what survives a predicate, so with any predicate present it bounds the OUTPUT and says nothing about the WORK; the engine may evaluate and discard arbitrarily many rows first. Stripped to a bare primary-key range, LIMIT n reads exactly n index entries and n rows — independent of sparsity, deletion history or where the gaps fall. All selectivity moves into memory. A short page can now only mean exhaustion, which is precisely what it could not mean while a predicate was present. Four attempts, four wrong quantities: the result (a fixed budget the over-matching pre-filter starved), candidates returned (a no-match query must evaluate every eligible row before returning an empty page), keyspace width (above), and finally actual rows. The record carries the table; it is worth more than the code. Deleting the predicate deletes a whole bug family with it: the JSON-escape reasoning, the narrow-only-on-verbatim-ASCII rule, the exhaustive Unicode sweep that proved it sound, the ESCAPE '/' portability workaround, and the may-over-match-never-under-match invariant that turned out to be conditional on something untrue. SearchFieldValuesPrefilterSupersetTests is deleted entirely; the one assertion worth keeping — that the SQL has no predicate — moved to the query-shape suite, which pins the SQL string exactly so "just a cheap filter" fails a test instead of silently unbounding the walk. Measured cost of no server-side narrowing, on a seeded 20,000-song library (in-memory SQLite): worst case (no match, full walk) 20,000 rows / 10 round trips / 391.9 KiB / 119ms SQL, ~40ms warm end-to-end. Empty q, dense and non-ASCII prefixes all stop on page 1 at ~39 KiB and ~40ms. Judged acceptable for a debounced typeahead against a local file. If it ever is not, the answer is #669, not reintroducing selectivity — the record says so explicitly. Also fixed: - Round-trip count was advertised as 10; it is at most 10 for album_artist and 11 for artist, which also runs its EF query. The MAX(Id) probe is gone with the keyspace scheme, so there is no extra scalar call. - The duplicated-formula ceiling test is deleted rather than rewritten. It re-implemented the loop's arithmetic and would have passed through an off-by-one or a stall in the real loop; the dense integration tests carry that coverage. Its MaxVisited >= Window assertion was a style constraint in correctness clothing. - Stale text swept by grepping the mechanism nouns rather than re-reading: candidate/keyspace/ pre-filter/superset/row cap/LIKE/ESCAPE and the removed constant names, across handler, tests, record, api-conventions and the endpoint description. The two surviving "pre-filter" mentions are deliberate history. Test comments that rendered escaped non-ASCII as literal characters (which contradicted the raw-storage assertion in the same file) now show the escape text. New test List_Valued_Walk_Reads_Live_Rows_Regardless_Of_Id_Density reproduces the one-row killer and fails against attempt 4.	2026-07-27 03:10:36 +02:00
timothy	fbc7b2a1dd	fix(578): bound the Id KEYSPACE — LIMIT cannot bound a query that matches nothing ... BLOCKER. Round 3's ceiling counted LIKE-positive candidates, which is not the quantity that needed bounding. To return an empty page the engine must first evaluate every eligible row, so a no-match query came back with rows.Count == 0, ended the walk having counted zero against the ceiling, and had already inspected the entire table. Round trips and materialized rows were bounded; database work was not. Worse, a dense widened prefix could materialize 20x the candidates and make 10x the round trips of round 1 — a regression dressed as a bound. Third time bounding the wrong quantity: revisions 1-2 bounded the RESULT (a fixed LIMIT budget the widened pattern starved), revision 3 bounded the CANDIDATES, and neither bounds what the database LOOKS AT. Now the Id range is closed on both sides — `Id > @AfterId AND Id <= @AfterId + @Window` — so each round trip is a primary-key range scan of known width. The LIKE still decides what comes back; it no longer decides how much gets looked at. The walk advances by the WINDOW, never by what returned (an empty page means "nothing matched in this stretch", not "exhausted"), and there is deliberately no LIMIT in the SQL — the window caps the row count, and a LIMIT would only restore the illusion that it is doing the bounding. One indexed SELECT MAX(Id) up front stops the walk burning windows on empty keyspace. Result: at most 10 round trips and 20,000 rows inspected for any q, matching or not. New test `List_Valued_Walk_Cannot_Inspect_Past_The_Bound_When_Nothing_Matches` covers the case every earlier bound test missed — they all gave the pre-filter something to return. With a narrow pattern and a needle at row 30,000, revision 3 returns the needle (right answer, unbounded work) and this revision returns []. Paired with a positive control at row 20,000 inside the bound. The ceiling's remaining false negative (20,000 "zzz" rows then "éclair" at 20,001 -> []) is kept and still pinned: it is the documented bounded-best-effort contract at a rarer threshold, not a defect. Stale/false comments, several of them repeat offenders: - The handler still used "Zulu"/"Éclair" as the truncation example, where both orderings pick "Zulu". I had corrected the test and left the comment. Now "Zulu"/"apple" in both. - "leans entirely on the row cap" described a mechanism that no longer exists. - The escape example rendered as literal Édith/É/é rather than the escape TEXT (Édith, É, é) in both the handler and the record, contradicting the explanation it was giving. - "every accented artist was unsuggestable" overstated it: exactly three of the nine pinned cases fail 1b78dc9e (those where query and stored casing differ, so the escape texts diverge); the other six pass. The record now says what the test comment already said. - The record claimed the ordinal switch left response SETS unchanged. False — ordering runs before Take(limit), so with "Zulu"/"apple", empty q and limit=1 the comparer changes which value survives. - The_Scan_Ceiling_Is_A_Whole_Number_Of_Batches was a style preference wearing a correctness costume (a 20,500 ceiling is perfectly safe — the final window clamps). Replaced with the invariant that actually matters: the walk's own arithmetic terminates and lands exactly on the ceiling.	2026-07-27 03:10:35 +02:00
timothy	a37847e509	fix(578): a fixed row budget turns an over-matching prefilter into a false negative ... BLOCKER 1. The invariant a883e5f0 established — "the pre-filter may over-match, it must never under-match" — is only sound while the candidate set is not truncated. It was truncated. A non-ASCII or JSON-escaped prefix collapses the pattern to the bare `%"%` anchor, so every row becomes a candidate, and `ORDER BY Id LIMIT 1000` then spent the whole budget on rows that could not match. Seed 1000 songs by "zzz", put the only "éclair" in row 1001, ask for album_artist?q=é: a883e5f0 returns [], while 1b78dc9e returned "éclair" because its (separately broken) tighter pattern kept the candidate set small. Neither revision was correct — the old one under-matched at the pattern, the new one under-matched at the cap. Widening a predicate under a fixed budget starves it. So the budget is gone. Candidate rows are now walked keyset-paged on Id (`Id > @AfterId … ORDER BY Id LIMIT @Batch`), continuing past non-matching candidates and stopping on the first of: enough distinct exact matches for `limit`, a short page (source exhausted), or a 20,000-candidate-row ceiling in 2,000-row batches. The bound is on effort; it no longer silently decides the result, and the lossy case needs 20,000 rows that already passed the pre-filter before it bites. BLOCKER 2. The endpoint description and the record's rule claimed ordinal matching/dedup/ordering endpoint-wide. False for EF-backed fields: the database runs LOWER/DISTINCT/ORDER BY/LIMIT before any ordinal code, so `genre?q=é` still misses a stored "Éclair" on SQLite. Both are now scoped to the final in-memory stages, and the underlying gap is referenced as #668 rather than described as fixed. #669 (normalized SongArtist table) is referenced as the follow-up for the scan cost. Accuracy corrections to my own claims, all verified by re-running the mutations: - Only THREE of the nine Unicode cases fail 1b78dc9e (é/édith/BJÖRK — where query and stored casing differ, so the escape texts diverge); the other six pass it. The comment said all nine. They stay as continuity coverage, now labelled as such rather than as regression guards. - Ordering_Is_Best_Effort used "Zulu"/"Éclair", where DB and ordinal orderings BOTH pick "Zulu" — it could not demonstrate the divergence it claimed. Now "Zulu"/"apple", which actually diverges: ordinal ranks "Zulu" first, the DB ranks "apple" first, and limit=1 returns ["apple"]. The record sentence was false and is corrected. - The record printed literal "Édith"/"é" where it needed to show the escape TEXT (Édith, é), contradicting the very explanation it was giving. - Corrected the cost claim: the leading wildcard forces scan ACCESS, but each page stops once it has filled @Batch, so a dense query finishes early — it is not necessarily a full table scan. - The Unicode sweep is labelled a PROOF OBLIGATION: it is revision-independent and passes every revision, which is correct for what it is but must not read as regression coverage. One process note: the new record's frontmatter had a lone apostrophe inside a single-quoted YAML scalar ("SQLite's"). decisions_validate.py's hand parser accepted it; scripts/tests caught it.	2026-07-27 03:10:35 +02:00
timothy	1641ca8305	fix(578): the LIKE prefilter under-matched every accented artist; make the superset provable ... Review of 1b78dc9e found the pre-filter's correctness claim was false, and the claim was in the decision record as well as the code. F1 (high). The pattern JSON-encoded the whole query prefix on the reasoning that the stored text escapes non-ASCII, so encoding the prefix the same way would line up. It does not: SQL LOWER() lowercases the *escape text* (`É` -> `é`); it cannot case-fold the codepoint that escape denotes. So `q=é` built `%"é%`, the stored `Édith Piaf` never matched, and the row was discarded before the in-memory filter could accept it. Every accented artist — Beyoncé, Björk, Sigur Rós, Édith Piaf — was silently unsuggestable, which in a music library is the common case. The invariant that was missing, now stated in the code: the SQL pre-filter is an OPTIMIZATION. It may over-match; it must never under-match. Correctness lives in the in-memory filter. So the pattern now narrows only on the leading run of characters the JSON writer stores verbatim and stops at the first character it cannot prove — `q=Beyoncé` still narrows on `beyonc`, `q=é` narrows on nothing and leans on the row cap. Soundness rests on two facts now asserted by exhaustive computation rather than argued: no non-ASCII codepoint in U+0080..U+10FFFF OrdinalIgnoreCase-equals a printable ASCII character (false for InvariantCultureIgnoreCase, which folds ~190 — the choice of Ordinal is load-bearing), and the exact set of ASCII the encoder escapes. F1b. `UseRequestLocalization` honours Accept-Language, so the culture was caller-controlled and `ToLower()` plus the default linguistic `StartsWith(string)` let a header change the answer. Comparison is now OrdinalIgnoreCase and ordering StringComparer.Ordinal throughout — including the shared FilterSortTake that state/video_dynamic_range/content_rating also use. Sets unchanged, order now ordinal rather than culture-dependent. F2. The merge comment asserted an exactness the code does not have: sources truncate by their own ordering (DB collation / primary key), not the merge's, so a dropped value can outrank a survivor. Comment and record now say best-effort, exact only below the truncation points. F3/F4. The cap now rides `ORDER BY Id` rather than the JSON column: MySQL sorts TEXT by only max_sort_length bytes, so the old ordering was not deterministic there, and sorting the whole matching set was avoidable work. What the cap still does NOT bound is the scan — a leading-wildcard LIKE cannot seek an index — so that cost is now documented as accepted, with a normalized `SongArtist` table named as the follow-up candidate rather than left implicit. Every clause above is covered by a test verified to FAIL when that clause is mutated (old pattern builder: 5 red; culture chain: 3 red; cap=3 / cap=limit / ORDER BY json / no cap: red each). F5. Converted to a proper supersession. The old record did not merely hold a stale fact — it recorded song/music-video credits as an "intentionally-uncovered gap" and album_artist as unsupported, and this reverses that call, which `docs.decision-lifecycle` says is never a line-edit. `api.search-field-values` is archived with its original prose restored, and `api.search-field-values-sources` replaces it carrying the whole endpoint contract.	2026-07-27 03:10:35 +02:00
timothy	cd6f36185c	feat(578): artist typeahead covers music-video and song credits; album_artist stops 404ing ... `GET /api/v1/search/fields/{name}/values` sourced `artist` from `ArtistMetadata.Title` only — entity artists — so the free-text credits that `LuceneSearchIndex` also writes to the `artist` field (`MusicVideoArtist.Name`, `SongMetadata.Artists`) produced no suggestions, and `album_artist` 404'd outright. `MusicVideoArtist` turned out to be a real entity table, so it just joins the existing server-side pipeline as a `Concat` — one bounded `UNION ALL` + `LOWER(...) LIKE ... LIMIT` on both providers. `SongMetadata.Artists`/`AlbumArtists` are the hard case: EF 9 maps them as primitive collections (one JSON array per row in a single column), and neither provider can project the elements server-side — SQLite needs the SQL APPLY operator it lacks, Pomelo MySQL 9.0.0 has no primitive-collection support at all. Both failures are now pinned by a test, so a provider upgrade that fixes them shows up as a red rather than as a stale workaround. For those columns the handler pre-filters on the raw JSON (`LOWER(col) LIKE '%"<encoded-prefix>%' ESCAPE '/'` — a deliberate superset, since it matches a row and not an element), caps the rows at 1000 with an `ORDER BY` that makes the truncation deterministic, then splits and exact-filters in memory. Provider portability is by construction rather than by trusting `LIKE`: the prefix is JSON-encoded before matching (which is how it is stored, and which makes the pattern pure ASCII, so SQLite's ASCII-only `lower()` and MySQL's Unicode-aware `LOWER()` agree) and lowercased in C#, so the match is correct under a case-sensitive MySQL collation as well as a case-insensitive one. The escape character is `/`, never `\`: `ESCAPE '\'` is not a portable SQL literal. An empty `q` stays supported for these fields — the row cap already bounds it, and a non-empty-`q` carve-out would make one group of fields behave differently for the same client code. Docs: new `api.search-field-values-list-columns` record (additive sibling — the base record's rule still reads true; only its body's "intentionally-uncovered gap" claim was stale), plus `api-conventions.md`, `spa-conventions.md` §12 and the regenerated `v1.json`. Decisions-Edit: yes	2026-07-27 03:10:35 +02:00
timothy	8de02d5bde	Merge pull request 'fix(649): point the ENFORCED review-verdict gate at the shared PR-file enumeration' (#666) from fix/649-enforced-verdict-guard into main	2026-07-26 22:04:06 +00:00
timothy	3885fd6aea	docs(649): narrow the enumeration's stated guarantees to what it actually proves ... Two limitations the cold review surfaced are now written where the guarantees are described, rather than living only in a review transcript. Both are pre-existing and tracked separately (#663, #664); neither is fixed here. - Head-sha binding detects ONE-WAY movement. An A->B->A force-push round trip restores the expected sha, so the binding holds while the pages came from two states. The record previously read as though the race were closed. - A commit status is repo-GLOBAL, so a success earned on one PR is inherited by any other PR with the same head. Same property that makes the per-sha binding work, read from the other end. Refs #649 Decisions-Edit: yes	2026-07-26 23:29:44 +02:00
timothy	d51255a8ef	fix(632): "could not check" is a third outcome, not a quiet synonym for "nothing to check" ... Cold review's substantive finding. The first draft collapsed an unreadable status response into the graceful-adoption path: `vdesc` came back empty, so `recorded_base` was empty, so the comparison was skipped IN SILENCE — and a later, successful status read could then auto-grant, emitting "merge gate: satisfied" for a comparison that never happened. A transient Gitea hiccup is not evidence that the base is unchanged. The unreadable status response and a PR with no resolvable `.base.ref` now both fall through to a human `ask`, leaving exactly one benign silent case: a verdict that predates #632 and could not have carried the field. The emptiness check is done in SHELL before jq sees it, same jq-1.6 rule as the rest of this file. Also from review: the graceful-adoption test asserted only that the decision lacked the issue tag, so it would have passed for a base-specific ask or deny whose wording omitted it — the failure mode most likely to appear when someone edits these messages. It now asserts on the word "base". Recorded rather than fixed, because fixing it would be worse: docs-only PRs exit before this check, since that carve-out short-circuits the gate earlier. It does not auto-grant — it passes through to an ordinary permission prompt — so the exposure is a missing warning on a merge a human is already confirming, not a silent merge. The record now says so instead of implying the deny is unconditional. Mutation-verified: collapsing the unreadable case back into graceful adoption, skipping the check on a missing live base, and dropping the mismatch deny each redden their own test and nothing else. Refs #632	2026-07-26 23:27:22 +02:00
timothy	00e623c066	fix(632): bind a review verdict to its BASE branch, not only to its head sha ... #622 made `review-verdict/h10` a per-sha required status, so a new commit cannot inherit an old verdict — the required context is simply absent on the new head. Retargeting a PR's base reaches the same end from the opposite direction: the head sha and the status both hold still while the merge-base, and therefore the effective diff the verdict was formed against, changes underneath them. #622's record claimed the invariant holds "by construction"; this was the documented exception, and an unrecorded exception is how a guarantee degrades into a habit. `post-review-verdict.sh` now records the base branch in the status description as a trailing `(base: <ref>)`, and refuses to write a status at all if the base moved between reading the PR and posting — the same TOCTOU window the head check already covers, which the head check cannot see because retargeting does not move the head. `pretooluse-merge-consent.sh` reads the field back and denies when it no longer matches the PR's live `base.ref`. Two choices are load-bearing, and each is pinned by a test rather than left to a comment: - The comparator is `base.ref`, NOT `base.sha`. `base.sha` tracks the base branch's tip, which moves whenever anything merges to `main` — comparing it would invalidate every open verdict on every unrelated merge, converting a rare-event guard into a permanent merge deadlock. A base that merely advances is out of scope by design: rebasing onto it moves the head sha, which the per-sha binding already covers. - The field goes in the status DESCRIPTION, not the verdict comment. The comment body is parsed by `scripts/check-review-verdict.sh`, whose grammar had three false-opens in its history (#629); nothing parses the description, so this adds a field without reopening that surface. Scope is stated honestly rather than overclaimed: this is DETECTION on the hook path only. A commit status carries no base of its own, so the server-side required check cannot see a retarget, and a merge driven through the Gitea UI or API is unaffected. That is the accepted exposure — base changes are rare, manual, and this is a two-account repo — but it now fails loud in the one place that evaluates consent, instead of living only in a doc. Verdicts posted before this change carry no `(base: …)` and get NO opinion rather than a deny; denying would block every in-flight PR the day it lands, and the window closes on its own since verdicts are per-head and short-lived. Verified by mutation, six mutants, each killed by its intended test: remove the hook's deny; compare base.sha instead of base.ref; drop graceful adoption; stop recording the base; drop the TOCTOU guard; accept a PR with no resolvable base. The positive controls matter more than usual here — the test PR is deliberately non-docs (a docs-only PR short-circuits the whole gate and would never reach the base check) and the rest of the gate is unstubbed, so "the hook denied" alone proves nothing. Refs #632 Decisions-Edit: yes	2026-07-26 23:13:50 +02:00