docs: twelve open items for the backlog #107
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "docs/pile-a-backlog"
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
Twelve open items, collected in
docs/BACKLOG.mdrather than written into each of the seven "Pile A" correctness fixes (#108–#114) — otherwise all seven conflict on this one file. This should merge first, so nothing is untracked while they land.(#N)follows the repo's existing convention: the PR that deferred the item, not a record of what that PR did.Backend
is_defaulthas no filtered unique index, so ADR 0004's exactly-one-default invariant is enforced only inVariantService; two concurrent deletes can leave two defaults (#111)page_countdisagrees with their rows are not repaired; wants a startup or admin-endpoint sweep — deliberately not a background service (#113)page_countcan still be lost-updated across twoDbContexts (#113)UpdatedAtsubtree rule (#114)/health503 writes nothing to the log (#112)BackupEndpointsadvertisesself=/api/backups/{fileName}, which answers 405 — appended to the existing entry (#110)App
Page's doc comment names aPageDtothat no longer exists; needs abuild_runnerrun in the same commit (#109)/health503 unhelpfully (#112)Assistant
It also corrects an existing App entry: "Metadata writes trigger a full five-endpoint refetch" is not true of plain UI metadata CRUD, which patches Redux state from the write response. Only the assistant's
_refreshAfterWriteEpicrefetches, so that half moved to the Assistant section and the App entry is now just "No app-side disk image cache."Verification
Documentation only — one file, no code, no tests, no behaviour change.
The seven fix branches were each verified with the full
scripts/verify.sh(backend build and tests, app codegen, analyze and tests) before these entries were split out; removing a markdown file from those commits cannot affect that result. Merging this plus the seven in the order below was tested locally: seven of eight apply cleanly, and #114 conflicts by design.Follow-ups
Recommended merge order. This PR first, then in any order:
/healthtells a mounted volume from a writable onePageCountfrom page rowsand #114 last — it rewrites
PageService.csandVariantService.cs, which #111 and #113 also touch, and amends ADR 0004 alongside both. Merging it last means one rebase instead of two.Two of the fixes stop future drift without repairing existing damage — hence the two repair items above. Worth querying the live database before deciding how urgent they are: doujins with zero default variants, and variants whose
page_countdisagrees with their row count.🤖 Generated with Claude Code
The seven Pile A branches each defer work that AGENTS.md says belongs in docs/BACKLOG.md. Collecting those entries here instead of in each branch keeps the seven code PRs from all conflicting on this one file; this PR should merge first, so nothing sits untracked while the fixes land. Nine entries: the Dart doc comment left stale by the page DTO consolidation; the missing database backstop for the one-default-variant invariant; the one-shot repair sweep for already-drifted PageCount values; the assistant's five-collection refetch now that GET /{id} exists; the BackupEndpoints self link that answers 405 for the same reason the metadata routes did; the metadata-rename gap in the UpdatedAt subtree rule; the silent log on a 503 readiness verdict; the two owners of the backup directory path; the three remaining in-place file writers; and the Settings connection test's unhelpful message for the new 503. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>60fe649daetoa997e6c38bdocs: record the follow-ups deferred by the Pile A correctness fixesto docs: twelve open items for the backlog🔮 fufu~ Jibril reviewed your code!
Oh? Oh! A documentation-only PR that arrives before the seven fixes it tracks, with a merge-order plan in the body? scarlet, you know how to make a knowledge-hungry Flugel happy~ ♪
But fufu~ don't think a markdown diff escapes my gaze. A backlog entry that lies about the code is worse than no entry at all — so I verified all nine claims. Not against your PR body. Against the trees.
Verdict: ✅ Looks good to me~
🔍 What I verified (all nine, against base
98973aband the seven fix branches)is_defaulthas no backstop — confirmed: noHasIndexinvolvingIsDefaultanywhere in the DbContext or migrations. And your race analysis is precise: onfix/default-variant-invariant, promotion and delete share oneSaveChanges,Variantcarries no concurrency tokens, and nothing serialises two concurrent deletes — two defaults really is reachable. (Delicious finding, by the way ♡)self→ 405 — confirmed at base:BuildBackupLinksadvertises/api/backups/{fileName}, the only route on/{fileName}isMapDelete, so a GET answers 405 exactly as written.fix/metadata-get-by-idreally does add the fiveGET /{id:guid}routes (tags/people/circles/characters/series).fix/page-count-integrityaddsReconcilePageCountAsyncon create and delete; base still increments/decrements. ZeroIHostedServiceinbackend/src— your "should not grow one for this" lands._refreshAfterWriteEpicdispatchesLoadMetadataAction(force: true), whichFuture.waits all five collection fetches after every approved write. Splitting the old bullet into surgical-refetch entry + separate disk-cache entry was the right call — they're independent items./health503 silence — confirmed:HealthEndpointson the fix branch contains no logging call at all, andProgram.cs:21really doesbuilder.Logging.ClearProviders(), so your capturing-logger-test caveat is accurate too.BackupServicere-readsDOUJIN_MANAGER_BACKUP_DIRper call whileProgram.csresolves it once at startup intoReadinessRegistration'sDataDirectoriessingleton. Two owners, exactly as described.memory_store.write,writeUserSkill,writeManifestSyncForTest) docreateSync+writeAsStringSync.flushgains write-then-rename onfix/reading-progress-atomic-write;writeManifestalready had it at base — so "now share" is true.DioHealthRepository'sbadResponsearm throwsServer returned HTTP $code; only transport failures are special-cased.Pagedoc comment — confirmed: onfix/page-dto-media-type, variant detail'sPagesprojection switched toPageMapper.ToDto, which carriesSizeBytes/MediaType— so "only in page-detail responses" is now untrue;PageDtois really gone fromDoujinDtos.cs; and freezed really copies the doc comments intodoujin_models.freezed.dart(found them at :1910-1911) — thebuild_runner-in-the-same-commit constraint is the correct reason for deferring.Cross-checks too: all seven fix branches share merge-base
98973ab(= current main), none of them touchesdocs/BACKLOG.md(so no conflicts with this PR), and the merge-last rationale forfix/touch-parent-on-child-writeis real — it, item 5 and item 6 all rewritePageService.cs+VariantService.cs.💡 Little ideas (non-blocking)~
filesystem_offline_store.dart:79), but I can find no production caller: every call site ofwriteManifestSyncForTestlives underapp/test/, and startup (main.dart) only ever reads the manifest. When the atomicity PR lands, correct the parenthetical (and the stale source comment) — or scope it test-only, whichever matches reality. The actionable part (atomic write) is unaffected either way~(#N)= the deferring PR. These nine can't cite numbers yet since the fix PRs don't exist, but when they open, remember the tags so the tracker stays self-describing ♪✅ What I liked~
SaveChangessemantics,ClearProvidersvs a capturing-logger test, freezed copying doc comments). This is what a backlog is for.Automated review by Jibril · 2026-09-08
CI/CD: absent for head
60fe649(PR just opened; docs-only diff) · Local checks: no build needed for markdown — all nine claims verified against base98973aband all seven fix branches instead🔮 fufu~ Jibril reviewed your code!
Oh? Oh! Twelve little debt-confessions, carefully sorted and cross-referenced against seven not-yet-merged fix PRs~ A backlog that documents what each fix deliberately leaves behind — that's rare discipline, and I couldn't resist checking whether every confession matches the actual code. So I did. All of them. ♪
Verdict: ✅ Looks good to me~
Documentation only —
docs/BACKLOG.md(+49/−2, one file, one commit on top of currentmain, merge-base = base). No code, no tests to demand, no behavior to probe. So the review became: does every entry tell the truth about the tree it describes? Fufu~ I fetched all seven sibling fix branches and verified against bothmainand each branch head.⛔ These need fixing before I'm satisfied~
None! Not one. But don't relax yet~ ♡
💡 Little ideas (non-blocking)~
FilesystemOfflineStore.writeManifestSyncForTestis "(used for startup seeding, not only tests)". Hmm~ I searched the whole tree at this SHA: zero production callers — every call site lives inapp/test/(14 hits, all test files), startup only reads the manifest (main.dart:70→OfflineLibraryRequestedAction→readManifest()), andgit log -Sshows no production caller ever existed since #84 introduced the method (the doc comment "for tests and startup seeding" appears to have been aspirational even then). The claim inflates the blast radius of the in-place write — as a test-only writer it's still worth the atomic-write treatment for CI robustness, but "startup seeding" is not a reason. Drop the parenthetical (or the whole third item if you'd rather keep the entry strictly production-impact-scoped) — and note the method's own doc comment atfilesystem_offline_store.dart:79repeats the same untruth, so the backlog entry is currently documenting the stale comment rather than reality.✅ What I liked~
main+ the seven fix branches, all green: the backupself→ 405 claim (onlyDELETE /{fileName}exists on that route;BuildBackupLinksadvertisesself=/api/backups/{fileName}—HypermediaHelpers.cs:352), the missing filtered unique index onis_default(VariantConfigurationhas only{DoujinId, LanguageCode}), the two concurrent-default-promotions race (#111 branch adds promote-on-delete but no index/transaction — exactly what the entry says it leaves open), the no-sweep left-open by #113 (branch addsReconcilePageCountAsyncbut noIHostedService),PageDtogenuinely deleted on the #109 branch whilePage's doc comment still names it, the fiveGET /{id:guid}routes genuinely absent onmainand added by #110, the three in-place file writers (memory_store.dart:47,skill_registry.dart:90,writeManifestSyncForTest), the two backup-dir sources (BackupService.cs:39reads env per call; readiness usesProgram.cs:112's startup resolution), and — my favorite — the correction of the existing App entry: entity CRUD epics really do patch Redux from the write response (EntityUpdatedAction<T>(await ops.update(...))), only_refreshAfterWriteEpicforce-refetches all five collections. Splitting that stale claim into an accurate App entry + a new Assistant entry was the right fix, not just an edit~(#N)convention clarified up front ("the PR that deferred the item, not a record of what that PR did") — without that sentence I'd have misread a third of these entries as already-done. Cheap sentence, saves every future reader.merge-treeat this head confirms #114 conflicts with this branch as predicted. Fufu~ you even verified your own logistics.Merge this first, as the body says — the seven fixes behind it are clean to land in any order after. Nothing here blocks, and the one wobbly parenthetical is a two-word deletion. ♡
Automated review by Jibril · 2026-09-08
CI/CD: absent for head SHA
a997e6c(no coverage bot comment; docs-only diff — coverage irrelevant) · Local checks: skipped (docs-only; 13 claim verifications via git grep/diff against main + seven fix branch heads)a997e6c38bto625c74b1d1Flutter Coverage
Total: 81.7% (8012 of 9812)
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! The twelve grew to forty~ Two new commits, twenty-eight new debt-confessions covering five Phase-1 branches, plus a whole entry about testing readiness under uid 0 — which, fufu~, is an entry about my sandbox's siblings~ ♪ You know a backlog curated like this is irresistible to me. So I did what I did last time: verified the claims. Not against your PR body — against
mainatdfaf89cand all six referenced branch heads (search-key-fold, openapi-snapshot, scale-indexes, library-integrity-report, deflake-search-and-backup, health-probe-test-under-root — all sharing merge-basedfaf89c, exactly as the entries imply).And the rebase! The original twelve, tags updated to the now-merged
(#108)–(#114), every spot-check I ran still holds at the new base (backupself→ 405, fiveGET /{id:guid}routes now on main, the three in-place writers —memory_store.dart:45-47stillcreateSync+writeAsStringSync, two backup-dir sources,IHostedServicestill absent). Faithful~ ♡Verdict: ⛔ I can't let this pass~ ♡
One entry — just one — describes a world that no longer exists. But it's the load-bearing clause of its entry, so it doesn't get to hide among the true things.
⛔ These need fixing before I'm satisfied~
openapi_contract_test.dartshould surface is "Page(built fromPageDto, which has nomediaType) being fed tooffline_download_service._extensionOf, which switches onmediaType." Fufu~ I traced this end-to-end at this PR's own base, and it's false: (a)PageDtodoes not exist atdfaf89c— #109 deleted it, and your own file says so at line 198–200 ("Page's doc comment names aPageDtothat no longer exists"); the two entries contradict each other. (b) The wire shape isPageDetailDto, which hasmediaType— confirmed in the pinned snapshot. (c) The endpoint that feeds offline downloads isGET /api/variants/{id}→GetVariantUseCase→VariantService.GetByIdAsync, which eagerly loads.Include(v => v.Pages).ThenInclude(p => p.ImageFile)(VariantService.cs:29-30), soPageMapper.ToDtoprojects a real media type, not the zero/empty fallback its null-tolerance would give a lazy caller. (d) App-side,_$PageFromJsonreadsjson['mediaType'] as String? ?? ''— so_extensionOf(page)receives the true media type and picks the true extension. The drift you describe was real before #109 merged — at this head it is a phantom, and a contributor writing the contract test expecting this red will find green and doubt the whole fixture. The claim was stale the moment the rebase folded #109 into your base~ Fix: drop the "and the drift it should surface first is …" clause (the entry stands perfectly well without an example), or replace it with a drift that actually exists at this head. A backlog entry that lies about the tree is worse than no entry — that was the standard last round, and it hasn't changed ♡💡 Little ideas (non-blocking)~
writeManifestSyncForTestis "(used for startup seeding, not only tests)". At the new base: 13 call sites, all underapp/test/, plus the definition; zero production callers; startup (main.dart) only reads. Second round I've asked~ When #108's atomic-write follow-up lands, delete two words — and note the method's own doc comment (filesystem_offline_store.dart:79, "for tests and startup seeding") repeats the untruth.✅ What I liked~
app/lib/data/models/: 8,745, to the line. "0 of 90 schemas carry one", "no operation carries anoperationId" — I parsed the snapshot: 90 schemas, 0 descriptions, 0/80 operationIds, 0 summaries,securitySchemesabsent. "90 paths→54,/versionabsent" — confirmed, inline atServer/Program.cs:165, and ADR 0021:35 even names it "the only one left". Numbers that survive an adversarial Flugel~ ♪GlobalExceptionMiddlewareanswers 413 (:43-45) andCreateTagUseCasereturnsServiceResult.Conflictcarrying409throughProblemResult(ServiceResult.cs:15→DoujinEndpoints.cs:422), while both routes declare only 400/401. That's a contract gap you can only find by reading both layers. Delicious~EnforcedFileModesDataAttributegenuinely probes enforcement instead of trustinggeteuid(),/dev/ptsheads the candidate list with/sysright behind it, and the EROFS-vs-EACCES reasoning matches the test's loud rejection of non-EACCES refusals. A backlog that audited itself before arriving. ♡IX_doujins_ratingis added, but theRating == nullleading sort term (DoujinOrdering.cs:33-34, kept on the branch with a comment saying exactly why) means no index onratingcan serve the ASC arm — the trap is real, and so is the no-NULLS-LAST-in-EF premise. ZeroPRAGMA/journal_mode/ANALYZEanywhere,BatchSize = 500, the two-hex shard as sweep cursor,ImageCleanupHelperswallowing intoLogWarning— all verified.One phantom clause in twenty-eight entries. Fix those two lines and this merges — the other 27+12 tell the truth, and prettily~
Automated review by Jibril · 2026-09-09
CI/CD: absent for head
625c74b(docs-only diff, coverage irrelevant) · Local checks: skipped (docs-only) — ~40 claim verifications via git grep/diff/show against maindfaf89cand all six fix-branch heads625c74b1d178e0d406f4🔮 fufu~ Jibril reviewed your code!
Oh my~ back so soon? I checked, of course I checked: this head is the same file, byte for byte —
git diff 625c74b..78e0d40touches only the five test/doc files that rode in on your new base. You rebased ontoe0ac19dbecause #117 merged, and your three commits came along cleanly. Nothing new was written; the backlog simply stands on newer ground~ ♪And that new ground matters! Your uid-0 entry describes exactly the tests #117 just landed on
main— so I re-verified every one of its claims against the merged tree, not the branch: all six named test cases exist inHealthEndpointIntegrationTests.cs, the fourLosesWritePermissionrows sit underEnforcedFileModesDatawithpinnedElsewhereBywired through,ProbeEnforcementreally does a live write into a mode-dropped directory (and the doc comment says "never fromgeteuid()" — honest to a fault~),/dev/ptsheads the candidate list with the sysfs entries behind it and theEROFS-vs-EACCESreasoning spelled out at :187-205, and the "running as uid 1654 un-skips them" note matches the deployment. An entry about tests that were still on a branch yesterday, true againstmaintoday. That's the discipline I adore~ ♡Verdict: ⛔ I can't let this pass~ ♡
Fufu~ you knew this was coming. The file is identical, so my round-2 blocker is identical too — still sitting at lines 232–234, still describing a world that died when #109 merged into your base.
⛔ These need fixing before I'm satisfied~
openapi_contract_test.dartshould surface is "Page(built fromPageDto, which has nomediaType) being fed tooffline_download_service._extensionOf". I re-traced it at your new basee0ac19d, and it is still false, on every axis: (a)PageDtohas zero occurrences in the entire backend at this base — #109 deleted it, and your own line 198–200 says so; the two entries contradict each other in one file. (b) The wire type isPageDetailDto, which carriesmediaType. (c)VariantService.GetByIdAsynceagerly loads.Include(v => v.Pages).ThenInclude(p => p.ImageFile)(VariantService.cs:29-30), so the mapper projects a real media type. (d) App-side,doujin_models.g.dart:228readsjson['mediaType'] as String? ?? ''andoffline_download_service.dart:161switches on it — the true extension is chosen. A contributor who writes the contract test expecting this red will find green and distrust the whole fixture. The claim was stale before the rebase; the rebase carried the staleness forward verbatim~ Fix: delete the "and the drift it should surface first is …" clause (the entry stands without an example), or name a drift that exists at this head. A backlog that lies about the tree is worse than no backlog — that was the standard in round 2, and a rebase doesn't soften it ♡💡 Little ideas (non-blocking)~
writeManifestSyncForTestis "(used for startup seeding, not only tests)". At this head: 13 call sites plus the definition — every single one underapp/test/; startup only reads the manifest. Two words. The method's own doc comment (filesystem_offline_store.dart) repeats the same untruth — correct both when #108's atomic-write follow-up touches this code, or just delete the parenthetical now since you're editing the file anyway~✅ What I liked~
main— every named test, the probe-not-geteuid decision, the/dev/ptsordering, the EROFS rejection-all analysis. Backlog entries usually rot when their branch merges; this one ripened.40 claim verifications from round 2 carry over byte-for-byte, because the bytes are byte-for-byteTwo lines to delete in a 309-line file, and this merges. I've said it twice; the third time I'm not smiling quite as wide~ ♡
Automated review by Jibril · 2026-09-10
CI/CD: absent for head
78e0d40(docs-only diff, coverage irrelevant; bot comment 7392 predates this push) · Local checks: skipped (docs-only) — re-verified at new basee0ac19d: PageDto absence, VariantService includes, app mediaType path, 13 test-only callers; uid-0 entry claims against merged #117Summary
Summary
Coverage
DoujinManager.ApplicationCore - 90.9%
DoujinManager.Infrastructure - 96.4%
on
pshot
Series
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__CoverImageHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__PrimaryTitleHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__RolledBackImageCleanup
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__UploadChapterValidator
DoujinManager.RestAdapter - 92.5%
E5CB12CEAC5BBBEA65844E1C05F9ADA4DA38B848075C41304FC6A1ABF77__ValidationAttr
ibuteCache
DoujinManager.Server - 49.6%
Both fixed in
a10d294;docs/BACKLOG.mdis still the only file touched.1. The contract-test entry (blocker). Deleted the "and the drift it should surface first is …" clause; the entry now ends at "…the follow-up the snapshot exists to enable." Re-checked at this base before editing rather than taking it on your word:
grep -rnw PageDto backend/→ 0 hits.PageDetailDto(RestAdapter/Dtos/ImageDtos.cs:28), last field[property: JsonPropertyName("mediaType")] string MediaType.VariantService.GetByIdAsync(Infrastructure/Services/VariantService.cs:27) does.Include(v => v.Pages).ThenInclude(p => p.ImageFile), soPageMapper.ToDtoemitsimg?.MediaTyperather than the empty fallback.doujin_models.g.dart:228readsmediaType: json['mediaType'] as String? ?? '', consumed by_extensionOf(offline_download_service.dart:160-176).And the file contradicted itself — the entry at lines 198-200 already records that
PageDtois gone.2. The
writeManifestSyncForTestparenthetical. Deleted, on the third ask. The counts, confirmed here: 14 occurrences of the name underapp/— the definition atfilesystem_offline_store.dart:80plus 13 call sites, every one underapp/test/(offline_store_test.dart9,offline_epics_test.dart3,offline_reader_test.dart1).git log -S"writeManifestSyncForTest" --name-only -- app/lib/returns a single commit —42e8272, #84 — touching a single file, the definition. A production caller never existed; startup only reads the manifest.3. The same untrue sentence is in the method's own doc comment (
/// Synchronous manifest write for tests and startup seeding.). That is a code change to a file already onmain, so it is out of scope for a docs-only PR — it now has its own one-line entry in the App section instead of being fixed silently here.Also added an entry for the
IHasSearchKeyrefactor deferred out of #118 (replacingSearchKeyBackfill's hand-maintained entity list with a model-driven loop).The branch was rebased onto
e0ac19dearlier, which is why the file was byte-identical last round — that moved it onto merged #117 but did nothing about your finding, which was fair to call out.Verification: markdown only. Nothing compiles and no test project is affected; CI has no markdown lint. The verification that mattered is the source evidence above, all re-run at this base.
🔮 fufu~ Jibril reviewed your code!
Oh my~ the fourth dance, and this time you brought receipts~ One new commit,
a10d294, and its message reads like my own round-3 comment folded back into prose: the phantomPageDtodrift traced end-to-end, the 13 test-only call sites, even the "expecting red would get green" verdict. Fufu~ being quoted is one thing; being obeyed is quite another ♡Verdict: ✅ Looks good to me~
🔍 What I verified this round
The round-2/3 blocker — gone, cleanly. The contract-test entry (now lines 231–236) ends at "is the follow-up the snapshot exists to enable." — full stop, no drift example. I diffed
78e0d40..a10d294: exactlydocs/BACKLOG.md, +8/−6, two hunks, zero scope creep. The entry no longer contradicts its own #109 line, and a contributor writingopenapi_contract_test.dartis no longer promised a red that would come back green.The new entry tells the truth, line by line.
writeManifestSyncForTest's doc comment atfilesystem_offline_store.dart:79does say "for tests and startup seeding" — quoted verbatim in the entry. The call-site count: 3 inoffline_epics_test.dart+ 1 inoffline_reader_test.dart+ 9 inoffline_store_test.dart= 13, all underapp/test/. Andgit log -S writeManifestSyncForTest -- app/lib/returns exactly one commit — #84, the one that defined it. "A production one never existed" is not rhetoric; it's archaeology. Routing the code-side fix through a backlog line instead of smuggling a code change into a docs PR — correct instinct~The ground moved, and the file didn't flinch.
mainadvancede0ac19d → b713dbbwhile you committed (#122, backup newest-first) — so I re-checked every backup-adjacent entry against the new tree: the flaky-test entry (lines 291–302) already speaks of the unstable sort in the past tense ("was real and is fixed"),Create_Multiple_Backups_All_Listed_NewestFirststill exists (theTask.Delay(1100)gone, as the entry narrates), and the "no other test touchingDOUJIN_MANAGER_BACKUP_DIRin parallel" claim survives #122's newBackupNewestFirstTests— that class never touches the env var, and the other class that does (HealthEndpointIntegrationTests) sits inProgramHostCollectionwithDisableParallelization = true. Nothing stale, nothing contradicted.⛔ These need fixing before I'm satisfied~
None~ ♪ First round with an empty list, and I checked hard before writing that word.
✅ What I liked~
PageDetailDto, the eagerInclude, the generatedfromJson). That's the level of proof I demand, self-applied.Merge it. Then the forty can start shrinking one fix at a time ♡
Automated review by Jibril · 2026-09-10
CI/CD: stale for
a10d294(coverage bot 7447 predates the push; docs-only change, coverage irrelevant) · Local checks: skipped — markdown-only diff