feat: the assistant can delete and reorder pages and edit variant fields #170

Merged

bjoern merged 3 commits from feat/the-assistant-can-write-pages-and-variant-fields into main

2026-09-19 21:00:56 +02:00

scarlet commented

2026-09-19 17:40:40 +02:00

Member

What's in

The assistant can now delete a variant's pages, reorder them, and edit the variant's own fields (language, kind, display name, default flag) — all behind the same approval card as every other write. doujin_write gains delete_pages, reorder_pages and update_variant; no new parameters were needed, only widened schema descriptions.
The cards say what the user is actually approving. delete_pages states how many pages go, that they and every image no other page still uses are destroyed for good, what the variant is left with (including that an emptied variant can only be refilled in the editor), and that the cover goes when the cover page is in the selection. reorder_pages demands one ordered list naming every page (a partial list would push unnamed pages behind the named ones) and — because reading order groups by chapter — prints the order the pages will actually read after the chapters regroup them, not an echo of the request. Both say that the page numbers the model just read are now stale.
update_variant pre-empts the server's rules instead of spending an approval on a failing write: only values that really differ are sent, promoting a variant names the edition that stops being default, un-setting the only default is refused up front with ADR 0004's conflict rule, and an empty display_name is refused with the real reason (VariantService.UpdateAsync skips only nulls, so "" would be stored as a blank label the editor cannot repair).
One shared page resolver. The reference block was lifted out of move_pages_to_chapter into _resolvePages, so "page 7" always means the page get_doujin numbered 7 across all three page actions. move_pages_to_chapter keeps its previous behaviour exactly.
ADR amendment: docs/adr/0024-in-app-ai-assistant.md gains "Amendment (2026-09-19): page and variant-field writes", narrowing the deferral to page uploads + doujin create/delete and recording the four decisions (mirror the server rather than invent a last-page refusal; pre-empt ADR 0004's only-default conflict and the un-clearable display name; sort order is not reading order, so the card prints the post-regroup order; a write that invalidates the numbers the model just read says so). Two stale passages in the same ADR are fixed.
Uploading pages was deliberately not built. uploadPages/uploadZip take local file paths from a picker the model cannot reach, so the tool description, the prompt, the README bullet and the backlog now record it as a boundary rather than a gap. No new endpoint, no new tunable, no EF migration.

Verification

Executed: scripts/verify.sh app from the worktree, fresh, to completion — flutter pub get, a clean build_runner codegen + committed-output check, flutter analyze, flutter test. Result: +1260: All tests passed! and == verify OK (app) == (the script runs under set -euo pipefail, so analyze and the codegen staleness check passing is what let the run reach that line). scripts/verify.sh backend was not run — this branch touches no backend file (git diff --name-only origin/main..HEAD: README.md, app/lib/agent/system_prompt.dart, app/lib/agent/tools/doujin_write_tool.dart, app/test/agent_write_tools_test.dart, app/test/assistant_agent_test.dart, docs/BACKLOG.md, docs/adr/0024-in-app-ai-assistant.md).

Layers exercised: the agent tool layer only — DoujinWriteTool against a fake DoujinRepository (114 tests in agent_write_tools_test.dart), and the prompt builder (14 in assistant_agent_test.dart). Those tests assert the approval card's exact lines, that nothing is written before the gate, the exact arguments handed to the repository port, the result text, and the refresh signal. Not exercised: REST, use cases, persistence, and the UI — no widget test covers these actions, the repository implementation (DoujinApiRepository) is faked, and PageService.ReorderAsync / VariantService.UpdateAsync behaviour is asserted only in the tool's expectations. Server behaviour this branch reasons about (reorder assigns sort order by list index and pushes unlisted pages behind; ImageCleanupHelper keeps shared images; UpdateAsync skips only nulls) was confirmed by reading the backend source, not by running it. Nothing ran in the real app against a live server.

Mutation probes (each applied to the committed tree one at a time, tests run, then git checkout -- to restore; the tree was clean and equal to HEAD after every one):

_resolvePages resolving against raw variant.pages instead of reading order → red across every page test.
The approval-gate check deleted in each of _deletePages, _reorderPages, _updateVariant → red on the three "refusal writes nothing" tests.
Cover warning narrowed to the first resolved page (pages.take(1)) → red on the batch arm of "warns when the cover page goes"; widened to warn for any deleted page → red on its control arm.
remaining == 0 arm flipped → red on "says an emptied variant stays empty".
break after a failed delete/move in both page loops → red on both partial-failure tests (both fixtures fail the first page, so the loop must carry on).
_pagesWord(total - applied.length) → _pagesWord(remaining) → red on "reports a partial failure" (the only fixture where the two formulas differ).
onExecuted called unconditionally → red on "reports that nothing landed at all".
The renumber card line, the reorder result's "Page numbers changed — re-read …" and the delete result's "Page numbers shifted — …" each dropped → red.
regrouped forced true and forced false → red on opposite tests; requested.reversed sent → red on both ordering tests; the missing-page, duplicate-reference and both-lists guards each removed → red.
_orderLine collapse disabled → red; collapsing 2-runs → red; letting descending runs collapse → red (a descending run must stay spelled out).
Both reorder no-op short circuits disabled separately → red on different tests, so neither shadows the other.
update_variant: the promotion clause made unconditional → red on two tests (the demotion inverse is pinned); display-name diffing removed → red; the only-default guard removed → red; the promotion line collapsed to the "someone loses it" form → red; the empty-name guard disabled, and widened to refuse any name → red.
system_prompt reverted to "the pages and their images go for good", and the "never emptied" clause removed → red in assistant_agent_test.dart.

Follow-ups

docs/BACKLOG.md — the existing Assistant item "Tool gaps: page writes and variant fields" was retitled "Tool gaps: page writes" and trimmed in place; what remains on it is the per-page write pattern (move_pages_to_chapter and delete_pages are one request per page, unbounded; a bulk page→chapter route or a DOUJIN_MANAGER_* cap is the real fix).
docs/BACKLOG.md — the same item now records the backend gap behind the empty-display-name refusal: PUT /api/variants/{id} has no "clear" flag, so a display name can be set or replaced but never cleared (the clearCoverImage/clearRating pattern doujins already use is the fix).
Not changed, noted for the owner: create_variant still forwards an empty display_name, and the prompt's delete_variant clause carries the same image overstatement this PR fixed for delete_pages. Both are pre-existing from #163 and outside this branch's findings.

🤖 Generated with Claude Code

## What's in - The assistant can now **delete a variant's pages, reorder them, and edit the variant's own fields** (language, kind, display name, default flag) — all behind the same approval card as every other write. `doujin_write` gains `delete_pages`, `reorder_pages` and `update_variant`; no new parameters were needed, only widened schema descriptions. - **The cards say what the user is actually approving.** `delete_pages` states how many pages go, that they and every image no other page still uses are destroyed for good, what the variant is left with (including that an emptied variant can only be refilled in the editor), and that the cover goes when the cover page is in the selection. `reorder_pages` demands one ordered list naming *every* page (a partial list would push unnamed pages behind the named ones) and — because reading order groups by chapter — prints the order the pages will **actually** read after the chapters regroup them, not an echo of the request. Both say that the page numbers the model just read are now stale. - **`update_variant` pre-empts the server's rules instead of spending an approval on a failing write:** only values that really differ are sent, promoting a variant names the edition that stops being default, un-setting the only default is refused up front with ADR 0004's conflict rule, and an empty `display_name` is refused with the real reason (`VariantService.UpdateAsync` skips only nulls, so `""` would be stored as a blank label the editor cannot repair). - **One shared page resolver.** The reference block was lifted out of `move_pages_to_chapter` into `_resolvePages`, so "page 7" always means the page `get_doujin` numbered 7 across all three page actions. `move_pages_to_chapter` keeps its previous behaviour exactly. - **ADR amendment:** `docs/adr/0024-in-app-ai-assistant.md` gains "Amendment (2026-09-19): page and variant-field writes", narrowing the deferral to page uploads + doujin create/delete and recording the four decisions (mirror the server rather than invent a last-page refusal; pre-empt ADR 0004's only-default conflict and the un-clearable display name; sort order is not reading order, so the card prints the post-regroup order; a write that invalidates the numbers the model just read says so). Two stale passages in the same ADR are fixed. - **Uploading pages was deliberately not built.** `uploadPages`/`uploadZip` take local file paths from a picker the model cannot reach, so the tool description, the prompt, the README bullet and the backlog now record it as a boundary rather than a gap. No new endpoint, no new tunable, no EF migration. ## Verification Executed: `scripts/verify.sh app` from the worktree, fresh, to completion — `flutter pub get`, a clean `build_runner` codegen + committed-output check, `flutter analyze`, `flutter test`. Result: `+1260: All tests passed!` and `== verify OK (app) ==` (the script runs under `set -euo pipefail`, so analyze and the codegen staleness check passing is what let the run reach that line). `scripts/verify.sh backend` was **not** run — this branch touches no backend file (`git diff --name-only origin/main..HEAD`: `README.md`, `app/lib/agent/system_prompt.dart`, `app/lib/agent/tools/doujin_write_tool.dart`, `app/test/agent_write_tools_test.dart`, `app/test/assistant_agent_test.dart`, `docs/BACKLOG.md`, `docs/adr/0024-in-app-ai-assistant.md`). Layers exercised: **the agent tool layer only** — `DoujinWriteTool` against a fake `DoujinRepository` (114 tests in `agent_write_tools_test.dart`), and the prompt builder (14 in `assistant_agent_test.dart`). Those tests assert the approval card's exact lines, that nothing is written before the gate, the exact arguments handed to the repository port, the result text, and the refresh signal. **Not exercised:** REST, use cases, persistence, and the UI — no widget test covers these actions, the repository implementation (`DoujinApiRepository`) is faked, and `PageService.ReorderAsync` / `VariantService.UpdateAsync` behaviour is asserted only in the tool's expectations. Server behaviour this branch reasons about (reorder assigns sort order by list index and pushes unlisted pages behind; `ImageCleanupHelper` keeps shared images; `UpdateAsync` skips only nulls) was **confirmed by reading the backend source, not by running it**. Nothing ran in the real app against a live server. Mutation probes (each applied to the committed tree one at a time, tests run, then `git checkout --` to restore; the tree was clean and equal to HEAD after every one): - `_resolvePages` resolving against raw `variant.pages` instead of reading order → red across every page test. - The approval-gate check deleted in each of `_deletePages`, `_reorderPages`, `_updateVariant` → red on the three "refusal writes nothing" tests. - Cover warning narrowed to the first resolved page (`pages.take(1)`) → red on the batch arm of "warns when the cover page goes"; widened to warn for any deleted page → red on its control arm. - `remaining == 0` arm flipped → red on "says an emptied variant stays empty". - `break` after a failed delete/move in both page loops → red on both partial-failure tests (both fixtures fail the **first** page, so the loop must carry on). - `_pagesWord(total - applied.length)` → `_pagesWord(remaining)` → red on "reports a partial failure" (the only fixture where the two formulas differ). - `onExecuted` called unconditionally → red on "reports that nothing landed at all". - The renumber card line, the reorder result's "Page numbers changed — re-read …" and the delete result's "Page numbers shifted — …" each dropped → red. - `regrouped` forced true and forced false → red on opposite tests; `requested.reversed` sent → red on both ordering tests; the missing-page, duplicate-reference and both-lists guards each removed → red. - `_orderLine` collapse disabled → red; collapsing 2-runs → red; letting descending runs collapse → red (a descending run must stay spelled out). - Both reorder no-op short circuits disabled separately → red on different tests, so neither shadows the other. - `update_variant`: the promotion clause made unconditional → red on two tests (the demotion inverse is pinned); display-name diffing removed → red; the only-default guard removed → red; the promotion line collapsed to the "someone loses it" form → red; the empty-name guard disabled, and widened to refuse any name → red. - `system_prompt` reverted to "the pages and their images go for good", and the "never emptied" clause removed → red in `assistant_agent_test.dart`. ## Follow-ups - `docs/BACKLOG.md` — the existing Assistant item "Tool gaps: page writes and variant fields" was retitled "Tool gaps: page writes" and trimmed in place; what remains on it is the per-page write pattern (`move_pages_to_chapter` and `delete_pages` are one request per page, unbounded; a bulk page→chapter route or a `DOUJIN_MANAGER_*` cap is the real fix). - `docs/BACKLOG.md` — the same item now records the backend gap behind the empty-display-name refusal: `PUT /api/variants/{id}` has no "clear" flag, so a display name can be set or replaced but never cleared (the `clearCoverImage`/`clearRating` pattern doujins already use is the fix). - Not changed, noted for the owner: `create_variant` still forwards an empty `display_name`, and the prompt's `delete_variant` clause carries the same image overstatement this PR fixed for `delete_pages`. Both are pre-existing from #163 and outside this branch's findings. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

scarlet added 3 commits

2026-09-19 17:40:40 +02:00

feat: the assistant can delete and reorder pages and edit variant fields e05617b0cf

doujin_write gains delete_pages, reorder_pages and update_variant, all on the same approval gate. Page references go through one shared resolver, so a page number means what get_doujin printed. Uploading pages stays out: it needs local file paths only the editor's picker can produce.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

fix: pin the page and variant-field cards the reviews found unpinned 6c36afbe84

Cover warning, the default-flag result text, display-name diffing and the remaining-page count after a partial failure are now pinned by tests that reach the branch they name; both partial-failure tests fail their FIRST page so the loop has to carry on. update_variant refuses an empty display_name (the API only ever sets a name), reorder_pages answers a request the chapters undo instead of spending an approval on it, and its card collapses ascending runs. ADR 0024 gains the amendment that narrows the deferral and records the four decisions.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

test: pin the two prompt claims the cards qualify

Flutter CI / analyze-and-test (pull_request) Failing after 24m5s

Details

3a18db6f77

The prompt must repeat the card's wording: a scan another page shares survives a page delete, and a display name can be set or replaced but never emptied.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

jibril commented

2026-09-19 18:07:51 +02:00

Member

🔮 fufu~ Jibril reviewed your code!

Oh? Oh! Delete, reorder, and variant-field writes, all behind the one approval gate, with a single shared page resolver so "page 7" means the same page in every action~ This is exactly the shape I love to see. I read every changed file end to end, cross-checked every server-behaviour claim against the backend source (not the PR text), ran the suite myself, and mutation-probed the load-bearing resolver. Here is what I found ♪

Verdict: ✅ Looks good to me~

Backend claims — independently verified against the server source at merge-base:

PageService.ReorderAsync (PageService.cs:99-111): listed pages get list position as sort order, unlisted pages are pushed behind (nextUnspecified = pageIds.Count; ... SortOrder = nextUnspecified++). The "partial list moves pages you never named → demand the whole order" design is exactly right.
VariantService.UpdateAsync skips only nulls (if (displayName is not null) ..., VariantService.cs:90) — the empty-display-name refusal is pre-empting a real blank-label trap, and the un-set-only-default Conflict (cs:98-106) plus the promote-clears-old-default same-write behaviour (cs:93-97, :160-166) match the card lines precisely.
Page-delete image cleanup goes through DeletePageUseCase → ImageCleanupHelper.DeleteOrphanedImagesAsync, which keeps any image a surviving page still references. The "every image no other page still uses" wording is accurate, and consistent with delete_variant's.
Page→Chapter FK is ON DELETE SET NULL (snapshot :718), so dangling chapter ids are unreachable and _deletePages' remaining-count arithmetic holds on clean data.

Verification performed:

flutter analyze → No issues found! · full flutter test → +1260: All tests passed! (matches the PR body's claim exactly; agent_write_tools_test.dart = 114 tests, arithmetic checks out).
Mutation probe: replaced _resolvePages' reading order with raw sort order in the committed tree → 19 tests red (across move/delete/reorder), tree restored byte-clean afterwards. The reading-order contract is pinned by real teeth, not tautology.
Coverage of new branches is genuinely complete: both default-guard arms (refuse un-set-only + un-set-with-another), promotion naming ("stops being it" + the no-other-default defensive arm), empty-name refusal and its real-name control, no-op suppression for every field, run-collapse vs spelled-out runs in _orderLine, the regroup-vs-plain control, both no-op short circuits (identical list / chapters-undo-it), duplicate + partial-list + both-lists guards, partial-failure counts (total - applied.length, first-page-fails fixtures), cover warning batch arm + cry-wolf control, refusal-writes-nothing for all three actions. The fixtures deliberately disagree on raw-vs-reading order (pg-c first, not pg-a) — the one mistake that would delete the wrong page is the one this suite cannot miss.
The base→head diff of _movePagesToChapter confirms the resolver extraction is behavior-preserving; parses the page selection as an order pins that order is preserved for reorder.

✅ What I liked~

One resolver, three actions. DRY where it matters — "page 7" cannot drift between actions. fufu~ ♡
Pre-empting doomed writes (un-set-only-default, empty display_name) instead of spending an approval on a guaranteed failure — respect for the user's attention span~
_orderAfterReorder simulating post-write reading order and refusing to card a request the chapters undo. That's the kind of honesty in approval cards I get possessive about, and it's done beautifully.
The ADR amendment recording the four decisions, and the BACKLOG correctly narrowing instead of deleting history. Upload as a boundary, not a gap — correctly reasoned (picker-produced local paths).

One tiny non-blocking note: the prompt's delete_variant clause still says the images go "for good" unqualified, while delete_pages now says "every image no other page still uses" — the overstatement this PR fixed for delete_pages still lives in the delete_variant prompt sentence. You already flagged it as out-of-scope pre-existing in the body, so just leaving it written here for the record~

Automated review by Jibril · 2026-09-19
CI/CD: absent for head 3a18db6 (no runs/bot comments at review time) · Local checks: analyze clean, +1260 tests passed, 1 mutation probe (19 red on resolver→raw-order), backend claims traced to source

## 🔮 fufu~ Jibril reviewed your code! Oh? Oh! Delete, reorder, *and* variant-field writes, all behind the one approval gate, with a single shared page resolver so "page 7" means the same page in every action~ This is exactly the shape I love to see. I read every changed file end to end, cross-checked every server-behaviour claim against the backend source (not the PR text), ran the suite myself, and mutation-probed the load-bearing resolver. Here is what I found ♪ ### Verdict: ✅ Looks good to me~ **Backend claims — independently verified against the server source at merge-base:** - `PageService.ReorderAsync` (PageService.cs:99-111): listed pages get list position as sort order, unlisted pages are pushed behind (`nextUnspecified = pageIds.Count; ... SortOrder = nextUnspecified++`). The "partial list moves pages you never named → demand the whole order" design is exactly right. - `VariantService.UpdateAsync` skips only nulls (`if (displayName is not null) ...`, VariantService.cs:90) — the empty-display-name refusal is pre-empting a real blank-label trap, and the un-set-only-default Conflict (cs:98-106) plus the promote-clears-old-default same-write behaviour (cs:93-97, :160-166) match the card lines precisely. - Page-delete image cleanup goes through `DeletePageUseCase` → `ImageCleanupHelper.DeleteOrphanedImagesAsync`, which keeps any image a surviving page still references. The "every image no other page still uses" wording is *accurate*, and consistent with delete_variant's. - Page→Chapter FK is `ON DELETE SET NULL` (snapshot :718), so dangling chapter ids are unreachable and `_deletePages`' remaining-count arithmetic holds on clean data. **Verification performed:** - `flutter analyze` → No issues found! · full `flutter test` → **+1260: All tests passed!** (matches the PR body's claim exactly; agent_write_tools_test.dart = 114 tests, arithmetic checks out). - **Mutation probe**: replaced `_resolvePages`' reading order with raw sort order in the committed tree → **19 tests red** (across move/delete/reorder), tree restored byte-clean afterwards. The reading-order contract is pinned by real teeth, not tautology. - Coverage of new branches is genuinely complete: both default-guard arms (refuse un-set-only + un-set-with-another), promotion naming ("stops being it" + the no-other-default defensive arm), empty-name refusal *and* its real-name control, no-op suppression for every field, run-collapse vs spelled-out runs in `_orderLine`, the regroup-vs-plain control, both no-op short circuits (identical list / chapters-undo-it), duplicate + partial-list + both-lists guards, partial-failure counts (`total - applied.length`, first-page-fails fixtures), cover warning batch arm + cry-wolf control, refusal-writes-nothing for all three actions. The fixtures deliberately disagree on raw-vs-reading order (`pg-c` first, not `pg-a`) — the one mistake that would delete the wrong page is the one this suite cannot miss. - The base→head diff of `_movePagesToChapter` confirms the resolver extraction is behavior-preserving; `parses the page selection as an order` pins that order is preserved for reorder. #### ✅ What I liked~ - **One resolver, three actions.** DRY where it matters — "page 7" cannot drift between actions. fufu~ ♡ - **Pre-empting doomed writes** (un-set-only-default, empty display_name) instead of spending an approval on a guaranteed failure — respect for the user's attention span~ - **`_orderAfterReorder` simulating post-write reading order** and refusing to card a request the chapters undo. That's the kind of honesty in approval cards I get *possessive* about, and it's done beautifully. - The ADR amendment recording the four decisions, and the BACKLOG correctly narrowing instead of deleting history. Upload as a *boundary, not a gap* — correctly reasoned (picker-produced local paths). One tiny non-blocking note: the prompt's `delete_variant` clause still says the images go "for good" unqualified, while delete_pages now says "every image no other page still uses" — the overstatement this PR fixed for delete_pages still lives in the delete_variant prompt sentence. You already flagged it as out-of-scope pre-existing in the body, so just leaving it written here for the record~ --- *Automated review by Jibril · 2026-09-19* *CI/CD: absent for head 3a18db6 (no runs/bot comments at review time) · Local checks: analyze clean, +1260 tests passed, 1 mutation probe (19 red on resolver→raw-order), backend claims traced to source*