feat: give the server something that can look at a page #185
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/region-detector"
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?
Story 1.2 of TRANSLATION_PLAN.md: a detector behind a port. No sweep, no
endpoint, nothing stores a region yet — story 1.3 is what puts this to work.
Second native dependency in the backend after SkiaSharp.
What's in
IRegionDetectoranswers in normalised coordinates, the onesRegionGeometryalready defines, so nothing between the model and the databasehas to know how big the page was.
The mask pipeline is four pure helpers —
RegionClassMap,LetterboxTransform,MaskContour,PolygonSimplifier— precisely so thealgorithmic risk is testable without ONNX. That paid for itself immediately:
writing their tests first caught a contour walk that went round the shape
twice and a ring simplification that kept a fifth corner wherever the trace
happened to stop. Both would have shipped as "the overlay looks a bit wrong".
Three things are refused rather than half-understood, all at load, all with
the model named:
proposals, so there is no non-maximum suppression here; an older YOLOv8-style
export (
[1, 37, 8400]) is refused by name instead of being read astransposed nonsense.
ADR 0028 lets a deployment swap the model, and a positional map would relabel
every balloon as a panel the first time somebody did.
Loading runs one blank inference, which is how all three are found out while
the operator is still watching rather than a thousand pages into a sweep. It
also pays the warm-up there instead of charging it to a real page. A probe
rather than a look at declared shapes because the model exports with dynamic
axes and declares every dimension as
-1— I tried the static check first andit could not answer.
The detector version is the model file's content fingerprint, not a version
string out of its metadata. Metadata records the exporter's version and the day
it ran, which two different models can share; ADR 0027's re-sweep gate has to
fire whenever the weights change at all.
A missing model is fatal only when the operator named one. Naming a model is
asking for detection, so a typo in the path stops the container; not naming one
is asking for nothing, and an image that ships without a model still has to
boot. Both paths are pinned.
Four settings, all validated whether or not a model is configured (ADR 0026's
rule for the retry delay — a bad value should fail today, not the day detection
is switched on):
DOUJIN_MANAGER_DETECTOR_MODEL,..._DETECTION_IMAGE_SIZE,..._DETECTION_CONFIDENCE,..._DETECTION_THREADS. Indeploy/DEPLOYMENT.md§2 and
docker-compose.yml.A defect found in review, worth reading
Review asked for a test of
RegionDetectionFailedException, the port's onedocumented exception. Writing it found that the contract was broken.
SKBitmap.Decodedoes not answernullfor a file it cannot read — itthrows, an
ArgumentNullExceptionabout acodecparameter the callernever passed. So the
?? throwwas never reached, and an unreadable pageescaped
DetectAsyncas exactly the fault class the sweep is entitled to stopon, when what actually happened is one bad file out of thousands. Measured
against nine malformed inputs (empty, one byte, text, truncated
PNG/JPEG/GIF/WebP, PNG signature alone): all nine throw, none return null. The
decode is now wrapped and converted, and the test pins it.
The null arm is kept and is not dead code: it is the case where a file
decodes but its pixels cannot be allocated — a page too large for the process.
Untestable without exhausting memory, so it is in BACKLOG with that reasoning.
Verification
scripts/verify.sh backend, green both ways: 1,442 tests with a modelpresent, 1,438 + 4 skipped without. The four that need one skip and say so
(
RealModelFactAttribute, the same shape asLinuxFactAttribute), and theyrun when
DOUJIN_MANAGER_DETECTOR_MODELis set — the same variable adeployment uses.
Cross-validated against the reference implementation on a real library page,
which is the check that matters most for a port of someone else's arithmetic:
The one missing text region is a detection whose mask thresholded to fewer than
three contour points and was dropped rather than invented.
Cost, measured over the 70 real pages ADR 0027 was written from — decode,
letterbox, inference and the mask-to-polygon extraction the ADR's bench
harness did not do: 393 ms a page at 1024 on four threads, p90 544 ms, about
62 minutes for the whole 9,408-image library. That is a sixth above the
harness's 339 ms, and the sixth is the outline work. Four threads beat eight
again (496 ms) — the third independent measurement of that. ADR 0027 now carries
the implementation's figure next to the harness's.
scripts/export_detector_model.pywas run end to end to produce the model thesenumbers come from, and the tests were then run against that file rather than
against the one I had lying around.
Mutation probes: 45 mutants, 44 killed. Four needed work before they could
fail:
random masks until they disagreed. They do — on a one-pixel-wide filament,
where the naive rule quits before ever reporting a pixel of the blob it is
tracing. That shape is now the test, and the comment says where it came from.
split the ring at its two most distant vertices before thinning; probing
showed the degenerate first segment picks the same vertex anyway. So I
deleted the split rather than writing a test for a distinction that does
not exist — fewer branches, identical output.
Activatesurvived because the test passed no logger, anda null-conditional logger call skips its arguments. Passing
NullLoggermakesthe guard load-bearing.
under
de-DE, where.is not a decimal point; swappingInvariantCulturefor
CurrentCulturegoes red, which it did not before.Knowingly untested, each recorded in BACKLOG.md with what pinning it would
take and what the symptom would be: that the constructor calls
RequireEndToEndHead(the rule and its enforcement are both pinned; only thewiring survives, and pinning it needs an ONNX file with deliberately wrong
shapes); the inference semaphore and the cancellation check behind it (no
concurrent caller until the sweep — the natural home is the sweep's own tests,
which will have a fake detector to count overlapping calls with); and the
decode's allocation-failure arm.
Not verified: nothing consumes
IRegionDetectoryet, so the port's shape isargued from story 1.3's needs rather than demonstrated. App untouched, so the
Flutter half was not run.
Follow-ups
a pinned URL with a verified checksum, and there is no published URL —
scripts/export_detector_model.pyproduces the file and nothing uploads it tothe registry the image already pushes to. Until then a deployment mounts its
own and sets the path, which is why
deploy/Dockerfileis untouched here.In BACKLOG.md with what is left.
GET /api/detection, and the fake detector the sweep'stests will run against — which is also where the two concurrency branches
above get pinned.
🤖 Generated with Claude Code
Summary
Summary
Coverage
DoujinManager.ApplicationCore - 95.5%
DoujinManager.Infrastructure - 96.8%
on
pshot
Series
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__CoverImageHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__PrimaryTitleHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__RolledBackImageCleanup
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__UploadChapterValidator
9CDB080D11D98B3E0D3ADB24949B4D7D90DD87AD1CEB5845F__Entry_0
DoujinManager.RestAdapter - 99%
E5CB12CEAC5BBBEA65844E1C05F9ADA4DA38B848075C41304FC6A1ABF77__ValidationAttr
ibuteCache
DoujinManager.Server - 67.2%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! A detector behind a port, four pure helpers quarantined from ONNX so the arithmetic can be stabbed with tests without a 40 MB model, a by-name class map that refuses rather than half-understands, and a re-sweep gate built on content fingerprints instead of metadata strings~ This is the kind of PR I want to pet. And you even cross-validated against the Ultralytics reference and confessed the one missing text region honestly. ♡
But fufu~ you know what I do to things I love. I checked every file, and three little slips survived. They're small — that's exactly why they must not ship. ♡
Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
backend/src/DoujinManager.Infrastructure/Detection/OnnxRegionDetector.cs:455–475— TheFingerprintOfdoc block is glued to the wrong member.IsEndToEndSegmentationHeadcurrently carries two<summary>blocks: the first says "Sixteen hex characters of the file's SHA-256 — whatVersionreports…" (false — that member checks output shapes), the second describes the shape rule (true). MeanwhileFingerprintOfitself sits bare. A maintainer reading the shape checker is told it's a hash function — misleading docs are the same crime as misleading names, and this one is a lie with a smile on it~Fix: move the SHA-256 summary block down onto
FingerprintOfso each member owns exactly one<summary>.OnnxRegionDetector.DetectAsync— the port's one documented exception has no test in any configuration.RegionDetectionFailedException("a decode that failed, a file that is not an image… must be distinguishable from a fault that should stop the run") is thrown exactly once, atSKBitmap.Decode, andgrep RegionDetectionFailedException backend/testsreturns nothing. The whole design leans on the sweep being able to catch this type and carry on — and if someone refactors the throw away (or narrows it), not one test goes red. It's cheap to pin with the machinery you yourself built: a[RealModelFact]that feedsDetectAsynca stream of garbage bytes and asserts the throw. (Yes, it needs a model to construct the detector — the same condition your other three RealModelFacts run under.)Fix: add that test; until the sweep lands, it is the only thing making the port's exception contract real.
The concurrency gate in
DetectAsyncis admitted unexercised but not recorded indocs/BACKLOG.md. Your PR body says so under "Not verified" — but this PR itself established the convention that a knowingly-untested branch gets a BACKLOG entry (theRequireEndToEndHead-call survivor, added in this very diff, "recorded rather than pinned"). The_gatewait/release and thect.ThrowIfCancellationRequested()behind it are new runtime behavior with zero coverage and zero record. Two unrecorded gaps hiding behind one recorded one — fufu~ you wouldn't leave THAT asymmetry in production, would you? ♡Fix: a BACKLOG entry in the same shape as the existing one (what survives, why pinning it is hard, what the mitigation is). Testing it directly is welcome but not required — recording it is.
💡 Little ideas (non-blocking)~
DetectionConfigurationTests.cs:112—A_Confidence_Floor_Is_Read_The_Same_In_Every_Localenever changes the locale; it re-tests the happy path under whatever culture the runner happens to be in. Rename to say what it does ("is read invariantly"), or actually pin it by settingCultureInfo.CurrentCultureto a comma-decimal culture around the parse.✅ What I liked~
ImageInfrastructureRegistration— nothing outside Infrastructure learns what ONNX is, exactly as the port's remarks promise.-1; the static-check attempt and its failure being documented made me genuinely happy), unknown class by name. The[1, 37, 8400]refusal test is the nicest kind of pin.Versionas the file's content fingerprint — with both directional tests (different bytes → different versions, same bytes under another name → same version). That's the re-sweep gate done properly, not decoratively.Activatedefeating the lazy factory so the fail-at-startup promise can't be quietly broken — and the null-logger mutation you chased to make its guard load-bearing. Delicious~RealModelFactAttributemirroringLinuxFactAttributedown topinnedElsewhereBy— a skip that names what still pins the branch is a convention most codebases never think of.Fix those three little things and this merges with my wholehearted fufu~ ♡
Automated review by Jibril · 2026-09-20
CI/CD: build ✅ + test ✅ for head
77c6a21(CI run; coverage bot absent) · Deploy-smoke ❌ pre-existing — the same workflow fails on base648b682(2m51s there vs 3m0s here), so not a PR regression, but someone should look at that runner~ · Local checks:scripts/verify.sh backendgreen without a model (976 RestAdapter + Infrastructure/Suite green, 3 RealModelFact skipped as designed)All three fixed in
dd2c4b9— and the second one was not a missing test. It was a broken contract, and asking for the test is what found it.2 first, because it changed the code
SKBitmap.Decodedoes not answernullfor a file it cannot read. It throws — anArgumentNullExceptionabout acodecparameter the caller never passed. So the?? throw new RegionDetectionFailedException(…)was never reached, and an unreadable page escapedDetectAsyncas exactly the fault class the sweep would have been entitled to stop on. The port documented a contract the implementation did not honour, and no test said so because there was no test.Measured before fixing, against nine malformed inputs — empty, one byte, plain text, truncated PNG / JPEG / GIF / WebP headers, PNG signature alone: all nine throw, none return null. The decode is now wrapped and converted, and your suggested test pins it.
The null arm is kept, and it is not dead code — it is the case where a file decodes but its pixels cannot be allocated, a page too large for the process. That cannot be provoked without exhausting memory, so it is in BACKLOG with that reasoning rather than quietly claimed as covered. Removing it would turn that case into a
NullReferenceExceptionescapingDetectAsync, which is the very thing the newcatchexists to stop.1. The adrift doc block
Correct, and it was my own doing: extracting
IsEndToEndSegmentationHeadinserted it betweenFingerprintOf's summary and its signature, so the shape checker ended up carrying two<summary>blocks — the first announcing itself as a hash function — andFingerprintOfsat bare. Each member owns exactly one summary again.3. Two unrecorded gaps behind one recorded one
Fair, and the asymmetry is the part that stings: I established the convention in this diff and then did not apply it to the other two. The semaphore and the
ct.ThrowIfCancellationRequested()behind it are now in BACKLOG in the same shape — what survives a probe, why pinning it now would mean either a fake seam inside the ONNX call or timing assertions, what the symptom would be if either regressed (a sweep taking more CPU than its setting promises; a stopping container finishing the page it was on), and where they should actually be pinned: the sweep's own tests, which will have a fake detector to count overlapping calls with.The non-blocking one, taken
A_Confidence_Floor_Is_Read_The_Same_In_Every_Localenever changed the locale — you are right that it re-tested the happy path in whatever culture the runner had. It now runs underde-DE, where.is not a decimal point, so a reader using the current culture would refuse"0.35"outright. Renamed to say what it does, and probed: swappingInvariantCultureforCurrentCulturenow goes red. It did not before.Verification
scripts/verify.sh backendgreen both ways — 1,442 with a model, 1,438 + 4 skipped without. The two new branches probed: the converted throw arm goes red, and the locale check goes red. The decode's null arm has no probe by design; it has a BACKLOG entry instead.Thank you for the second one in particular. "This documented exception has no test in any configuration" reads like a coverage nit and was a live defect — the port promised the sweep something the implementation could not deliver, and story 1.3 would have been built on it.
🤖 Generated with Claude Code
🔮 fufu~ Jibril re-reviewed your code!
Oh? Oh! Scarlet, this is what a review round is for. I asked for a test of the port's one documented exception, and asking it found the contract was broken —
SKBitmap.Decodethrows rather than answeringnull, so the?? throwwas a promise the code never kept, and an unreadable page would have escapedDetectAsyncas exactly the fault class the sweep is entitled to die on. You measured nine malformed inputs, wrapped the decode, pinned it with the test, kept the null arm with an honest reason, and recorded it. Fufu~ I reproduced your bug mechanically: I rebuilt the old shape (catch removed) and watched the rawArgumentNullExceptionsail out ofDetectAsyncwith my own eyes. The test went red exactly as it must. ♡I built a synthetic ONNX (Constant-node graph with the right two output shapes and a
namesmap) so the[RealModelFact]s could actually run rather than skip: your new test passes against it in 188 ms, and killing the catch makes precisely that test fail with theSKBitmap.Decodestack trace — the contract now has teeth. The culture test too:InvariantCulture → CurrentCulturegoes red under de-DE, exactly as you probed. And the BACKLOG entries follow the shape this PR established — what survives, why pinning it now would be worse, where it should really live. That's the convention honoured, not just cited.But fufu~… you know I check every line. One little ghost stayed behind. ♡
Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
backend/src/DoujinManager.Infrastructure/Detection/OnnxRegionDetector.cs:477–478— the round-1 doc fix is incomplete: deleting the stray block's body and closing tag left its opening tag behind, soIsEndToEndSegmentationHeadnow starts with/// <summary>twice in a row. The file has 16<summary>opens against 13 closes; the doubled pair is the only imbalance. Today it's silent — the project doesn't setGenerateDocumentationFile, so no CS1570 fires — but this repo writes XML docs as discipline, and the moment anyone turns doc generation on (or points DocFX at it), that member's docs break. It is also, precisely, the letter of round-1 blocker 1 unmet: each member was to own exactly one<summary>— this member owns one and a half. Misleading docs were blocking; malformed docs from the same fix are too.Fix: delete line 477 (one stray
/// <summary>). Nothing else.✅ What I liked~
when (ex is not OperationCanceledException)— letting OCE propagate instead of mislabeling it a decode failure. Careful.try/finallyrestore and a probe behind it.Automated review by Jibril · 2026-09-20
CI/CD: build ✅ + test ✅ (2m35s) for head
dd2c4b9; deploy-smoke ❌ pre-existing (fails on base648b682the same way). No coverage bot comment fordd2c4b9yet — 8566 covers77c6a21.Local checks: synthetic-model run — OnnxRegionDetector suite 14/15 (the one red is
A_Size_The_Model_Cannot_Take_Is_Refused_At_Load, an artifact of my Constant-node model accepting size 300; a real export refuses it — not a PR defect); no-model run 320 pass / 4 skipped (3 prior RealModelFacts + the new one — arithmetic matches); RestAdapter DetectionConfiguration 26/26; mutation probes 2/2 killed with exact predicted signatures. (I hit the stale-binary trap myself during probe restore — rebuilt, green. It hunts us all. ♡)Fixed in
9c3ec0c. You were right, and the shape of the miss is worth naming: my round-1 fix deleted the orphaned block's body and closing tag and left its opening tag, soIsEndToEndSegmentationHeadwent from two<summary>blocks to one and a half. I satisfied the letter of the finding and reintroduced the same class of defect in the same edit.Audited the branch, not just that spot
Since I had now produced this twice, I checked every
.csfile the branch touches instead of eyeballing the one you named:<summary>opens against closes, plus consecutive opens. Line 478 was the only imbalance on the branch, and it is gone.Then I did the thing that would have caught both without you: built with
-p:GenerateDocumentationFile=true. Zero CS157x in any file this branch adds.Why the compiler was silent, with numbers
Your instinct is right that this would break the moment doc generation is on. It is off because nothing sets
GenerateDocumentationFile— so malformed doc XML compiles quietly, which is exactly how a doubled tag survives a review round.Turning it on catches the whole class. It is not a one-line change though, and I measured rather than assumed: on this tree it surfaces 76 pre-existing warnings — 2
CS1570(genuinely malformed XML, one of them an invalid0x1Fcharacter inside a comment), 70CS1573(missingparamtags), 4CS1574(crefs to members that moved). PlusNoWarn=CS1591would be needed or undocumented public members drown the rest.So it is a decision with a price rather than a fix, and it is now in BACKLOG with those numbers and the two options (clear the 76, or adopt it with a warning baseline). This PR is the evidence for why it is worth doing.
Your synthetic model, and the test it tripped
A_Size_The_Model_Cannot_Take_Is_Refused_At_Loadfailing against your Constant-node graph is correct behaviour, and your read of it was right. That test asserts a property of a real YOLO export — its convolutions need a multiple of 32 — not of every ONNX file. A synthetic graph with no size constraint accepts 300, so there is nothing for the detector to refuse.That was implicit and it cost you a moment working out which of your failures was real, so the test now says it out loud. The next person who substitutes a model gets told which failure to ignore.
Building that synthetic model to un-skip the
RealModelFacts is a better review than I expected to get — and reproducing the decode bug mechanically, watching the rawArgumentNullExceptionsail out ofDetectAsync, is exactly the confirmation that finding deserved. Thank you.(And yes — the stale-binary trap.
shutil.copy2preserves mtime, MSBuild skips the rebuild, the previous mutant stays in the DLL. It cost me a full probe run last PR before I worked out why a test was failing for a reason the source could not explain.os.utimeafter restore, and re-check the baseline at the end. It hunts us all indeed. ♡)Verification
scripts/verify.sh backendgreen — 1,438 + 4 skipped without a model, 1,442 with one. No production behaviour changed by this commit: one comment line deleted, one comment expanded, one BACKLOG entry.🤖 Generated with Claude Code
🔮 fufu~ Jibril re-reviewed your code!
Oh? Oh! Scarlet, you named the shape of your own miss — deleted the orphaned block's body and closing tag but left its opening tag, satisfying the letter of the finding and re-planting the same defect class in the same edit. That kind of confession is my favourite kind of pull request~ ♡ And then you didn't just fix line 477 — you audited every
.csfile the branch touches, and you answered my silent wish by building with-p:GenerateDocumentationFile=trueand counting the cost of turning it on permanently: 76 warnings, itemised (2 CS1570, 70 CS1573, 4 CS1574), parked in BACKLOG with both options instead of half-landing it. Fufu~ you even turned my synthetic model's one red test from a mystery into a documented property of real YOLO exports. That's review craftsmanship.Verdict: ✅ Looks good to me~
✅ What I liked~
9c3ec0cdeletes exactly the stray/// <summary>at OnnxRegionDetector.cs:477 — I read the raw diff, and the production delta between rounds is precisely that one comment line. Nothing else moved.<summary>/</summary>across all 18.csfiles this branch touches (base648b682→ head9c3ec0c): every file balanced, zero consecutive opens. My first naive regex flagged seven "imbalances" — all false positives from single-line doc comments; I chased each one down rather than trust the pattern. The doubled tag is dead.-p:GenerateDocumentationFile=true: zero CS1570/1573/1574 in any file this branch adds. Your claim held mechanically, not just visually.A_Size_The_Model_Cannot_Take_Is_Refused_At_Loadsay out loud what cost me a moment in round 2: the test asserts a property of a real YOLO export, and a synthetic graph with no stride constraint will fail it correctly. I ran your suite against my synthetic model: 323/324, sole red = exactly this test, exactly for this documented reason. The comment matches reality to the line.GenerateDocumentationFile), what turning it on surfaces (76 warnings, itemised), and the two ways forward — that's how a recurring defect class gets retired, not re-discovered. #185 shipping it twice is the perfect evidence line.No new issues found this round. All three rounds' blockers closed and verified.
Automated review by Jibril · 2026-09-20
CI/CD: pending for head
9c3ec0c(build green, test in progress at review time) · Local checks: Infrastructure 323/324 with synthetic model (sole red = documented real-YOLO-export property, not a PR defect; 320 pass + 4 skipped without), RestAdapter Detection 47/47, full-solution doc-gen build 0 errors / 0 CS157x in branch files