feat: assistant opens the reader and Offline page, sets covers, lists newest doujins #92
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/assistant-tool-gaps"
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?
The in-app assistant can now open a doujin in the reader, open the Offline page, set a doujin's cover, and list doujins newest-first (or by title/rating/updated).
What's in
navigateaccepts/read/<doujinId>/<variantId>(optional?page=N, positive integer) and/offline; the static system prompt's route table advertises both (still byte-stable across turns — the pinning test passes).doujin_writegainsset_cover: the image id is resolved against the doujin's pages across all variants before the approval gate, so the card reads "cover: (none) → page 2 of the en/translated variant" (or "cover: image → …" when the current cover is not one of its pages) and a foreign image id errors up front; on approval it callsDoujinRepository.updateDoujin(coverImageId:)exactly like the Editor. The backend only checks the image exists, not that it belongs to the doujin, so the client-side check is the only ownership guard today.search_doujinsgainssort_by(created | updated | rating | title) andsort_directionthrough the existingSearchQuery— no backend change; unknown names error before the request; every result row now ends withadded: YYYY-MM-DD.Verification
scripts/verify.sh app: green (agent tool and assistant suites: 64 tests in the two touched files; full app suite green on the implementer run and on the six-branch integration tree).6a616aa): allowlist anchor dropped → red;page=0/ non-numeric page allowed → red;/offlineremoved → red; sort-name validation disabled → red;set_covergate bypassed → red; old-cover fallback label regressed → red; default sort name changed → red.?page=Nis inferred fromapp.dart, not driven through the UI.Decisions to revisit
Ordering is two parameters mirroring the wire enums rather than one
order:enum;set_coverfetches each variant (novariant_idhint); a server-side ownership check forcoverImageIdwould belong in a backend backlog line.🤖 Generated with Claude Code
Review follow-ups on the assistant tool gaps branch: - DoujinWriteTool set_cover: the card's old-cover fallback ("image <id>" when the current cover is not one of the doujin's own pages) had no test. New test: fake repo with coverImageId 'foreign', set_cover to 'img-2', card change line must read "cover: image foreign -> page 2 of the en/translated variant". The branch is reachable because the backend only checks the image exists, not that it belongs to this doujin (DoujinUseCases: 400 on an unknown id, any existing image accepted) — the code comment said "stores any GUID", reworded. - SearchDoujinsTool: the header's default sort name when only sort_direction is given was untested; the ordering test now also calls with sort_direction 'asc' alone and expects "ordered by created asc" with sortBy left null on the SearchQuery. Verified: scripts/verify.sh app (691 tests, analyze clean). Mutation probes, each restored afterwards: regressing the fallback to '(none)' fails "set_cover labels a foreign current cover by its image id"; changing the default sort name to 'newest' fails "passes the ordering into SearchQuery and announces it". Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>Flutter Coverage
Total: 79.0% (7308 of 9252)
🔮 fufu~ Jibril reviewed your code!
Oh~ opening the reader and the offline library for the assistant, covers with human-readable approval cards, newest-first listings... scarlet keeps feeding my collection of well-shaped diffs and I keep coming back hungrier~ ♡
I traced every claim in the PR body against the tree: GoRouter really does land
/read/:doujinId/:variantId?page=N(app.dart:112int.tryParse(...) ?? 1, clamped inreader_page.dart:276, empty-variant guarded at:144), the backend defaults really areCreated/Desc(SearchEndpoints.cs:58,68),LibrarySortField.namereally is the wire format the backendEnum.TryParses, variant pages really arriveSortOrder-sorted (VariantEndpoints.cs:185) so the "page N of the en/translated variant" labels are truthful, andUpdateDoujinUseCasereally only checks image existence — the client-side scan is genuinely the only ownership guard today. The verification section is honest to a degree I want to frame and hang on my wall~Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
app/lib/agent/tools/search_doujins_tool.dart:219— the${sortDirection ?? 'desc'}fallback is a dark branch. It fires exactly whensort_byis given withoutsort_direction— and no test does that. Your "only a direction given" test pins the other fallback (sortBy ?? 'created'→ "ordered by created asc"), but its mirror went untested. I proved it in a review clone: mutating?? 'desc'→?? 'asc'keeps the whole suite green (agent_tools_test23/23 pass on the mutant). This isn't a cosmetic string — it's the assistant's claim about result order on a path the model will absolutely take ("sort by title" with no direction). If it drifts, the model reasons on a false premise and no test objects. Fufu~ you wouldn't leave an unpinned branch in production, would you? ♡Fix: one more arm in
passes the ordering into SearchQuery and announces it—SearchDoujinsParams(sortBy: 'title')alone, assert the header containsordered by title desc(andrepo.lastQuery?.sortDirectionstays null, proving the backend default is what's being announced).docs/BACKLOG.md— the PR body says a server-side ownership check forcoverImageId"would belong in a backend backlog line," but the diff only removes the shipped tool-gaps line; no ownership line lands under## Backend. Your own BACKLOG header states the convention explicitly: "Add to it in the same PR that defers something (PR bodies are not a tracker)." This PR defers something — the guard it itself documents as "the only ownership guard today" being client-side — so the line belongs here, not in a PR body that will scroll away~Fix: one line under
## Backend, e.g.cover_image_id has no ownership check — any existing image id is accepted as a cover (#92); the assistant's client-side scan is the only guard.💡 Little ideas (non-blocking)~
doujin_write_tool.dart:534— theimageId.isEmptyhalf of the guard is untested (set_cover without image_idcovers only the null arm). I checked the siblings:add_title's missing-fields test is null-only too, so this matches house convention — but since''comes from an LLM, oneimageId: ''arm in the existing test would pin it for free.✅ What I liked~
image <id>fallback for a foreign current cover (with its own pinning test!) shows you thought about the ugly states, not just the demo state. The backend's dangling-reference tolerance (cover_image_idhas no FK, existence-only check) correctly treated as a boundary to defend against, not a detail to hand-wave.LibrarySortField.values.asNameMap()— converting a backend 400 the model can't act on into an error that names the valid values. Textbook tool design~_readerQueryKeysvs_entityQueryKeys) instead of one global allowlist —/tags?page=2correctly rejected while/read/...?page=12passes. Route-family-aware validation is the right abstraction.page=0, allowlist anchor, gate bypass, label regression...) — I re-ran the suites and probed the one branch you missed; the discipline shows ♪Automated review by Jibril · 2026-09-06
CI/CD: absent for head SHA
6a616aa· Local checks: 77/77 pass across the three touched suites (agent_tools, agent_write_tools, assistant_agent); 1 mutation probe RED-verified (dark fallback), clone left pristineBoth blockers and the non-blocking idea in
4ea63bd(tests + one backlog line, nolib/change).1. The dark
?? 'desc'fallback — the ordering test gained the mirror arm:SearchDoujinsParams(sortBy: 'title')alone asserts the header saysordered by title descand thatlastQuery.sortDirectionis null, so what is announced is pinned as the backend default rather than a direction the tool chose. Probe:'desc' → 'asc'turns that arm red (ordered by title asc).2. Backlog line — added verbatim under
## Backend:cover_image_idhas no ownership check, any existing image id is accepted as a cover; the assistant's client-side page scan is the only guard.Non-blocking taken — the
set_cover without image_idtest now has animageId: ''arm. Worth knowing: without theisEmptyhalf the empty string does not crash, it falls through to the page scan and errors withimage "" is not a page of this doujin, so the guard's value is the clearer "requires image_id" message; the arm pins that branch either way (probe red).Verification on
4ea63bd:scripts/verify.sh appgreen (691 tests; the count is unchanged because both additions are arms inside existing tests, as you suggested).🔮 fufu~ Jibril reviewed your code!
Ohh~ you came back, and you brought me gifts~ ♡ A single test-only commit that closes both blockers AND the little idea I left behind, with zero production drift. This is the kind of response that makes a reviewer's heart do improper things~
Verdict: ✅ Looks good to me~
Round-1 items — all closed, and I checked with my own hands~
?? 'desc'fallback (search_doujins_tool.dart:219) — CLOSED. The newsortBy: 'title'-only arm asserts the header announcesordered by title descand thatrepo.lastQuery?.sortDirectionstays null — pinning both the fallback string and the fact that it's the backend default being announced, exactly the two-sided claim I asked for. Mutation-probed in my review clone:?? 'desc'→?? 'asc'now goes RED on preciselypasses the ordering into SearchQuery and announces it(it kept the suite green in round 1 — the branch is no longer dark). Restored, green again. The comment above the arm even documents why no direction is sent. Delightful~cover_image_id has no ownership check — any existing image id is accepted as a cover (#92); the assistant's client-side page scan is the only guard.sits under## Backend, following the header's own rule (defers in the same PR,(#N)attribution, imperative phrasing matching its siblings). I read the surrounding section — it lives exactly where a future server-side-guard PR will look.imageId.isEmptyarm (non-blocking) — picked up too! The empty-string case now asserts the sameset_cover requires image_iderror with zero cards. Mutation-probed as well: dropping|| imageId.isEmptyfrom the guard goes RED on exactlyset_cover without image_id errors without a card. You turned my little idea into a pinned invariant — fufu, I'm spoiled~✅ What I liked~
sort_direction.Ready to merge~ The assistant can finally show me the newest doujins and open the reader itself. Do take good care of it, okay? ♡
Automated review by Jibril · 2026-09-06
CI/CD: stale for head SHA
4ea63bd(coverage bot 7041 covers6a616aaonly) · Local checks: 77/77 pass across the three touched suites (agent_tools 23, agent_write_tools 41, assistant_agent 13); 2 mutation probes RED-verified (sort-direction fallback, empty image_id guard), clone left pristine