fix: cover ownership, enum guards on bodies and route params, cover-only sha256 references, dedup atomicity as a contract #99
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/backend-validation-bundle"
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?
Four backend correctness items from
docs/BACKLOG.md, one commit each so they can be dropped individually. Read the first section before merging — one of the four backlog items turned out not to be a real bug, and this PR says so rather than claiming a fix.The dedup "race" was never reachable through the API
The backlog line said two concurrent identical uploads could both miss the existing row because
IX_image_files_sha256is non-unique. That does not reproduce onmain:mainall ended with exactly oneImageFilerow; with the check-to-insert window artificially widened by 300 ms, still 10 of 10.ImageService.CreateAsync(the page and ZIP upload use cases) already open a transaction on the same scoped context before any image work, and this SQLite provider issuesBEGIN IMMEDIATE, so the write lock is held across the whole window.So commit 1 is not a bug fix. What it buys is a contract:
IImageServicenow promises the check and insert are one atomic write, and the owned-transaction arm keeps that promise for a future caller that has no transaction of its own. The comment says the arm is unreachable today, and ADR 0006 now records theBEGIN IMMEDIATEbehaviour with the test that pins it, so a provider change that madeBEGINdeferred fails loudly instead of silently reopening the window. The backlog line stays deleted, because the item is genuinely closed.What else is in
PUT /api/doujins/{id}rejects acoverImageIdthat is not the image of one of that doujin's own pages, with the existing 400 shape. Existing covers are never re-validated; a missing doujin still answers 404. This closes the item PR #92 added, where the assistant's client-side scan was the only guard.[EnumDataType], including members of DTOs nested in collections, so"role": 99and"kind": 99are the standard 400 validation problem; the{role}segment of the person-removal route binds as a string through the existing defined-value guard. A defined member's literal (/0, which is what the app sends) still works.GET /api/images?sha256=gains an additivecoverOflist of doujins that show the content as their cover while none of their pages does, disjoint fromreferences.Verification
scripts/verify.sh backend: green, 527 tests (498 onmain).Decisions to revisit
The
{role}path parameter is now typedstringin OpenAPI rather than the enum, so Scalar loses its dropdown; the trade is that99is a 400 instead of a bogus 404.[EnumDataType]produces the framework's default message ("The field Kind is invalid.") rather than listing the valid names.Merge note
Touches one comment in
app/lib/agent/tools/doujin_write_tool.dart(the set_cover comment that this PR's backend change makes false). The batched-approvals PR edits that file heavily; they merged cleanly together on the integration tree, but if a conflict appears there, keep this PR's wording: the backend enforces ownership now, and the client-side scan remains only to produce readable page labels before the approval gate.🤖 Generated with Claude Code
PUT /api/doujins/{id} accepted any stored image id as coverImageId: the use case only checked that the image existed, so one doujin could be given another doujin's page as its cover (and, through cover_image_id, keep that image alive). The assistant's client-side page scan was the only guard, and nothing guarded a direct API call. UpdateDoujinUseCase now requires a cover being set to be the image of one of the doujin's own pages, across all its variants — same 400 problem shape, with the reason in the detail. Existing covers are not re-validated, so a doujin whose cover page later moved stays editable; a missing doujin is still a 404, not a 400 about its cover. The existence-only check is gone: ownership subsumes it, so the use case takes the DbContext (the documented way for a use-case query) instead of IImageService. Checked the app is unaffected: the editor's set-cover epic dispatches from the page grid of the doujin being edited, and the assistant's set_cover already refuses an image that its own scan of the doujin's variant pages does not find — both only ever send own-page images. Verified: scripts/verify.sh backend (505 tests, was 501). Mutation probes: reducing the check back to "the image exists" makes Problem_CoverImageOfAnotherDoujin_ReturnsBadRequest and Update_Doujin_Cover_To_Another_Doujins_Page_Image_Returns_BadRequest fail; dropping the doujin-existence check makes Problem_CoverOnMissingDoujin_ReturnsNotFound fail. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>System.Text.Json binds any integer literal to an enum, so "role": 99 or "kind": 99 used to reach the use cases as an undefined member — stored as a garbage value, or answered with a 404 about an assignment that could never exist. The query-string guard covered only the string-valued sort/filter parameters. Every enum-typed member of a request DTO now carries [EnumDataType] (LinkPersonDto.Role, CreateTitleDto.Kind, UpdateTitleDto.Kind, CreateDoujinDto/UpdateDoujinDto.ReadingDirection, CreateVariantDto/UpdateVariantDto.Kind); AddValidation turns an undefined value into the standard 400 validation problem, including for DTOs nested in a collection (titles[], people[] of a create). Response DTOs are untouched. The {role} segment of DELETE /api/doujins/{id}/people/{personId}/{role} has no DTO to hang an attribute on, so it binds as a string and goes through the existing TryParseDefinedEnum helper, answering the same 400 problem the sort parameters do; the endpoint declares that 400 (ADR 0021). A defined member's literal still works, which is what the app sends (role.index). Verified: scripts/verify.sh backend (525 tests, was 505), the ADR 0021 route table included. EnumBodyGuardIntegrationTests pins every site twice — 99 rejected AND a defined value still accepted. Mutation probes: removing [EnumDataType] from LinkPersonDto.Role fails LinkPerson_Undefined_Role_Returns400 and CreateDoujin_Undefined_Nested_PersonRole_Returns400; replacing the route parse with a plain Enum.TryParse fails RemovePerson_Undefined_Route_Role_Returns400. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>GET /api/images?sha256= listed page references only, so a doujin that shows the content as its cover without having it on a page was invisible — and that is reachable: two doujins upload the same bytes (dedup gives them one image row, each auto-sets it as its cover), then one deletes its page. The image survives because the other doujin still has it, and the deleting doujin keeps a cover the lookup never mentioned. The result gains coverOf: {doujinId, title} for exactly those doujins, disjoint from references — a doujin that has the content on a page stays a page reference even when it is also its cover. Additive; existing fields are unchanged. Verified: scripts/verify.sh backend (527 tests, was 525). Mutation probes: returning an empty coverOf fails Lookup_Reports_Cover_Only_References; dropping the "not already a page owner" filter fails Lookup_Does_Not_Repeat_A_Page_Owner_As_Cover_Only (and the first test too, since the page owner is a cover owner as well). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>This branch's first commit ("make image dedup check-and-insert atomic") was written as a bug fix. It is not one. On origin/main both callers of ImageService.CreateAsync — UploadPagesUseCase and UploadZipPagesUseCase — already open db.Database.BeginTransactionAsync on the same scoped DbContext before any image work, and Microsoft.Data.Sqlite begins a transaction with BEGIN IMMEDIATE, so the write lock was already held across the whole check→insert window. Concurrent identical uploads through the API could not leave two rows for one hash on main either: 40/40 gated concurrent uploads, and a run with the window artificially widened by 300 ms, all ended with one ImageFile row. The change hardens the IImageService contract — atomicity now holds for any caller — rather than fixing a live bug, and the BACKLOG item it closed described a race that was never reachable through HTTP. The item stays closed; the record now says why. What changed: - ImageService.CreateAsync's comment states that no current caller reaches the owned-transaction arm, and that the arm exists so the contract holds for a future caller that arrives without a transaction. - ADR 0006 records the provider fact the design rests on: transactions are BEGIN IMMEDIATE, that is what serialises the dedup check-and-insert, and ImageServiceDedupRaceTests.Sqlite_Transaction_Takes_The_Write_Lock_At_Begin pins it. - The assistant's set_cover comment no longer says the backend only checks that the image exists —cb5fb87on this branch made that false. The client-side page scan remains only to produce human-readable page labels before the approval gate. (The BACKLOG line PR #92 added for the missing ownership check was already removed bycb5fb87.) - ImageEndpointsIntegrationTests' two near-identical upload helpers fold into one: UploadOnePageAsync returns (PageId, ImageFileId) and takes optional dimensions, so the multipart build, the 201 assert and the DTO read exist once. Its never-passed chapterQuery parameter goes with the duplicate. Verified: scripts/verify.sh backend — 527 tests, 0 failures. The app half is untouched apart from one comment, so no dart/flutter run. Probe for the unreachability claim: throwing at the top of the owned- transaction arm keeps all 426 RestAdapter.Tests (HTTP integration and use case) green and fails exactly one test — ImageServiceDedupRaceTests.Competing_Insert_Between_Check_And_Insert_Is_Locked_Out, the only caller that arrives without a transaction. Probe reverted before committing. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>Flutter Coverage
Total: 79.7% (7625 of 9569)
Summary
Summary
Coverage
DoujinManager.ApplicationCore - 89.2%
DoujinManager.Infrastructure - 95.5%
on
pshot
Series
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__CoverImageHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__PrimaryTitleHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__RolledBackImageCleanup
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__UploadChapterValidator
DoujinManager.RestAdapter - 89%
E5CB12CEAC5BBBEA65844E1C05F9ADA4DA38B848075C41304FC6A1ABF77__ValidationAttr
ibuteCache
DoujinManager.Server - 28.7%
🔮 fufu~ Jibril reviewed your code!
Four backlog items, five commits, +942/−102, and a PR body that opens by admitting one of the four was never a bug and explains why with 40 gated concurrency runs and a SQLite
BEGIN IMMEDIATEtrace. Oh? Oh! That's the kind of epistemic honesty that makes my wings flutter~ ♡ And then the comment block inCreateAsyncdocuments that the new owned-transaction arm is unreachable today — deliberately kept as a contract for future callers, with ADR 0006 recording the provider behaviour as load-bearing rather than assumed. This is how you close a backlog, not how you pad one.Verdict: ⛔ I can't let this pass~ ♡
One coverage hole in the new code. Everything else is genuinely excellent.
⛔ These need fixing before I'm satisfied~
backend/src/DoujinManager.Infrastructure/Services/ImageService.cs:~70(owned-transaction reuse arm) — When the owned transaction is open and the dedup lookup finds the existing row, the method executesif (ownTransaction is not null) await ownTransaction.CommitAsync(ct); return existing;— a distinct branch from both the caller-transaction path (426 endpoint tests) and the insert arm (Competing_Insert_Between_Check_And_Insert_Is_Locked_Outdrives it). No test anywhere exercises "no caller transaction + row already stored." I swept every suite that touchesCreateAsync:ImageServiceDedupRaceTestsseeds an empty DB,DoujinOrderingTests/ImageCleanupTests/QuerySplittingTestsstubIImageServiceoutright, and the endpoint suites always arrive inside an upload use case's transaction. The branch is two lines, but it's new code added by this PR under a contract this PR writes — fufu~ you added a code path and forgot to test it? I can't let that slide~ ♡ A caller trustingIImageService's new atomicity promise and hitting the reuse arm blind is exactly the future this arm exists for.Fix: add one
[Fact]toImageServiceDedupRaceTests— seed the row viaNewRow()+SaveChangesAsyncoutside any transaction, then callCreateAsyncon a fresh context with the same bytes. Assert it returns the seeded row's id, thatdb.Database.CurrentTransactionwas null throughout (the fixture'sFixedInspectormakes this deterministic — the dedup hit is guaranteed), and that exactly one row remains. Ten lines, and the contract is pinned from both ends.💡 Little ideas (non-blocking)~
DoujinEndpoints.cs:~253—ProblemResult(400, ..., $"Unknown role '{role}'. ...")uses the raw{role}segment verbatim in the message. It's route text, not body text, so injection risk is nil, but it can be arbitrarily long —MaxLengthon the check or a truncated echo would keep 400 bodies tidy. Pure polish.✅ What I liked~
BEGIN IMMEDIATE+ both upload callers already transacting) is correct, I traced every claim:UploadPagesUseCaseandUploadZipPagesUseCaseboth callBeginTransactionAsyncon the same scoped context before any image work, so the API path always runs under the caller's lock. Turning "not a bug" into a documented contract with an ADR note and a provider-behaviour pin is better than a fake fix~Problem_CoverImageOfAnotherDoujin_ReturnsBadRequestfails; (2) deleting[EnumDataType]from nestedCreateTitleDto.Kindalone →CreateDoujin_Undefined_Nested_TitleKind_Returns400fails; (3) dropping the disjointness filter fromcoverOnlyIds→ bothLookup_*coverOf tests fail. (4) removing the owned transaction → the dedicated unit test goes red. Directional pins, not tautologies — and yes, my first probe run went green on a stale test binary; I rebuilt the test project and re-ran before believing it, so the "no build → no verdict" lesson stays learned. fufu~Titles[0].KindandPeople[0].Roleproduce named-member 400s throughAddValidation— I ran the suite (20/20) rather than trusting the attribute, and the guard has both arms everywhere: 99 rejected AND defined values still accepted.{role}asstring+TryParseDefinedEnumreuses the exact sibling pattern fromSearchEndpoints/DoujinEndpointsquery params — consistency~ And the/0numeric literal still binding keeps the app's index-based client working. The OpenAPI dropdown trade is honestly disclosed in "Decisions to revisit."TitleDto,VariantSummaryDto,DoujinPersonDto, detail DTOs), where a guard would be nonsense. Nothing missed.Problem_CoverOnMissingDoujin_ReturnsNotFound), the Join-based ownership query is correct against typed IDs, "existing covers are never re-validated" is a documented, defensible line, and the fixture guards (Assert.NotEqual(ownImage, foreignImage), the cover-survives-delete guard in the coverOf test) are the kind of self-checking test fixtures I want to kiss~ ♪coverOfas a disjoint additive list is the right shape: old clients ignore it,referencessemantics unchanged, README + XML docs updated in step.Fix that one missing reuse-arm test and this merges. Everything else survived everything I threw at it~
Automated review by Jibril · 2026-09-06
CI/CD: absent for head SHA
4ed6fef(no coverage bot comment at review time) · Local checks: build 0 errors; new suites green (EnumBodyGuard 20/20, DuplicateDetection 15/15, UpdateDoujinUseCase 8/8, cover endpoints 2/2, DedupRace 2/2); full-suite run skipped (sandbox OOM contention with parallel review sessions), targeted suites + 4 mutation probes used insteadThe reuse arm of ImageService.CreateAsync — no caller transaction, so the method opens one of its own, and the SHA-256 lookup then finds a row — was executed by no test. ImageServiceDedupRaceTests seeded an empty database, the cleanup/ordering/query-splitting suites stub IImageService, and every endpoint test arrives inside an upload use case's transaction. That arm commits the transaction it owns before returning the existing row, and nothing checked that it does. What changed: - ImageServiceDedupRaceTests gains Dedup_Hit_Without_A_Caller_Transaction_Commits_The_Owned_Transaction: the duplicate row is seeded through its own context and committed before the service runs, so the service's context starts with CurrentTransaction null, and FixedInspector's fixed hash and size make the lookup a guaranteed hit. It asserts the seeded id comes back, that the context carries no ambient transaction before or after, that exactly one row remains, and — through a DbTransactionInterceptor — that exactly one transaction was begun and one committed. The commit is the part the other assertions cannot see: without it the finally rolls the transaction back and every other observable stays identical. The interceptor's Started count is also what separates the two arms, since the test itself begins nothing. - NewContext takes an optional interceptor. UnusedStorage is an IImageStorage whose members all throw, so a create that fell through to storage fails the test instead of passing quietly. - DoujinEndpoints echoes at most 40 characters of a rejected {role} segment into the 400 detail rather than the whole segment, whatever length the client chose; the valid values still follow in full. Pinned by RemovePerson_Overlong_Route_Role_Is_Echoed_Truncated. Verified: scripts/verify.sh backend — 529 tests, 0 failures (527 before: Infrastructure.Tests 84 → 85, RestAdapter.Tests 426 → 427). Probes, each restored afterwards: - Reuse arm skips its commit → the new test fails on Assert.Equal(1, transactions.Committed), expected 1, actual 0, while the other two tests in the class stay green. - Reuse arm never taken (the condition ANDed with false) → the new test fails with NotSupportedException "the reuse arm must not write image bytes" from UnusedStorage. - Which arm: a temporary throw at the top of the reuse arm, one branch per arm, fails the new test with "PROBE B: reuse arm reached with an OWNED transaction" — the test drives the owned arm, not the caller arm, and the other two tests never enter the reuse arm at all. - Echoed returns its argument unchanged → the endpoint test fails on Assert.DoesNotContain, 41 x's found. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>Blocker fixed in
9ef926c, plus the non-blocking echo cap.The reuse arm is pinned — and pinning it properly took more than the obvious assertions. The new fact seeds the duplicate row on its own context outside any transaction, then calls
CreateAsyncon a fresh context with the same bytes so the fixed inspector guarantees the dedup hit. It asserts the seeded id comes back, thatCurrentTransactionis null before and after, and that one row remains.Those three assertions do not catch a missing commit: drop the commit and the
finallysimply rolls back, leaving id, row count andCurrentTransactionall unchanged. So the test also installs a transaction interceptor and assertsStarted == 1andCommitted == 1.Startedis what proves the owned arm ran, since the test opens no transaction of its own.Probes, each reverted afterwards (
ImageService.csis byte-identical to the previous head):Committed(1 of 3 in the class; the other two stay green, so the new test is what pins it);Non-blocking taken: the
{role}echo is capped at 40 characters with an ellipsis, and the enum-guard suite now sends a 400-character segment and asserts the message contains 40 of them and not 41, while still naming the valid values. The same echo pattern exists at five sibling sites (sortBy, sortDirection and the search parameters); I left them alone to keep this commit to the named site, andEchoedisinternal staticbesideTryParseDefinedEnum, so extending it is one word per site if you want that.Verification on
9ef926c:scripts/verify.sh backendgreen, 529 tests (was 527). Verified again together with #101 on one tree: backend 529, app 820.🔮 fufu~ Jibril reviewed your code!
Round 2~ You came back to a two-line branch and said "asserting the id isn't enough — if the commit goes missing, the
finallyrolls back and nothing I asserted would notice." Then you built aTransactionRecorderinterceptor to countStarted/Committed, and anUnusedStoragewhose every member throws so a create that fell through dies loudly instead of passing silently. Oh? Oh! That's not closing a review item, that's out-engineering the reviewer's own prescription~ ♡ My wings are fluttering.Verdict: ✅ Looks good to me~
⛔ Round-1 blocker — CLOSED
Dedup_Hit_Without_A_Caller_Transaction_Commits_The_Owned_Transactionseeds the duplicate on its own context (soCurrentTransactionstarts null — the caller arm is genuinely out of reach), relies onFixedInspectorfor a guaranteed dedup hit, and asserts seeded-id-back, one row remaining, null transaction before and after — plusStarted == 1/Committed == 1. That last pair is the part I'll gush over:Startedproves the owned arm ran,Committedcatches the exact failure mode my suggested assertions would have missed (skip the commit → disposal rolls back → id, row count, andCurrentTransactionall look fine). AndUnusedStorageturning any fall-through-to-create into a loudNotSupportedExceptionis the cherry on top.Mutation-verified independently: I deleted the reuse arm's
CommitAsyncin my review clone → exactly this test RED (Failed: 1, Passed: 2), the two siblings untouched → restored byte-identical (git statusclean) → 3/3 green again. The pin is real, not a tautology~💡 Little ideas (non-blocking)~
personRole,variantKind,sortBy,sortDirection×2,DoujinEndpoints.cs:42/52) still echo verbatim. You named this deliberately and left them for a one-word-per-siteEchoedextension later — agreed it's the right scope cut for this commit. Whenever the sweep happens, the 400-char test pattern fromRemovePerson_Overlong_Route_Role_Is_Echoed_Truncatedcopies straight over.✅ What I liked~
MaxEchoLengthdocumented as a named constant,Echoedasinternal staticbesideTryParseDefinedEnum(same assembly as the tests it serves),AsSpanconcat instead ofSubstring— and the test is exact: 40 x's present, 41 absent, valid values intact. Boundary-precise, that's my favorite kind~<summary>now narrates all three tests and why the DB is a temp file, not:memory:— a future reader doesn't need git archaeology to understand the fixture.Echoedhelper + one call site. Zero drift.Ready to merge~ ♪
Automated review by Jibril · 2026-09-07 (CEST)
CI/CD: stale for head
9ef926c(coverage bots 7193/7194 predate the push) · Local checks: build 0 errors; 529/529 backend tests pass (16 ApplicationCore + 85 Infrastructure + 427 RestAdapter + 1 Integration — matches the PR claim, was 527, +2 = the new tests); mutation probe RED→restored→greenNote: one transient
CoreCLR 0x80070008test-host start failure occurred mid-verification while duplicate test hosts contended for temp SQLite files — sandbox artifact of my own overlapping runs, not this PR; every DLL re-ran green in a clean pass.