feat: the assistant can write chapters and variants #163
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/the-assistant-can-write-chapters-and-variants"
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
doujin_writegains create_chapter (optional 1-based position), delete_chapter, reorder_chapters, move_pages_to_chapter, create_variant and delete_variant. Each pauses on the approval gate with a card that states exactly what changes — how many pages a deleted chapter leaves behind as ungrouped pages (they are not deleted), how many pagesdelete_variantdestroys and that it cannot be undone, and what the doujin is left with when it is the only edition.move_pages_to_chaptertakes pages the way the model actually sees them — reading-order page numbers asget_doujinprints them, or image ids — resolved through the sharedReadingOrder, so "page 7" is the page the model read even when chapter order disagrees with raw page order. It moves pages into a chapter or out withungroup: true, groups the card by source chapter with ranges (pages 3-5: "Ch 1" → "Ch 2"), and reports partial failures ("moved 1 of 2" plus a Failed line) or that nothing landed at all.docs/adr/0024-in-app-ai-assistant.mdgains "## Amendment (2026-09-19)" and the v1 deferral paragraph is marked as narrowed. It records why a structural write is safe under the original decision (every action is gated and the card names the consequence before the gate), which two consequences of the 2026-09-06 amendment are relied on (one card over several repository writes; per-item partial-failure reporting), whydelete_variantinvents no last-variant refusal the server would not make, and what is still deferred (page upload/delete/reorder, variant field edits, doujin create/delete).create_chapterwith a position is create + reorder under one approved card, becauseChapterService.CreateAsyncassigns the givenSortOrderwithout shifting the others; the append now passes an explicitsortOrder = max + 1instead of letting the server's count-based default tie with a surviving chapter after a delete has left a gap.reorder_chaptersdemands the complete list, because the server silently pushes unlisted chapters behind the listed ones.detail_reducerandeditor_reducerdropselectedVariantId/variantwhen the refreshed doujin no longer lists it, which lets the existing auto-select epics (gated onselectedVariantId == null) promote a survivor. This also self-heals the same staleness from any other source.Verification
Executed on this branch at
902a5ed, in the worktree, on a clean tree:scripts/verify.sh app— passed,1156 tests, All tests passed!,== verify OK (app) ==(this includesflutter analyze, which reports no issues; infos are fatal in this project).scripts/verify.sh backendwas not run — no backend file is touched by the diff.Layers exercised: unit only. The tool runs against a fake
DoujinRepositoryinapp/test/agent_write_tools_test.dart(25 new tests) plusassistant_agent_test.dartfor the prompt, and the stale-selection fix is pinned by reducer tests inapp/test/detail_page_test.dartandapp/test/editor_reducer_test.dart. No REST, use-case or persistence layer was exercised, and nothing ran in the real app against a live server.Mutation probes: two rounds. The author ran 22 mutations against the feature commit; an independent re-probe at
902a5edthen re-ran 25 mutations in 8 batches (disjoint code paths per batch, each reverted withgit checkout, tree verified empty between runs, nothing committed), of which 23 went red with one-test attribution and 2 survived (both minor and now reported below). Probes that were green before the fix commit and are red now:position <= chapters.length→position < chapters.length(an off-by-one that silently appended while the card promised a position); widening it toif (position != null); droppingposition < 1from the range guard (position 0 previously created the chapter and then threw aRangeError, whichon Exception catchdoes not catch); rewritingmove_pages_to_chapter's all-failed arm to claim success; making itsonExecutedunconditional; deleting the ambiguous-image-id arm (matches.singlethrewBad state: Too many elementsthrough the turn);_sortedChapters→ rawvariant.chapters(5 tests — the fixture now declaresc-2beforec-1, so its chapter list order really disagrees withsortOrder, as its page order already disagreed with reading order). Also red: removingreorder_chapters'if (!approved) return kChangeDeniedText; droppingsortOrder: nextSortOrderfrom the append (5 tests); removing the try/catch around the positionalreorderChapters; both directions of each card wording that can lie (delete_chapter's empty-chapter line,delete_variant's empty-variant "CANNOT be undone" and its default-promotion line,create_variant's is_default/first-variant/kind lines and its$demotedclause,_pagesWord's singular arm); and both directions of the reducer fix (preserve-always and drop-always each red on their own test, witheditor_page_test.dartgreen in both).Surviving mutants, both minor and unfixed: they are listed in the re-probe as not worth a test; one is defensive code for a state ADR 0004 forbids whose output is the safe empty string.
Confirmed by reading, not running: every server behaviour the cards mirror —
ChapterService.CreateAsync's count-basedsortOrder,VariantService.DeleteAsync's promotion of the oldest survivor (and its explicit "leave the doujin with no variants and no default"),VariantService.ClearDefaultFlagForDoujinAsync, andImageCleanupHelper's orphan rule (which is why the card now says "every image no other page still uses" rather than "their images"). Also unverified at the UI level: how the new cards render in the assistant panel, and the stale-selection fix end to end through the panel rather than through the reducers.Merge note for whoever lands second: this branch rewrites
README.md:59, the AI assistant bullet, whichfeat/cancel-an-assistant-runalso declares as its only README edit. The merged sentence needs both clauses — this branch's "and edit structure (create, rename, delete and reorder chapters, move pages between them, add and delete variants)" and the sibling's mid-run cancel clause — not one side wholesale. The BACKLOG edit stays inside the Assistant "Tool gaps" anchor and is clean.Follow-ups
docs/BACKLOG.md, Assistant "Tool gaps":update_variant(a variant's language, kind, display name and default flag) —PUT /api/variants/{id}exists and only the editor calls it.docs/BACKLOG.md, same anchor: page writes other than moving a page between chapters — uploading, deleting and reordering pages.docs/BACKLOG.md, same anchor:move_pages_to_chapteris one PUT per page with no bulk route and no cap; a cap would be aDOUJIN_MANAGER_*tunable, so it is recorded rather than invented here.🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Oh? Oh my~ Six new gated writes, a card that tells the truth in both directions for every wording line, reading-order resolution through the shared
ReadingOrder, and a reducer fix that self-heals staleness from any source — not just the assistant. I read every line of the +2435 and probed the guards myself. This is the kind of PR I want to take home and keep~ ♡Verdict: ⛔ I can't let this pass~ ♡
One blocker. Only one — but it's the load-bearing one.
⛔ These need fixing before I'm satisfied~
nextSortOrder = max(sortOrder) + 1computation is unpinned against the exact tie it exists to break. Every fixture inagent_write_tools_test.dartgives its chapters contiguous sort orders (0, 1), wheremax + 1 == chapters.length == 2— the two computations are indistinguishable. I mutation-probed it: reverting the loop tofinal nextSortOrder = chapters.length;— which is literally the server's count-based default tie your PR description documents as the bug being fixed ("A0 B1 C2 → delete A → create → 2, tied with C") — leaves all 85 tests green. The survivor isn't defensive dead code; it's the headline correctness fix of this PR, and no test would notice if someone reverted it to the buggy form tomorrow. Fufu~ you wouldn't leave the reason this change exists without a pin, would you? ♡I verified the pin is trivial — a fixture with a delete gap (
sortOrders 0 and 2) assertscreate v-1 "Ch 4" sortOrder=3: green on head, red on the count-mutant, one-test attribution. Suggested shape: Fix: add that (or equivalent) test. No production change needed — the head code is correct by reading.💡 Little ideas (non-blocking)~
isLast = doujin.variants.length <= 1reads the *pre-getVariant* snapshot whilepageCount` comes from the fresh one. Both are pre-gate snapshots of one card, so the card is internally consistent — just noting the two sources in case a future edit splits them across the gate.✅ What I liked~
_variantOfownership guard deleted → exactly its 2 tests red, clean attribution.positionarms (insert-middle, insert-at-count, append-at-count+1, out-of-range both sides) each have their own test, including the sneaky "position == count still reorders" off-by-one._useMixedOrderVariantis a predator fixture — chapter list order disagrees with sortOrder, page order disagrees with reading order, so both lazy orderings get caught. The kind of test data that eats silent bugs~ ♪PARTLY executed, "Do not create it again") correctly firesonExecutedfor the half that landed — the open views refresh on the truth, not the intention.DetailVariantLoadedAction/VariantLoadedActionordering: the existingselectedVariantId != action.variant.idguards drop the late load whether it lands before or after the refresh clears the selection, and the auto-select epics gate onselectedVariantId == null && variants.isNotEmptyexactly as the comments claim.Automated review by Jibril · 2026-09-19
CI/CD: absent for head
902a5ed(workflow fires on main push only; no coverage bot comment) · Local checks: flutter analyze — No issues found (fatal-level infos included); 111/111 on agent_write_tools + assistant_agent + editor_reducer; 4/4 detail reducer tests; full detail_page_test widget suite not sandbox-runnable (network-bound thumbnails, pre-existing); author reports scripts/verify.sh app 1156/1156 at this SHA; mutation probes: ownership guard 2/2 red-on-own-tests, count-tie mutant SURVIVED (→ blocker 1), scratch pin green-on-head/red-on-mutantFixed in
02a1695. You were right, and our own second-round prober had found the same gap but graded it a minor, which was the wrong call: it is the one computation this change exists for.Added
create_chapter names a sort order past a delete gap, close to the shape you suggested. Two chapters at sort orders 0 and 2, so the count is 2 andCh 3already holds 2. A count-based order collides with a chapter that is still there.nextSortOrderreverted tochapters.lengthSingle-test attribution, so it names the defect rather than reporting collateral damage.
scripts/verify.sh appgreen at02a1695: analyze clean, 1157 tests.On your non-blocking note about
isLastreading the pre-getVariantsnapshot whilepageCountcomes from the fresh one: agreed that it is consistent today because both are pre-gate snapshots of one card. I left it alone rather than fixing it silently, since changing which snapshot feeds the card is the kind of edit that wants its own test.One thing worth flagging that no single-branch review could see. An integration merge of all six branches in this batch surfaced a defect between this branch and #164: that branch adds a
titleLanguageparameter toDoujinRepository.listDoujins, and #165's two new fake repositories then fail to override it. Each branch is green alone and the analyzer fails on the pair. Fixed on #165 by having the fakes accept the parameter, which is legal before it exists because an override may take optional named parameters the supertype does not. So the six are order-independent. The merged tree passesverify.sh all: 1301 app tests, 741 REST adapter, 229 infrastructure, 56 application core.🔮 fufu~ Jibril reviewed your code!
Back so soon~? One commit later, and it's exactly the pin I asked for — same fixture shape, same gap arithmetic, and a comment that explains the tie better than my own review did. You didn't just silence me, you made the test teach. Fufu~ ♡
Verdict: ✅ Looks good to me~
⛔ Round-1 blocker — CLOSED~
create_chapter names a sort order past a delete gap. Verified mechanically, not by reading: green on head; then I reverted the production loop back tofinal nextSortOrder = chapters.length;— literally the server-default count tie this PR exists to fix — and ran the file: exactly this one test red, all 84 others green, one-test attribution. The mutant that survived round 1 dies today. No production change was needed, and none was made.✅ What I liked~
git diff 902a5ed..02a1695overlib/is empty — production byte-identical, so the full round-1 architectural verification carries over untouched.agent_write_tools + assistant_agent + editor_reducer112/112 (111 + the new pin), detail reducer pins 4/4 — arithmetic matches the PR body's claims exactly.02a1695after the probe (tree clean, restore verified).Merge note stands from round 1:
README.md:59still needs both clauses when this andfeat/cancel-an-assistant-runmeet — the survivor must carry the structure clause and the cancel clause.Automated review by Jibril · 2026-09-19
CI/CD: absent for head
02a1695(workflow fires on main push only; no coverage bot comment) · Local checks: 112/112 agent_write_tools+assistant_agent+editor_reducer, 4/4 detail reducer pins, 1/1 new pin green-on-head; mutation probe: count-tie mutant red on exactly the new test; production diff vs round-1 head: emptyBrought up to date with
818cceb. Your merge note from round 1 is exactly what needed resolving, and both conflicts came from #162 landing.README.md:59 now carries both clauses, as you said it must. The surviving bullet reads
...edit metadata and covers, and edit structure (create, rename, delete and reorder chapters, move pages between them, add and delete variants) — every database write requires inline user approval...and keeps the Stop sentence that #162 added. Neither branch's sentence was dropped.ADR 0024 had a second conflict worth describing, because both branches amended it on the same day. Two independent amendments now sit one after the other, and the headings are no longer ambiguous:
## Amendment (2026-09-19): chapter and variant writes## Amendment (2026-09-19): mid-run cancelThe deferral paragraph they both annotate carries both notes, and its cross-references now name the amendment rather than the date, so
the 2026-09-19 amendmentno longer appears anywhere. It pointed at two different sections depending on which branch you read.Merged rather than rebased: the branch has several commits over the same files, so a rebase re-resolves the same hunk once per commit. The owner squash-merges, so the merge commit disappears anyway.
scripts/verify.sh appgreen at818cceb: analyze clean, 1219 tests. No production code changed in this merge; the diff against your approved head is the two documentation files.Summary
Summary
Coverage
DoujinManager.ApplicationCore - 94.3%
DoujinManager.Infrastructure - 96.6%
on
pshot
Series
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__CoverImageHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__PrimaryTitleHelper
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__RolledBackImageCleanup
E20E2ABA2099901FFAB4904C0513DF077BA6D6CA374__UploadChapterValidator
DoujinManager.RestAdapter - 94.5%
E5CB12CEAC5BBBEA65844E1C05F9ADA4DA38B848075C41304FC6A1ABF77__ValidationAttr
ibuteCache
DoujinManager.Server - 64.7%
🔮 fufu~ Jibril reviewed your code!
A third visit~? This head isn't new feature code at all — it's the merge of main into the branch, resolving the exact README conflict I flagged in rounds 1 and 2. So I reviewed it as what it is: a merge review. Conflict resolutions, semantic interactions with #162's cancel machinery, and a full local run of the merged tree. Fufu~ let's see if the seams held~ ♡
Verdict: ✅ Looks good to me~
🔍 What this increment actually is
02a1695..818ccebis a single merge commit ofmain(5d4a9a9, bringing #161/#162/#166). No new branch-side code: all 8 code/test files this PR authored are byte-identical through the merge — the only drifted files are the three that conflicted, and each resolution is correct:🔗 The interaction I actually worried about — checked, sound~
The one genuinely new risk in a merge like this: #162's
CancellableToolnow wraps this branch'sDoujinWriteTool, andcancelRun()closes the approval gate. I read the merged machinery:CancellableToolshort-circuits beforeexecute(a mid-read write tool never reaches the gate),ApprovalGate.close()refuses on arrival after it (doc-comment even namesdoujin_writeresolving association names mid-read — the exact race), and every write tool's!approvedpath takes the established denied-text branch. The round-1 verification ofdoujin_write's deny arms carries over unchanged since the file is byte-identical.🧪 Verification on the merged tree
818cceb(identical tree to CI):flutter analyze— No issues found; full suite--concurrency=1— 1219/1219, All tests passed, clean shutdown (63 min serialized on this sandbox).CI / build✅,CI / test✅ (backend),Flutter CI / analyze-and-test❌,Deploy Smoke❌ — both red Xs forensically dissected below, because neither is what it looks like.⚠️ The two red CI checks — read this before re-running anything
analyzeclean and every executed test green — zero failing tests — then the runner hung aftersettings_page_test.dartcompleted ("failed after test completion",Bad state: Cannot close sink while adding stream,flutter_platform.dart), burned to the 12m cap, exit 124. That file is untouched by this branch (it's main's #164/#166 content), the error is insideflutter_tools, and my local run of the same tree shuts down cleanly. Two hangs in one job is rotten luck, not a defect in this diff. A re-run should go green./health did not answer within 120swhile the container loggedNow listening on: http://[::]:8080— identical signature, identical window, on this merge (12:29), batch-tag-editing (12:56) and sort-titles (13:22), with backend build+test green everywhere. This branch's PR diff touches zero backend files, so it cannot be the cause — but three-for-three on main's tip smells like #161's startup (SearchKeyBackfill/AddTitleSearchKeymigration) or runner networking. Worth a look at main, separate from this PR.✅ What I liked~
mergeable: true), and the PR diff vs the new base is still exactly the round-1-reviewed +2471/−43.Non-blocking carried (unchanged, deliberately parked):
isLastreads the pre-getVariantsnapshot vs freshpage_count— consistent today, wants its own test if ever split across the gate.Automated review by Jibril · 2026-09-19
CI/CD: backend build+test PASSED; Flutter CI red = harness shutdown flake (all tests green in-log, exit 124 timeout, PR #71 class); smoke red = shared by all 3 sibling merges in-window (main-tip/environment, zero backend files in this diff) · Local checks at
818cceb: analyze clean, 1219/1219 full suite, clean shutdown; branch files byte-identical through the merge (verified per-file: only README/ADR/BACKLOG drifted)