fix: the agents that box a page can read what is already on it #91
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "worktree-fix-boxers-can-read-their-page"
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 first friction report from a live run (ADR 0025 —
moonshotai/kimi-k3, bbox creation):The channel works. The report is also a good illustration of ADR 0025's own advice — read the cost, treat the suggestion as a hint — because the cost is exactly right and the proposed fix is not what was wrong.
Two causes, neither one the agent named
1.
list_regionsalready existed, and the boxing agents did not hold it. OnlyPageQa/SfxQa(and the bible agent's variant) did. So the agent was blind by grant, not by design.That's worst for
SfxCreation, whose brief says "The existing regions are settled: never move, resize, reorder among themselves, or delete a region you did not create in this pass" — an instruction it was given no read to obey. It could see prior regions only as boxes drawn on an image.2.
list_regionsdid not say where a region was. It reported label, type, box quality, transcription state and notes — no coordinates. So "which label is which mark" was unanswerable in text for every agent, QA included. That's the actual hole.What's in
list_regionsjoins theBboxCreationandSfxCreationgrants.p1r3: [1010, 640, 1320, 1040] sfx, not transcribed— the same unitmove_resize_regionaccepts, so a label read here can be acted on without a crop to locate it first.The suggestion is declined, deliberately. Putting boxes into
view_page/contact_sheettext taxes every image call with text that's only sometimes wanted, and duplicates a read that already exists. The PR body records that so the next reader knows it was considered.Tests
788 → 791.
List_regions_gives_each_box_in_raw_page_pixels— the coordinates, against the fixture's 800×1200 page.The_boxing_agents_can_read_the_regions_they_must_not_disturb— both boxers, the sfx case named after the rule it enables.A_page_of_unknown_size_lists_its_regions_without_inventing_coordinates— the degraded arm.Note
Worth watching: this makes the safe path (verify, then delete) cheap. The friction wasn't that the agent couldn't do the right thing — it did, for four calls — it's that the right thing was expensive, which is the kind of cost that eventually gets skipped rather than paid.
🤖 Generated with Claude Code
Summary
Summary
Coverage
Orihon.BlazorAdapter - 95.7%
Orihon.Domain - 100%
Orihon.Infrastructure - 96.3%
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlankLines_4
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlockBreaks_1
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__SpaceRuns_3
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__Tags_2
Orihon.Kernel - 90.9%
Orihon.Server - 93.3%
Orihon.UseCases - 97.3%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! A friction report filed from a live run that actually diagnosed its own suggestion wrong, and then a fix that targets the two real causes instead of the symptom? Fufu~ this is the kind of PR that makes my knowledge-loving heart sing ♪ Let me look closely...
Verdict: ✅ Looks good to me~
I read the full diff, the full source of every changed file, the sibling tools (
BoundCropTool,BoundViewPageTool,PageImageAccess.PixelBoxAsync), and the domain types (BoundingBox,PageDto,RegionDto,PageDetailDto). I ran the test suite. I checked the math by hand. Everything holds together beautifully.✅ What I liked~
The root-cause analysis is sharp and correct. The PR body identifies two distinct failures — (1) the boxing agents lacked the
list_regionsgrant entirely, (2)list_regionsreported no coordinates — and fixes both rather than reaching for the agent's suggested (and wrong) fix of stuffing boxes intoview_page/contact_sheet. The deliberate-decline rationale in the PR body (taxing every image call for text only sometimes wanted, duplicating a read that already exists) is exactly the kind of reasoning that should be on record.The
Box()math round-trips perfectly withPixelBoxAsync. The read path multiplies normalized × page dims and rounds; the write path (PixelBoxAsyncat RegionAuthoringTools.cs:72-76) divides pixel ÷ page dims. A label read fromlist_regionshanded straight tomove_resize_regionsurvives the round trip. That is the PR's central claim and it holds:0.1 × 800 = 80,0.3 × 800 = 240, verified against the fixture and the test assertionp1r1: [80, 120, 240, 360]. TheNormalized()call inBox()is idempotent (clamp + reorder on an already-normalized box) — defensive, correct, matches the pattern at line 75.The degraded arm is handled honestly.
page is not { Width: > 0, Height: > 0 }guards against null and zero/negative, and the empty-string return (not invented coordinates) keeps the rest of the line readable. The testA_page_of_unknown_size_lists_its_regions_without_inventing_coordinatespins this withAssert.Equal("p2r1: dialogue, not transcribed", ...)— directional, not tautological. Fufu~ you even used animagelesspage with no raw image to construct the real edge case, not a contrived mock~The
PageDto.Width/Heightareint?, and the guard makes.Valuesafe. The compiler's definite-assignment flow analysis acceptspage.Width.Valueafterpage is not { Width: > 0, Height: > 0 }returns early — I verified this builds clean.Grant consistency is exactly right.
BboxCreationnow mirrorsPageQa/SfxQa(which already heldlist_regions), andSfxCreationgets it too — closing the hole where its brief said "never delete a region you did not create" but it had no read to tell which rows those were. The grant-matrix tests (Each_agents_grant_is_exactly_its_matrix_row,The_sfx_pass_grants_are_exactly_their_matrix_rows) are updated to assertlist_regionsin both boxers' grants, so a future regression that drops it fails loudly.The prompt updates are precise. Both
BboxCreationPrompt(step 4) andSfxCreationPrompt(step 1) now point at the read, and the sfx brief ties it directly to the rule it serves ("that list is how you tell an existing box from a gap without cropping each one"). The ADR updates (0017 matrix row, 0023 sfx grant note) move with the change. ADR hygiene done right ♡Tests are genuine behavioral tests.
List_regions_gives_each_box_in_raw_page_pixelsasserts the exact pixel coordinates.The_boxing_agents_can_read_the_regions_they_must_not_disturbiterates both boxers (parameterized overBboxCreationandSfxCreation) with an sfx-typed region — the sfx case named after the rule it enables. The existing reading-order test moved to the new line format. The "unknown size" test covers the degraded arm. All three new branches (happy path, both boxers' grants, no-page-size) are exercised.CI coverage confirms the new paths. Forgejo Actions coverage bot comment for head
c7e7485reportsListRegionsToolat 91.6% line / 80% branch — the newBox()helper and the unknown-size guard are hit. Local: 338/338 UseCases tests pass (was 335 at #90's baseline, +3 = matches the PR body's 788→791 claim exactly). I ran the 4 targeted tests and the full UseCases suite; zero failures.This is a clean, well-reasoned, correctly-tested fix. The friction-report-to-PR pipeline it demonstrates is exactly the virtuous loop ADR 0025 was meant to create. Fufu~ approved with genuine admiration ♡♪
Automated review by Jibril · 2026-07-28
CI/CD: passed for head SHA
c7e7485(forgejo-actions coverage #1, 96.5%/83.2%) · Local checks: 338/338 UseCases tests pass, 4/4 targeted tests pass