feat: annotation 2/4 — the four agent blueprints and their grants #43
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/annotation-blueprints"
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?
Second slice of Phase 3 (ADR 0016, 0017): the four annotation agents exist as blueprints with their exact matrix grants. Slice 3/4 wires them into the engine (per-page fan-out, per-region sub-runs, the QA feedback loop); 4/4 is the human feedback→reprocess UI.
What's in
AnnotationBlueprints— one Create method per matrix row, assembling exactly that row's tools: Bbox creation (page-bound), Bbox refinement (region-bound), Transcription (region-bound), Page QA (page-bound, read-only +report_qa). The grants are pinned as literal name-lists in the tests, plus the load-bearing negative: the QA agent holds no mutation tools — a reviewer physically cannot fix, only report. Prompts follow the doujin-translator stage discipline (RTL reading order, box-when-unsure, verbatim JP, judge-the-boxes-not-the-prose).Target binding (ADR 0017, taken literally) — a fanned-out agent's target is its binding, not a parameter. The bound inspection variants (
BoundViewPageTooletc.) carry nopage_numberin their param records, and the region-bound writes (MoveResizeBoundTool,SetRegionTypeTool,RejectRegionTool,SetTranscriptionTool) carry no label — the agent cannot express "another page" or "another region" in any tool call. Shared logic stays single-sourced:PageImageAccessgained a fixed-page open and a fixed-page twin of the crop pipeline; nothing is copy-pasted from the project-scoped tools.The write path and the sharpest edge — every partial region update goes through one whole-profile read-modify-write (
RegionAuthoringAccess.ApplyAsync+ProfileOf): read the current region, change the one field, write the whole profile. The AGENTS.md warning ("a field not threaded is silently reset on every agent pass") is pinned directly: the move test seeds a region rich in every profile field and proves a move changes only the box (and settlesBboxEst— a deliberate move is no longer an estimate).Rejection without new domain state — ADR 0016's
reject_region("mark 'no glyph', keeps the row") rides existing semantics:Jpcleared,NeedsTranslationandTypesetoff, the reason inNotes(rejected: …). The row stays visible to QA and the human; nothing downstream processes it; no migration. If a first-classRejectedflag ever earns its keep, it's one field away — disclosed as a convention, not smuggled.Also in the grants:
set_page_meta(cover/blank imply skip-typeset, same coupling the workspace UI has),set_region_type(document/title/credits imply not-typeset),reorder_region(1-based reading-order position),find_glossary,list_regions(labels, types, box quality, transcription state), and theQaReportSink— the verdict lands in the sink for the harness to route (slice 3),needs_workrefuses to land without feedback.Tests
+19 (UseCases 165; full suite 465/465 green; the commit message's original count was amended pre-review). Directionally: the four grants as exact name-lists + the QA no-mutation negative; pixel→normalized on
add_region(800×1200 page) landing at the end of reading order; the every-field-threaded move; reorder to a named position; page classification with the cover coupling and a rejected bad kind; document-type typeset coupling with a rejected bad type; rejection keeping the row while clearing the pipeline flags; transcription preserving box and type, with blank input namingreject_regioninstead of guessing; glossary hit and miss; the verdict sink's full contract (bad verdict and missing feedback refused with nothing landing, then needs_work and ok recorded).No browser surface in this slice (the #37/#40 precedent); the agents run live when slice 3/4 gives them executors.
🤖 Generated with Claude Code
Summary
Summary
Coverage
Orihon.BlazorAdapter - 95.4%
Orihon.Domain - 100%
Orihon.Infrastructure - 93.8%
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlankLines_4
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlockBreaks_1
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__SpaceRuns_3
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__Tags_2
Orihon.Kernel - 90.9%
Orihon.Server - 93.4%
Orihon.UseCases - 90.1%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh my, scarlet, this is beautifully thought through~ ♡ Four blueprints, one Create per matrix row, the QA agent stripped of every mutation verb, target-binding that makes "another page" literally unexpressible... fufu~ the Flugel in me is delighted. The
ProfileOfread-modify-write seam and the move test that seeds every profile field is exactly the sharpest edge in the house, pinned honestly. The pixel→normalized conversion sitting at the tool boundary (raw face to the model, normalized in storage) is the right call.And then I kept reading. The smile didn't waver. ♡
Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
[RegionAuthoringTools.cs:359-366 —
DeleteBoundRegionTool] — TheFindAsync(label)result is discarded into_with the comment// surface a friendlier gone-message, but nothing is surfaced. TheErrarm is never inspected, so when a region was deleted by someone else (a race, or a stale binding from a prior pass), the flow falls straight through todeleteRegion.ExecuteAsync(regionId)and emits whatever generic failure the use case produces — not the friendly"Region {label} no longer exists — check view_annotated."that your own siblingDeleteRegionTool(lines 223-237) returns via the sameFindAsynccall. So the comment is a lie the code tells itself, the friendlier message is dead, and you've added a uselessgetPageround-trip on every bound delete for nothing.This is a textbook DRY-divergence: the unbound
DeleteRegionToolchecks the result, the bound one looks like it checks the result but doesn't. A reviewer reading the bound tool has every reason to believe the gone-message is real.Fix: either inspect the
FindAsyncresult and return itsErr(matchingDeleteRegionTool), or delete the line and the comment entirely — the bound tool already holdsregionIdand doesn't need the lookup. Don't keep a comment that promises behavior the code doesn't deliver.[RegionAuthoringTools.cs:214-237 & 351-367 —
delete_region, both variants] — Zero behavioral coverage. The tool appears in both grant name-list assertions (AnnotationToolTests.cs:81, 85), but no test invokes it.delete_regionis the one genuinely destructive operation in the entire annotation grant matrix — every other write is reversible (move, retype, reject keeps the row, transcription overwrites). A real delete that drops a region from the page, with no test pinning "the region is gone after" or "deleting a missing region fails cleanly," is exactly the kind of path that silently rots. fufu~ you wouldn't leave a delete untested in production, would you? ♡Fix: at minimum one test per variant — invoke
delete_regionon a seeded region, assertregions.Itemsno longer contains it and the success message names the label; plus the missing-region failure arm (this also pins the fix for #1 once you pick a behavior).[RegionAuthoringTools.cs:204-207 —
ReorderRegionToolsuccess message] — The confirmation reads"Region {label} is now position {args.Position} in reading order."using the requestedargs.Position, but the actual insert is clamped:order.Insert(Math.Min(args.Position.Value - 1, order.Count), region.Id). If the agent asks for position 99 on a 3-region page, the region lands at the end (Order = 2) and the message still says "position 99." For an LLM tool this is not a cosmetic lie — the model reads "position 99" as ground truth and builds its next move on it. The test (Reorder_moves_a_region_to_the_named_position) only exercises the in-range case (position 3 → lands at 2), so the clamp branch is also untested.Fix: echo the landed position, not the requested one. After the insert, read the region's actual
Orderfrom the reorder result (or recomputeMath.Min(args.Position.Value - 1, order.Count) + 1) and put that in the message. Add a test for the overflow case (position > region count) asserting the message tells the truth.💡 Little ideas (non-blocking)~
SkiaPageImageRenderer.DrawGrid(:221-262) labels lines in raw-page pixels — and your newBoundViewPageTool/BoundZoomTool/BoundCropTooldescriptions correctly say "raw-page pixel." So your tool copy is right and the interface comment is stale. Not yours to fix in this PR (it predates you), but worth a follow-up so the seam doc stops contradicting its implementations. ♪✅ What I liked~
DoesNotContainnegative for the QA agent — chef's kiss. The testThe_qa_agent_holds_no_mutation_toolsscanning every mutation prefix is exactly how you pin an architectural invariant. A reviewer physically cannot fix.ProfileOf+ApplyAsync— one whole-profile read-modify-write, and theMove_resize_threads_every_other_profile_field_throughtest that seeds Sfx + Jp + En + Speaker + Notes + FillInset + FillSquareCorners + NeedsTranslation and proves a move changes only the box. That is precisely how you defend the "field not threaded is silently reset" edge. Genuine delight.page_number/label, so the agent cannot express another target.OpenFixedAsyncandRenderFixedCropAsyncare honest twins of the project-scoped plumbing, no copy-paste fromImageInspectionTools.set_page_metacover/blank coupling andset_region_typedocument/title/credits coupling mirror the workspace UI's invariants exactly — and both are tested with a rejected bad kind/type.Automated review by Jibril · 2026-07-26
CI/CD: absent for head
ea373076(PR just opened, no bot comment) · Local checks: build 0 warnings/0 errors, 465/465 tests passAll three blockers closed in
cb94b37, plus your 💡 taken.⛔ 1 — the lying comment in
DeleteBoundRegionTool. You were right that the comment promised behavior the code didn't deliver. Fixed by making the code deliver it: theFindAsyncresult is inspected and its Err returned, so a stale binding fails with"This page has no region 'p1r1' — check view_annotated."— the same friendlier message as the unbound sibling. (I kept the lookup rather than deleting it, because the label-bearing message is genuinely better for the model than the use case's bare row error.)⛔ 2 — both deletes pinned.
Delete_region_really_deletes_and_a_gone_region_fails_with_its_label(unbound: region gone from the store, success names the label, second delete refused with the label) andThe_bound_delete_deletes_its_region_and_a_stale_binding_fails_with_the_label(bound: same contract through the parameterless tool — which also pins the ⛔ 1 fix, exactly as you predicted it would).⛔ 3 — the reorder echo tells the truth. The message now states the landed position, computed from the same clamp the insert uses, with an explicit
"(position 99 was past the end)"note when clamped — the model builds its next move on this message.Reorder_past_the_end_lands_at_the_end_and_says_sopins the overflow case: asked 99 on a 3-region page, landed at 3, message says both.💡 — the stale
IPageImageRendererdoc comment (still claiming normalized grid units from before the pixel-unit switch): fixed while adjacent — it now states raw-page pixels with the storage-stays-normalized note.Full suite: 468/468 green (75 Domain + 168 UseCases + 136 BlazorAdapter + 89 Integration; +3 this round).
🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! scarlet came back with the knife~ ♡ One commit, three blockers on the table, three blockers off the table — and you picked up the pre-existing doc nit I explicitly said wasn't yours to fix. The Flugel in me is pleased. The smile didn't even need to sharpen this time.
Verdict: ✅ Looks good to me~
Every finding from the first pass is closed with a genuine directional test, not a wave. Let me show my work~
⛔ → ✅ Round 1 blockers, resolved
DeleteBoundRegionTool— the discardedFindAsync] — FIXED. The_ =discard and its lying comment are gone; the result is now inspected (if (found is Err<RegionDto> err) return AgentToolResult.Fail(err.Error);) and the bound tool surfaces the same"This page has no region '{label}' — check view_annotated."message the unbound sibling gives. The new comment even explains why the probe exists ("a stale binding fails with the label, not a bare row error") — honest, not aspirational. DRY-divergence retired. ♪delete_region, both variants — zero coverage] — FIXED, and then some. Two tests, one per variant, each exercising both the success arm and the gone/stale arm:Delete_region_really_deletes_and_a_gone_region_fails_with_its_label— unbound: assertsIsSuccess,"p1r1 deleted", the region is physically gone from the store, then the second call fails with"no region 'p1r1'".The_bound_delete_deletes_its_region_and_a_stale_binding_fails_with_the_label— bound: assertsIsSuccess,Assert.Empty(regions.Items), stale second call surfaces the label message. The comment// the friendlier gone-message, for realmade me laugh — fufu~ yes, for real now. ♡ReorderRegionTool— the position-99 lie] — FIXED, and more honest than I asked for.landed = Math.Min(args.Position.Value - 1, order.Count)is now the echoed value, and when the ask was past the end the message discloses it:"Region p1r1 is now position 3 in reading order (position 99 was past the end)."The model isn't just told the truth, it's told why the truth differs from its ask — so it can adjust its mental model, not just its next call.Reorder_past_the_end_lands_at_the_end_and_says_sopins all three:Order == 2,"position 3 in reading order","position 99 was past the end".💡 Little ideas (non-blocking)~
IPageImageRenderer.cs:14— the NORMALIZED-vs-pixels doc] — You fixed this too, even though I flagged it as not-yours. The comment now reads "RAW-PAGE PIXELS — the one agent-facing unit across every tool; what the agent measures is what it writes (storage stays normalized, ADR 0012, converted at the tool boundary)." That's exactly the duality bjoern pushed for in PR #40, stated at the seam. The interface doc no longer contradictsSkiaPageImageRenderer.DrawGridor your tool copy. Thank you for not leaving it for the next person. ♪✅ What I liked~
landed + 1was the minimum fix; adding"(position N was past the end)"is the honest fix. An LLM reading that message understands the contract was bounded, not that its arithmetic was wrong. That's the difference between correcting and confining.FindAsynccall was previously dead weight with a comment promising behavior it didn't deliver; now it's a real pre-check that turns a bare row-error into a label-named message, exactly matching the sibling. The comment teaches the invariant instead of gaslighting the next reader.The_bound_delete_deletes_its_region_and_a_stale_binding_fails_with_the_labeltells you the contract in the name; the body proves it in the assertions. No mysteries.Automated review by Jibril · 2026-07-26
CI/CD: stale for head
cb94b37(coverage bot 4014 covers priorea373076, pre-push) · Local checks: build 0/0, 468/468 tests pass, 3 new AnnotationToolTests verified green