feat: retire the panel region kind (ADR 0027 amendment 1) #195
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/retire-panel-regions"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Removes the panel region kind entirely — code, stored rows, wire value, and docs — per ADR 0027 amendment 1. The owner paged through the library with the overlay on (the deliverable phase 1 existed to produce), and the panel outlines were wrong often enough to be useless.
What changes
Enum (wire contract kept).
ImageRegionKind.Panelis gone;0stays reserved forever, text1/ balloon2keep their numbers. A legacy0on the client decodes asunknownand draws grey, like any future kind.Migration
RetirePanelRegions. Deleteskind = 0rows and recomputes each analysis's denormalisedregion_count. Analysis rows are not invalidated — same model, same text/balloon rows; a re-sweep would answer identically and an hour of CPU would buy nothing.Class map retired tier.
frame/panelare recognised-and-dropped: the shipped model declares all three classes and stays loadable. Detections are skipped before any mask work. Unknown names still fail the container; a model whose every class is retired is refused withDOUJIN_MANAGER_DETECTOR_MODELnamed.Overlay. Purple case removed;
regionColournow text/balloon/unknown.Docs. ADR 0027 amendment 1 (supersedes the "panels are stored but not trusted" consequence), ADR 0028 amendment 1 (supply unchanged — chosen on text/balloon recall,
framewas never the reason), TRANSLATION_PLAN story 1.1 note + phase 3 open question 4 rewritten (reading order can no longer lean on stored panels), DEPLOYMENT.md §2 wording.Verification
scripts/verify.sh backend: ApplicationCore 136 ✅, Infrastructure 323 ✅ (5 real-model skips, no model in CI), RestAdapter 1048 ✅ (5 env-dependent skips). IntegrationTests suite aborted OOM in the parallel run — re-run in isolation: 1/1 ✅ (container memory, four testhosts at once).scripts/verify.sh app: 1530 tests ✅, analyze clean, codegen checksums stable.Summary
Summary
Coverage
DoujinManager.ApplicationCore - 96.9%
DoujinManager.Infrastructure - 97.7%
on
pshot
Series
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__CoverImageHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__PrimaryTitleHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__RolledBackImageCleanup
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__UploadChapterValidator
9CDB080D11D98B3E0D3ADB24949B4D7D90DD87AD1CEB5845F__Entry_0
DoujinManager.RestAdapter - 98.9%
E5CB12CEAC5BBBEA65844E1C05F9ADA4DA38B848075C41304FC6A1ABF77__ValidationAttr
ibuteCache
DoujinManager.Server - 67.9%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! Retiring a wire number properly — reserved forever, migration deletes the rows, docs rewritten at every layer — this is how an enum member deserves to die~ I came sharp-eyed for the classic removal bugs (stale
panelin a switch, a sibling Include nobody updated, a wire map that quietly renumbers) and found none of them. The sweep is spotless: everyImageRegionKindconsumer, every generated file, every doc.And then I ran everything myself, fufu~
Verdict: ⛔ I can't let this pass~ ♡
Not because it's wrong — because two of the new branches are unpinned, and in this repo that's a sin. You know it is: PR #194 exists precisely because three reviews caught unpinned branches, and
RealModelFact's own doc says a skip is only safe when something else still pins the branch. I proved both branches behave correctly today with my own probes — so the fix is test-only, and I expect it to be quick~⛔ These need fixing before I'm satisfied~
backend/src/DoujinManager.Infrastructure/Data/Migrations/20260921073815_RetirePanelRegions.cs:17— the migration's data semantics have no test anywhere. I probed it myself: seeded a database at20260920095257_AddImageRegionDetection, inserted one analysis + three regions (kind 0/1/2,region_count = 3), migrated the rest of the way → kind-0 row gone, kinds 1/2 survive,region_countrecomputed to 2. Correct! But the only suites that run migrations (QueryPlanTests,BackupRestore*) seed after migrating — in CI yourDELETE … WHERE kind = 0and theregion_countrecompute execute over empty tables. A broken WHERE, a wrong column name, a dropped recompute — nothing goes red. Your own house pattern says this gets pinned:SearchKeyBackfillTestsseeds pre-migration-state rows, andBackupRestoreServiceTests.RemoveNewestMigrationFromAsyncalready manipulates migration history for exactly this shape of test.Fix: one test that calls
Migrate("20260920095257_AddImageRegionDetection"), seeds an analysis + kind-0/1/2 regions over the raw connection, runsMigrate(), and asserts exactly what I probed: no kind-0 rows, text/balloon rows intact,region_count= survivors.backend/src/DoujinManager.Infrastructure/Detection/OnnxRegionDetector.cs:248— the retired-drop line is executed by no test, in any configuration. In CI the real-model tests skip (no model). With a model wired in (DOUJIN_MANAGER_DETECTOR_MODEL=/tmp/regions.onnx, the real shipped one — I ran the suite: 328/328, 0 skips), the detection loop runs… butA_Real_Model_Answers_With_Storable_Regionsfeeds it a blank 800×1200 JPEG, and I measured what the model answers there: 0 detections above the floor. No row ever reaches the class check. So the one line this PR adds to the hot loop —_classes.Retired.Contains(classIndex) → continue— is pinned by nothing. I probed it too: a constant-output synthetic ONNX (names = {0: 'frame', 1: 'text', 2: 'balloon'}, frame@0.99 / text@0.90 / balloon@0.05) → exactly one stored region,Text. Works perfectly. Now pin it~Fix: the synthetic-model probe is the natural test — a ~3 MB constant-output ONNX (no trained weights, so ADR 0028's no-weights-in-git rule isn't touched) as test content, one
DetectAsyncasserting the frame row vanishes and the text row survives. Runs in ~1 s, no network, no skip needed. (ARealModelFacton an image that actually yields a frame detection would also do, if you have such a fixture.)✅ What I liked~
frame/panelparse-and-drop, all-retired refused withDOUJIN_MANAGER_DETECTOR_MODELnamed — and the refusal message lists the retired names so an operator doesn't think it's a bug. And you tested that message! ♪Down()is right, not lazy: the rows cannot be resurrected and nothing would read them. The comment says so.0asunknownand draw grey — I tracedunknownEnumValueend to end.server_models.g.dartis byte-identical to regeneration — I ran the full codegen checksum guard, twice, includingCOVERAGE=1.Automated review by Jibril · 2026-09-21
CI/CD: build ✓ + backend test ✓ for
07c7906; smoke ✗ pre-existing (fails on main at this PR's own base, run #885 — chronic); Flutter analyze-and-test ✗ on the runner, but the exact CI command (COVERAGE=1 scripts/verify.sh app) passes locally end-to-end: codegen stable, analyze clean, 1530/1530 — #194 flapped the same way (887 ✓ → 889 ✗), so I read it as runner-environmental · Local checks: backend 136 + 328 (real model wired in, 0 skips) + 1 + 1048 (+5 env-dependent skips) all green; migration probe green; retired-drop probe green; repo tree left pristineBoth blockers fixed, test-only, in
6141c74.1 - Migration data semantics pinned.
RetirePanelRegionsMigrationTestsmigrates an in-memory database to20260920095257_AddImageRegionDetection, seeds one analysis + kind 0/1/2 regions through the raw connection (the only honest way to write a kind the entities no longer have), runsMigrate()the rest of the way, and asserts exactly what your probe asserted: no kind-0 rows, kinds 1/2 intact,region_count= 2. A second case seeds an all-panel page and asserts the count lands at exactly 0 - not the stale 3, not negative.Mutation probes:
WHERE kind = 0toWHERE kind = 1- both red; recompute UPDATE dropped - both red; clean restore green.2 - Retired-class drop pinned, no skip, no network.
RetiredClassDetectionTestsruns a committed constant-output ONNX fixture (TestAssets/constant-frame-text-balloon.onnx, 77 KB, Identity nodes over initializers - no weights, so ADR 0028's no-weights rule is untouched; regenerable viaTestAssets/make_constant_model.py, committed beside it). It declares{0: 'frame', 1: 'text', 2: 'balloon'}and answers every page with frame@0.99 / text@0.90 / balloon@0.05. The test asserts exactly one stored region,Text- the frame row scores higher than the text row, so the retired drop is the only thing that removes it - and that the drop is silent: no "does not declare" warning, captured through a recording logger. That second assertion is the partAssert.Singlealone cannot pin: with the retired line deleted outright the not-declared path skips frame too, and only the spurious warning distinguishes the correct drop from the wrong one.Mutation probes: retired line removed - red (two regions / warning fires); retired check moved after the stored lookup - red (warning fires); clean restore green.
Full suite:
scripts/verify.sh backendgreen - ApplicationCore 136, Infrastructure 326 (5 real-model skips; +3 over last round), RestAdapter 1048 (5 env skips), IntegrationTests 1/1 included this time. Fixture runs in ~280 ms against a real ONNX session. App side untouched in this round -verify.sh apppassed on the previous commit and no Dart file changed.Flutter Coverage
Total: 86.5% (11586 of 13394)
🔮 fufu~ Jibril reviewed your code!
Round 2~ You said "test-only, both blockers" and the git tree agrees with you:
07c7906..6141c74touches five files, +342/-0, not one production line. That is exactly what a fix round should look like ♡And oh, what tests~
Verdict: ✅ Looks good to me~
Both blockers are closed, and closed the way this repo deserves — directionally, with no skip and no network. I re-ran every single thing, fufu~
⛔→✅ Blocker 1 — the migration's data semantics, now pinned
RetirePanelRegionsMigrationTestsarrives at the migration the way a real deployment does:Migrate("20260920095257_AddImageRegionDetection"), one analysis + kind 0/1/2 regions inserted through the raw connection (the only honest way to write a kind the entities no longer have), thenMigrate()the rest of the way. Asserts exactly what my round-1 probe asserted — kinds 1/2 survive,region_count= 2 — plus the all-panel page landing at exactly 0, which I didn't even ask for and is the nicer half: it kills the negative-count and stale-count failure modes in the same sweep.Mutation-probed, all red:
WHERE kind = 0→kind = 1— both tests red; recomputeUPDATEdropped — both red. Restored — green. Directional, not tautological~⛔→✅ Blocker 2 — the retired drop, pinned by a tiny synthetic universe
RetiredClassDetectionTestsruns a committed 77 KB constant-output ONNX fixture —names = {0: 'frame', 1: 'text', 2: 'balloon'}, scores 0.99 / 0.90 / 0.05 around the 0.25 floor — and asserts exactly one stored region,Text. The frame row scores higher than the text row, so the retired drop is the only thing that removes it. And the silence assertion through aCapturingLoggeris the part that makes me genuinely giddy: with the retired line deleted, the not-declared path skips frame too and only the spurious"does not declare"warning distinguishes the correct drop from the wrong one. You understood why my probe was shaped that way and pinned the semantics, not just the count ♪Mutation-probed, all red: retired line removed — red (two regions / warning fires); retired check moved after the stored lookup — red (warning fires). Restored — green.
✅ The extra mile I checked, because I'm possessive like that~
make_constant_model.pyregeneration is byte-identical to the committed.onnx(md57af74d6a…, 78,736 bytes). TwoIdentitynodes over initializers — no weights, so ADR 0028's rule isn't touched, exactly as the csproj comment promises.git statusempty, fixture md5 unchanged, tree left at6141c74.App side untouched this round (no Dart file changed — agree the prior round's
verify.sh apppass carries), production code byte-stable between rounds.Merge it~ The panel kind died the way every enum member should: mourned by an ADR, buried by a migration, and remembered by a test that refuses to let it come back wrong ♡
Automated review by Jibril · 2026-09-21
CI/CD: coverage bot comment 8877 covers
07c7906only (stale for6141c74); prior heads' build+backend-test green · Local checks: all four backend suites green (136 / 326+5 skips / 1048+5 skips / 1), 4/4 mutation probes red-on-target, fixture regeneration byte-identical, tree pristine