fix(684): key the pageSize guard registry on identity, not source position #686

Merged

timothy merged 2 commits from fix/pagesize-guard-line-churn into main 2026-07-27 19:39:39 +02:00

Conversation 1 Commits 2 Files Changed 3

+285 -97

timothy commented 2026-07-27 19:09:01 +02:00

Owner

Copy Link

Copy Source

fixes #684

Unbreaks main, which is red on the #650 pageSize call-site guard, and removes the recurrence.

What was actually wrong

The guard's registry pinned every call site by absolute line:column, making it a function of every
other file in the repo.

It was born red. #651 moved AutoTuneScreen.tsx up ten lines and FillerPresetsScreen.tsx down
seventy-two, and merged 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 the red; it first surfaced on the next push (#676's merge, which touches no web/src file
and is not the cause).

That is one ordering accident, not a recurring pattern — but the exposure is general: every PR is
green against its own base, so the breakage exists only in the merge result and lands on main after
review and after the merge gate.

Note the later "green" main runs (1393, 1388) don't contradict this — ~47s Build & test, i.e. the
docs-only fast path, so the web suite never ran.

The fix

• Registry identity is (file, kind, value). New / removed / changed sites all still fail. The
  MULTISET comparison is preserved. The scanner's positional pageSizeSiteId is untouched —
  pageSizeScan.test.ts asserts real AST positions against fixed fixtures, the opposite case.
• The one case this costs is stated, not implied: a same-identity substitution within one file now
  passes. Narrow, and previously caught only incidentally. Named in both 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; adds a search-bounded class for the windows-nothing shape.

Two things the review caught that changed the diff

Codex was over quota, so review was same-family, cold-brief, worktree-isolated. It found:

1. My root-cause narrative was wrong. I had written that #676 shifted AutoTuneScreen.tsx and
   that the guard "broke twice". #676 touches zero web/src files, and #651 is an ancestor of the
   guard's own merge. Corrected in the commit, the record and the guard comment.
2. A search-bounded classification was false, and it mattered. CollectionsScreen's
   AddItemsDialog degrades to an unfiltered whole-type 50-row window on an empty query (blank
   submit, or a kind-chip click), surfacing no totalCount. Labelling that search-bounded would have
   documented a live §3b violation as correct.

So this adds a deviation class instead: a registry must not launder a defect into a
compliant-looking label. Deviation entries must name a tracking issue — filed as #685 — enforced
by a structural field, because the first version of that test passed with the tracking reference
deleted (the note legitimately cites #644/#650).

Also corrects SmartCollectionDialog to class-b: it does render a {count} matches badge from
totalCount, which is class-b's defining evidence.

Verification

Behaviour verified by mutation, not by green, with each mutation confirmed to have applied:

Mutation	Expected	Result
New call site added	FAIL	✅
Registry entry removed	FAIL	✅
pageSize value changed	FAIL	✅
Third occurrence of a twice-registered identity	FAIL	✅
Deviation loses its tracking issue	FAIL	✅
Non-deviation gains an issue field	FAIL	✅
Report stops printing positions	FAIL	✅
Site moved 40 lines, otherwise unchanged	PASS	✅

Full web suite 1184/1184 · scripts/tests 338/338 · npm run typecheck + eslint clean ·
decisions validator green in CI diff mode (--base origin/main --head HEAD).

One note on the record: it was trimmed from 75 to 47 prose lines because
test_real_corpus_ceiling_sits_at_the_TAIL_BOUNDARY_of_the_distribution failed — my record sat
exactly on the p90 index. Trimmed rather than re-deriving the ceiling, which is what that test is
there to prevent.

fixes #684 Unbreaks `main`, which is red on the #650 `pageSize` call-site guard, and removes the recurrence. ## What was actually wrong The guard's registry pinned every call site by absolute `line:column`, making it a function of every *other* file in the repo. **It was born red.** #651 moved `AutoTuneScreen.tsx` up ten lines and `FillerPresetsScreen.tsx` down seventy-two, and merged **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 the red; it first surfaced on the next push (#676's merge, which touches no `web/src` file and is not the cause). That is one ordering accident, not a recurring pattern — but the exposure is general: every PR is green against its own base, so the breakage exists only in the merge result and lands on `main` after review and after the merge gate. Note the later "green" main runs (1393, 1388) don't contradict this — ~47s `Build & test`, i.e. the docs-only fast path, so the web suite never ran. ## The fix - Registry identity is `(file, kind, value)`. New / removed / changed sites all still fail. The MULTISET comparison is preserved. The scanner's positional `pageSizeSiteId` is untouched — `pageSizeScan.test.ts` asserts real AST positions against *fixed* fixtures, the opposite case. - **The one case this costs is stated, not implied:** a same-identity substitution within one file now passes. Narrow, and previously caught only incidentally. Named in both 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; adds a `search-bounded` class for the windows-nothing shape. ## Two things the review caught that changed the diff Codex was over quota, so review was same-family, cold-brief, worktree-isolated. It found: 1. **My root-cause narrative was wrong.** I had written that #676 shifted `AutoTuneScreen.tsx` and that the guard "broke twice". #676 touches zero `web/src` files, and #651 is an ancestor of the guard's own merge. Corrected in the commit, the record and the guard comment. 2. **A `search-bounded` classification was false, and it mattered.** `CollectionsScreen`'s `AddItemsDialog` degrades to an unfiltered whole-type 50-row window on an *empty* query (blank submit, or a kind-chip click), surfacing no `totalCount`. Labelling that `search-bounded` would have documented a live §3b violation as correct. So this adds a **`deviation`** class instead: a registry must not launder a defect into a compliant-looking label. Deviation entries must name a tracking issue — filed as **#685** — enforced by a *structural* field, because the first version of that test passed with the tracking reference deleted (the note legitimately cites `#644`/`#650`). Also corrects `SmartCollectionDialog` to `class-b`: it *does* render a `{count} matches` badge from `totalCount`, which is class-b's defining evidence. ## Verification Behaviour verified by **mutation**, not by green, with each mutation confirmed to have applied: | Mutation | Expected | Result | |---|---|---| | New call site added | FAIL | ✅ | | Registry entry removed | FAIL | ✅ | | `pageSize` value changed | FAIL | ✅ | | Third occurrence of a twice-registered identity | FAIL | ✅ | | Deviation loses its tracking issue | FAIL | ✅ | | Non-deviation gains an issue field | FAIL | ✅ | | Report stops printing positions | FAIL | ✅ | | **Site moved 40 lines, otherwise unchanged** | **PASS** | ✅ | Full web suite 1184/1184 · `scripts/tests` 338/338 · `npm run typecheck` + eslint clean · decisions validator green in CI diff mode (`--base origin/main --head HEAD`). One note on the record: it was trimmed from 75 to 47 prose lines because `test_real_corpus_ceiling_sits_at_the_TAIL_BOUNDARY_of_the_distribution` failed — my record sat exactly on the p90 index. Trimmed rather than re-deriving the ceiling, which is what that test is there to prevent.

timothy added 1 commit 2026-07-27 19:09:02 +02:00

fix(684): key the pageSize guard registry on identity, not source position ... fc8353c75c

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>

timothy added 1 commit 2026-07-27 19:17:11 +02:00

fix(684): review round 2 -- drop a false exhaustiveness claim I introduced ... 689451161e

The L1 fix (name every class-b shape) collided with the M1 fix (move
SmartCollectionDialog into class-b) in the same commit: the comment claimed
"exactly three shapes, which is the whole list" while the registry 80 lines
below already held four. That is the same false-exhaustiveness defect this PR
exists to remove -- a reader adding a fifth class-b site would conclude theirs
cannot be class-b despite rendering a real totalCount hint, and reach for
search-bounded or deviation.

The operative rule is now stated first and the shape list is explicitly
illustrative: a site qualifies by RENDERING a totalCount-backed hint, not by
resembling a listed shape.

Also, both non-blocking review items:
- the deviation prose said the tracking issue goes "in the note", while
  enforcement is on the structural `issue` field -- it now points at the
  mechanism that actually binds, and says why the note scrape was rejected;
- the UNREGISTERED report prints every position sharing an identity, so it now
  says "identity seen at:" rather than implying all of them are unregistered. A
  positionless key cannot know which occurrence is excess; the candidate set is
  the honest answer.

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

timothy commented 2026-07-27 19:20:28 +02:00

Author

Owner

Copy Link

Copy Source

Review-verdict: MERGEABLE @ 6894511

Cold-brief adversarial review, 3 rounds, worktree-isolated. Round 1 BLOCKED on a misattributed merge in the decision record and a false search-bounded classification that would have documented a live 3b violation as compliant (filed as #685). Round 2 BLOCKED on a false exhaustiveness claim I introduced by writing the class-b comment and the registry in the same pass. Round 3 clean at this head: registry counted independently (18 entries, 4 class-b, each named shape mapped 1:1 to source), the render-is-the-entry-requirement rule verified true of all four, and every guard behaviour re-verified by mutation with each mutation confirmed applied. Codex was over quota so this is same-family, not cross-family.

Review-verdict: MERGEABLE @ 6894511 Cold-brief adversarial review, 3 rounds, worktree-isolated. Round 1 BLOCKED on a misattributed merge in the decision record and a false search-bounded classification that would have documented a live 3b violation as compliant (filed as #685). Round 2 BLOCKED on a false exhaustiveness claim I introduced by writing the class-b comment and the registry in the same pass. Round 3 clean at this head: registry counted independently (18 entries, 4 class-b, each named shape mapped 1:1 to source), the render-is-the-entry-requirement rule verified true of all four, and every guard behaviour re-verified by mutation with each mutation confirmed applied. Codex was over quota so this is same-family, not cross-family.

timothy merged commit aa1f504e02 into main 2026-07-27 19:39:39 +02:00

timothy deleted branch fix/pagesize-guard-line-churn 2026-07-27 19:39:40 +02:00

timothy referenced this issue from a commit 2026-07-27 19:39:42 +02:00

Merge pull request 'fix(684): key the pageSize guard registry on identity, not source position' (#686) from fix/pagesize-guard-line-churn into main

timothy referenced this pull request 2026-07-27 19:40:20 +02:00

main is red: the #650 pageSize call-site guard keys its registry on line:column, so any merge that shifts a line breaks it #684

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

ad-hoc

One-off / ad-hoc work not tracked by a dedicated issue

api

REST API / HTTP endpoints

bug

Something isn't working

ci-cd

Build, test, deploy pipeline

content

Channel content / schedules / playlists

dependencies

Dependency updates (Renovate)

enhancement

New feature or improvement

frontend

ChicoryTV React SPA frontend

in-progress

Claimed by an active session — do not pick up

jellyfin

Jellyfin tuner / IPTV integration

parked

Excluded from automatic queue pickup; work only when explicitly selected

priority: high

priority: low

priority: medium

review

Adversarial review finding

security

Security / vulnerability fix

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

renovate timothy (Timothy)

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: timothy/ersatztv#686