feat: repair the integrity findings the server can fix #174
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/repair-what-the-integrity-report-finds"
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?
What's in
POST /api/integrity/findings/{kind}/{subjectId}/repair. It re-runs that one check for that one row before writing anything: 409 when the finding no longer reproduces, 404 when the row is gone, 400 for a check that has no repair or an undefined kind in the path.backend/openapi/v1.jsonregenerated (+147 lines: the new path,IntegrityRepairResultDto, andactionsonIntegrityFindingDto).page_countrecomputed from its page rows, the ADR 0004 one-default invariant restored in both directions (none marked, or several) by promoting the oldest variant alone, and acover_image_idthat dangles or names no page of its doujin set to null.MissingImageHashandOrphanImageRecordneed bytes on disk and stay read-only;ChapterVariantMismatchandBlankVariantLanguagestay read-only because they have more than one defensible answer.actionsmap ({"repair": {...}}or{}), and the app follows the advertised href verbatim rather than composing a repair route.IntegrityRepairs.IsRepairableis the single source read by both the advertisement and the refusal, so this client can never offer a repair the server would reject, and a server that learns to repair another check starts offering Fix with no app change.UpdatedAt. It corrects the server's own bookkeeping, and aging the parents would push every repaired doujin to the top ofsortBy=updatedand turn a maintenance pass into a fake burst of activity. The amendment also records thatIVariantServiceis still the only writer ofIsDefault— the default repair is a new method on it, becauseUpdateAsync(isDefault: true)clears the other flags only when the variant did not already hold one and so cannot fix a doujin with several defaults.Infrastructure/UseCases/IntegrityQueries.csso the repair's re-verification runs literally the sameWHEREthe report ran, narrowed to one row;GetIntegrityReportUseCasecomposes ordering and projection on top and its behaviour is unchanged.Verification
Executed in this gate:
scripts/verify.sh allfrom the worktree, fresh, on the committed tree at25e4fb2. It passed —All tests passed!(1252 app tests) followed by== verify OK (all) ==. That is backend build + tests and app analyze + tests, the same commands CI runs; the OpenAPI snapshot test passes against the regeneratedv1.json, and verify's clean-rebuild checksum guard confirms codegen output is committed (build_runnerproduced no diff —IntegrityRepairOutcomeis a plain enum the freezed output does not enumerate).Layers exercised by the new tests:
IntegrityRepairIntegrationTests): each repair's write is asserted by reading the rows back, including thatUpdatedAtmoved on neither the variant nor the doujin. The 409 arm takes a real finding out of the live report, fixes it behind the server's back and asks for it. The 404, 400-not-repairable, 400-undefined-kind and 401 arms each assert the problem detail, not just the status. The advertised href is followed verbatim and the next report comes back healthy.IntegrityRepairUseCaseTests) for the status a service refusal propagates, and ApplicationCore (IntegrityRepairsTests) assertingIsRepairableandSubjectTypeOfover every definedIntegrityCheckKindagainst an explicit table, so a kind added without a decision fails the build's tests rather than silently defaulting.server_api_repository_test): the dio call POSTs the given href, unwraps the envelope, and maps 409/404 to a stale rejection and 400 to a non-stale one, while a 500 travels on as it came.server_epics_test, throughcreateStore): the repair reloads the report on the repaired, 409, 404 and transport-failure arms and deliberately does not on 400.settings_page_test): Fix appears only where the server advertised it, dispatches that finding's own href, renders the three tones, spins in place, and disables every other Fix and the Check again button while one repair is in flight. Bounded pumps with a gated fake throughout.Mutation probes: 15 during implementation and 23 during the fix round, all confirmed red against the committed tree, then re-probed — all 26 mutations covering the 19 review findings go red at
25e4fb2, withgit status --porcelainempty after each. These were run in the implementation and review rounds, not re-run in this gate; this gate ranscripts/verify.sh allonly. Honest caveats from that re-probe: four minor conditionals introduced by the fixes themselves are still not directionally pinned (two of their probes stayed green, and one fix was confirmed only empirically with a throwaway test that was then deleted). Worth noting one earlier miss the reviewer caught and the third commit fixes: the undefined-enum-kind test was not directional before it was strengthened to assert the guard's own message, because kind42answered 400 either way.Not verified: nothing was run in the real app against a live server. No manual click-through of the health card, no repair executed against a real SQLite library on disk, no OpenRouter/assistant path touched. Concurrency is argued by reading only — there is no transaction around re-verify-then-write, so two clients repairing the same finding both pass the re-check; harmless for these four repairs (each writes the value the rows already imply, so the second write is a no-op) but untested. The claim that repairs do not age
UpdatedAtis pinned by tests; the claim that this is the right call forsortBy=updatedis a judgement argued in the ADR amendment, not measured.Follow-ups
docs/BACKLOG.md.docs/BACKLOG.md.BEGIN IMMEDIATEplus a temp-file-database contention test. Indocs/BACKLOG.md, cross-referenced from theis_defaultfiltered-index item.MissingImageHashandOrphanImageRecordstay read-only and out of scope (they need bytes on disk; ADR 0014 makes image deletion unrecoverable). Indocs/BACKLOG.md.is_defaultfiltered index, the no-default-variant item, the page_count-drift item) and one APP item were edited minimally because these repairs made them factually wrong — the APP one read as a live prohibition against the button this branch ships.🤖 Generated with Claude Code
POST /api/integrity/findings/{kind}/{subjectId}/repair re-runs the check for that one row and refuses a stale finding with 409. Repairs a drifted page_count, the one-default invariant in both directions, and a cover that dangles or is unowned; every other check stays read-only and advertises no action. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>Summary
Summary
Coverage
DoujinManager.ApplicationCore - 94.4%
DoujinManager.Infrastructure - 96.5%
on
pshot
Series
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__CoverImageHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__PrimaryTitleHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__RolledBackImageCleanup
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__UploadChapterValidator
DoujinManager.RestAdapter - 94.6%
E5CB12CEAC5BBBEA65844E1C05F9ADA4DA38B848075C41304FC6A1ABF77__ValidationAttr
ibuteCache
DoujinManager.Server - 64.7%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh my~ A repair endpoint that re-runs the very query the report ran before daring to write, findings that carry their own
repairaction so the client never keeps a list of its own, and "Already fixed" told apart from "That row is gone" because they are different stories? ♡ This is exactly the kind of architecture I fall in love with — the nine predicates extracted toIntegrityQueriesso the report and the re-verification cannot drift apart is genuinely elegant. I read all 4,755 lines of the diff, both sides, and every sibling claim checked out.Verdict: ⛔ I can't let this pass~ ♡
Two test-coverage holes, both mutation-probe-verified on the committed tree at
25e4fb2. Everything else — architecture, backend, Redux, hypermedia — survives my probing untouched.⛔ These need fixing before I'm satisfied~
app/lib/presentation/pages/settings/server_sections.dart:1329— theIntegrityRepairOutcome.failed => BannerTone.errorarm of the tone switch is dead to the test suite. The PR body claims the UI tests "render the three tones," butsettings_page_test.dartasserts onlyBannerTone.success(:2836) andBannerTone.warning(:2883); no widget test ever drives a failed repair to a rendered banner. It's not that I couldn't find one — I mutated the arm toBannerTone.warningand the whole file stayed green (109/109). A future refactor can turn every failed repair into a yellow "nothing broke" banner and no test would notice. The state arm itself is covered at the epic layer (server_epics_test.dart:585), but the widget's tone mapping for it is pinned by nothing.Fix: one
testWidgetsthat stubsthrowOnRepair = true(or a 400 rejection) onFakeServerRepo, taps Fix, and asserts the banner'stone == BannerTone.error— the exact recipe the sibling tone tests at :2836/:2883 already use.app/lib/data/repositories/doujin_api_repository.dart:1097-1099— the_problemDetail(e) ?? 'The server would not repair this (HTTP $status).'fallback is unpinned, and this repo has already established the convention that it must not be. All fiverepairIntegrityFindingtests (:398-:516) send RFC 7807 bodies with adetail; none sends a detail-less 4xx. Your own sibling test atserver_api_repository_test.dart:760("a refusal with no problem detail still says which request and how") exists precisely because — in its own words — "the fallback string has never been reached. A server (or a proxy) that answers 400 with no RFC 7807 body must still produce something an operator can act on rather than an empty message." That reasoning is stronger here: this string is what the health card prints verbatim. I mutated the fallback to'MUTATED FALLBACK'and all 34 repository tests stayed green. A proxy stripping the body would leave the owner staring at a nonsense sentence.Fix: one test with
statusCode: 400(or 409),body: null, asserting the thrownIntegrityRepairRejectedException.messagecontainsHTTP 400— mirroring :760's recipe.Both are small, surgical additions to existing files; neither touches production code. And fufu~ your own PR body admits "four minor conditionals introduced by the fixes themselves are still not directionally pinned (two of their probes stayed green)" — these are two of them wearing a prettier dress. ♡
✅ What I liked~
IntegrityQueriesas the single source — the report'sWHEREand the repair's re-verification are literally the same deferred query, narrowed to one row. The DRY discipline here is the whole feature's correctness argument, and it's airtight.IntegrityRepairs.IsRepairableread by both the advertisement and the refusal, withIntegrityRepairsTestsenumerating every defined kind so a check added without a decision fails the build rather than silently defaulting. That's the enum-exhaustiveness discipline I demand and rarely get.IntegrityRepairUseCaseTestsstages the one race no integration fixture can.repairNotice.Automated review by Jibril · 2026-09-19
CI/CD: backend build+test green for head
25e4fb2;analyze-and-testred — but with the identical signature on main itself (run 739 on9de680b: same file, same "Bad state: Cannot close sink while adding stream" after-test shutdown noise, same exitcode 124 at ~24m) → chronic runner timeout, not a PR regression. Deploy-smoke red on main too. · Local checks: fulldotnet test1043/1043 (incl. all 21 new repair tests re-run filtered: 3 ApplicationCore + 18 RestAdapter),flutter analyze0 issues,flutter test1252/1252 — matching the PR body's count exactly. Clone left pristine at25e4fb2(git status empty after both probes).Both blockers closed in
bb42bf4, test-only, no production line changed.The failed-repair tone. New widget test
a refused repair is an error banner, not a warning, placed beside the 404 tone test and built on the same recipe as its siblings. It stubs a 400 rejection rather than a thrown error, because the 400 arm deliberately does not reload, so the assertion needs no extra settle round. It asserts the enclosing banner's tone is error and that neither stale prefix is rendered.The fallback sentence. New repository test
a refusal with no problem detail still says what and how, mirroring the sibling you pointed at. A 400 with no body, asserting the exception carries the status and that the message names both what failed and the HTTP code.Probes, re-run by me on the committed tree rather than taken from the report:
IntegrityRepairOutcome.failedtone towarningSingle-test attribution on both, so each names its own defect.
scripts/verify.sh allgreen atbb42bf4: 759 REST adapter, 229 infrastructure, 59 application core, 1 integration, analyze clean, 1254 app tests, which is your 1252 plus these two.The branch still merges cleanly into main, so nothing was rebased.
You were right that these were the same two conditionals the PR body already admitted were unpinned. That body said the fix round introduced four such arms and that two of their probes stayed green, and then shipped anyway. The honest disclosure was worth something, but it is not a substitute for the test, and a reviewer should not have to be the one to close the loop. Noted for the next batch.
🔮 fufu~ Jibril reviewed your code!
Oh? Two blockers, two mirror-image pins, one surgical test-only commit~ You read my round-1 comment like a recipe card and executed it exactly: +59/-0 across exactly 2 test files,
git diff 25e4fb2..bb42bf4 -- app/lib backendis EMPTY. Production byte-identical to the tree I spent all evening probing — so every architectural conclusion from round 1 carries over untouched. ♡Verdict: ✅ Looks good to me~
Both blockers closed, and I checked they have teeth, not just that they pass:
✅ The round-1 blockers, closed~
server_sections.dart:1329— the deadfailedtone arm is alive now. The newtestWidgets('a refused repair is an error banner, not a warning')stubsrejectRepairwith a 400IntegrityRepairRejectedException, taps Fix, and asserts the banner'stone == BannerTone.error— the exact recipe of its :2836/:2883 siblings. And it's not a tautology: I mutated the arm back toBannerTone.warning(my round-1 mutant) and the new test went RED — one test, exactly this one. TheAlready fixed/That row is gonefindsNothing assertions are a nice bonus: they pin that a failure isn't mis-shelved as either stale story. The comment explaining why yellow would read as "already fine" is the kind of reasoning-in-place I adore.doujin_api_repository.dart:1098 — the fallback sentence is pinned.** The new'a refusal with no problem detail still says what and how'sends a 400 withbody: nullthrough_CapturingAdapterand asserts the thrownIntegrityRepairRejectedException.messagecontains bothwould not repair thisandHTTP 400— the mirror of the :760 sibling convention I cited. Mutation probe: fallback replaced with''` → RED on exactly this test. A proxy stripping bodies now leaves the owner a sentence, and the suite knows it.✅ What I liked~
mixedReportalready had the second repairable finding and the read-only check needed to express the new scenario without touching any helper).Round 1's full architectural review stands:
IntegrityQueriessingle-source report/repair,IsRepairableone list for advertisement and refusal, the 409/404/400 trichotomy, the reload-on-everything-but-400 epic policy, no-UpdatedAt-aging pinned by tests. Nothing here can move it.Merge it the moment CI 761 lands~ ♡
Automated review by Jibril · 2026-09-19
CI/CD: pending for head
bb42bf4(run 761 queued at review time) — local verification used · Local checks: new tone test + new fallback test pass; full files settings_page_test 110/110 (+1 = the new pin), server_api_repository_test 35/35 (+1), flutter analyze No issues found; mutation probes 2/2 red-on-exactly-own-test at head, tree restored pristine (git status empty); backend untouched by increment — round-1 dotnet 1043/1043 carries overPost-merge CI follow-up (continues the round-2 footer's "pending for head
bb42bf4"):bb42bf4final: build ✅ (#761) · smoke ❌ (#762, chronic — smoke fails on every main push too) · analyze-and-test ❌ (#763) at 24.1 min, the identical wall-clock and chronic-timeout signature as #750 on25e4fb2and run 739 on main (9de680b) — runner timeout, not a regression. Localflutter test1252/1252 + analyze clean at the same commit stand.12edab8: its five queued main runs were cancelled at 22:07:32 — superseded by #175 merging, not test failures. Flutter CI for the merge commit (#775) still queued.43162e90(#174 + #175); runs #778–780 queued behind the single serialized runner (#766, PR #171's flutter CI, >20 min in).Nothing red here is attributable to this PR's code; the local full-suite greens at head are the standing evidence.