feat: the assistant proposes a whole set of associations on one approval card #102
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/assistant-batched-approvals"
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?
Setting up a doujin through the assistant meant approving one card per tag. The comprehensive-setup skill asks for 15 to 25 of them, which made it unusable. Now the assistant proposes all associations — tags, people with roles, circles, characters, series — on one card the user reads and approves or denies once.
What's in
associationsarray on the existingassign/removeactions rather than new batch actions, so the action enum stays as it is and the single-item fields keep working as the one-element case. The array wins when both forms are sent, and that precedence is now pinned by a test rather than only documented.tag: + elf,person: + Alice as artist— names, never ids. On approval the writes run sequentially, continue past an individual failure, and return a per-item applied, failed and skipped summary. Denial writes nothing.entity_writecreate action already bundled name, description and aliases under one card, which is the precedent that makes this consistent rather than a new exception.Verification
scripts/verify.sh app: green, 773 tests; the write-tool suite went from 41 to 52.Decisions to revisit
Unknown ids reject the whole call so the user never sees a half-applied set; skips are the opposite and are tolerated, so a retry after a partial failure stays idempotent. If you would rather have distinct
assign_many/remove_manyactions for a clearer tool listing, that is the alternative I weighed and rejected.🤖 Generated with Claude Code
The comprehensive-setup playbook asks for 15–25 tags plus creators, characters and a series, and every `doujin_write` assign raised its own approval card, so cataloguing one doujin meant clicking through ~25 of them. `assign`/`remove` now take an `associations` array of `{entity_type, entity_id, role}` items instead of gaining new actions: the action enum stays at eight, one property carries the batch, and the existing single-item fields keep working as the one-element case. Every id is resolved to a display NAME before the gate — unknown ids, unknown entity types and role-less people are listed up front and the whole call is rejected without ever raising a card. Members already in the requested state are skipped rather than failed. What survives rides on ONE ChangeRequest whose lines read `tag: + elf` / `person: + Alice as artist`; on approval it executes sequentially and continues past an individual failure, and the result reports applied / failed / skipped per item (and says plainly when nothing was written). Denial writes nothing. Entity listings are now fetched once per type instead of once per id, so a 20-tag batch lists the tags once. `comprehensive-doujin-setup.md` collects ids through phases 4–6 and assigns them in a single call (new phase 7); the prompt's parallel-batching rule points at the array so the model does not fan out into one card per tag instead. The approval card already rendered a multi-line change list, so the UI is untouched. Verified: `scripts/verify.sh app` (771 tests, analyze 0 issues). Mutation probes on all six new guards — the pre-gate error return, per-item failure containment, the name-listing cache, the singular/plural card title, the "nothing was written" summary and the already-assigned skip — each turned `agent_write_tools_test.dart` red. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>Flutter Coverage
Total: 80.4% (7747 of 9641)
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! Twenty-five approval cards collapsing into ONE? I read the whole diff twice just to watch the validation pipeline flow again~ ♡ Resolve every id to a name before the gate, reject the whole call on anything unusable, skip what's already there, and only then bother the human — that is exactly the shape this needed. And you kept the action enum untouched by making the array win over the scalar trio. Delicious.
Verdict: ✅ Looks good to me~
I hunted for a reason to bite and came back hungry. Everything I probed held:
Verified myself (mutation probes, each RED on exactly its own test, restored byte-identical — working tree pristine at
3eb3c8fafterwards):_entityNamesmemoization →listTagsCallsExpected 1 / Actual 2. The 20-tags-one-listing claim is real, not decoration~errors.isNotEmptyrejection → the unknown-ids test fails. No card can ever rise from a broken batch.Architecture: the ADR 0024 amendment's precedent check is honest — I read
entity_write's_create: it already bundles name+description+aliases on one card and continues past alias failures with a per-item report. This PR is that pattern generalized, not a new exception. Approval granularity = the tool call is the right invariant, and "nothing reaches the repository without a card the user approved" survives intact.The tricky edges, all correct:
_parseAssociationskeeps a malformed entry as an empty spec instead of dropping it — the approved batch can never be quietly shorter than what the model asked for. That's the kind of paranoia I approve of ♡onExecutedfires once per call and only when something actually landed; all-fail reports "NOTHING was written" instead of masquerading.−/+card lines,labelsummaries, and singular/plural card titles all match the file's existing conventions.Coverage: every genuinely NEW branch has a directional test — mixed six-item batch (exact card lines, ordered writes,
listTagsCalls == 1), deny, partial failure, all-fail, unknown ids, invalid entries, skips (both directions, batch and scalar), remove batch, schema payload incl. the array-over-scalar precedence, and the schema property itself so it can't silently vanish from the model's view. Theplanned.isEmptyarm is exercised by the scalar no-op test through the new unified code. Suite 41→52,scripts/verify.sh appgreen with 773/773 locally — matching your PR body digit for digit.💡 Little ideas (non-blocking)~
expect(contains('or an associations array'))and it can never drift silently.t-1twice) would pin the all-skipped batch summary (Nothing to change — …; …, zero cards) distinctly from the scalar no-op. Pure pinning nicety.✅ What I liked~
Never issue one assign per tagis pinned inagent_skills_testso it can't regress.assistant_agent_testpins the exact wording under the byte-identical-prompt constraint. Sharp~Automated review by Jibril · 2026-09-06
CI/CD: passed for head SHA
3eb3c8f(coverage bot #7196, doujin_write_tool.dart 86.4%) · Local checks: scripts/verify.sh app green — analyze clean, 773/773; 3 mutation probes RED→restored→green