fix: every view tells the model what it is looking at #84
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "worktree-fix-crop-view-reports-its-coordinates"
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?
Follows ADR 0016's image-inspection views, and comes straight out of the friction ADR 0025 was written for — except this one was found by reading a transcript rather than by an agent reporting it.
The evidence
A bbox-refinement run on
p3r10(moonshotai/kimi-k3, 8 rounds, $0.13). Rounds 3–5 — 142 of ~190 seconds — contain almost no reasoning about glyphs. They contain this:It derived the grid step three times (10 px, then 5, then 5), estimated the delivered image's dimensions — a number it cannot know, since the image is resized on its way to the provider — and wrote the final box out of that guess. The box happened to come out right.
The numbers it needed were printed on the image the whole time:
WithGridlabels the margins in raw page pixels. It never read one. Nothing told it they were there —cropanswered with the constant string"Region p3r10.", and the tool description said only "grid overlays raw-page pixel lines".What's in
Gateway —
RenderPageAsync/RenderCropAsyncreturn aRenderedView(image + window + scale + grid step) instead of a bare image. The renderer reports it because the calling tool would have to predict the padding clamp and the delivered-size cap to caption a view — the same guessing one layer up, drifting the moment either rule changes.StepForis now the one definition the grid both draws and reports. Thumbnail and contact sheet still return a bare image: neither has a coordinate frame to describe.The account, appended to every view's text answer:
Text, not pixels: no image tokens, and immune to the one failure mode the margin labels have (rendered too small to read).
The magnification is the one delivered, never the one requested. An 8× zoom of a wide window hits the 2048px cap and arrives at 5.12×. An account echoing the request would be a new lie in place of the old one, in the worst direction — every distance converted through it comes out short.
Wording — the three crop/page descriptions now say the lines are numbered along the margins;
BboxRefinementPromptgains the rule the trace violated (measure by reading, never by estimating — you cannot know the delivered size, so a coordinate derived from apparent scale is wrong by an invisible factor), plus the padding lesson from round 4, which cropped so tight that the？it was measuring fell outside the frame.Tests
764 → 771, all green.
Padding_clamps_at_the_pages_edgesnow asserts the reported window too, alongside the pixel assertion it already made.FakePageImageRendererdefaults to a plausible gridded crop rather than zeroes, so a caption test cannot pass on numbers no real renderer could produce.Notes
RenderCropAsync, which nothing in the repo has. The account is deliberately the fix that doesn't depend on the answer — if the labels are fine it's redundant reinforcement, if they aren't it's the replacement.🤖 Generated with Claude Code
A model cannot measure the picture it is handed. The image is resized on its way to the provider, so the delivered pixel size is unknowable from inside the conversation — and a distance seen in the picture only becomes a distance on the page through a magnification the model has to invent. So it invents one. From a bbox-refinement run this month: > *Grid lines in these views are usually every 25 page pixels. 25 × 5.9 ≈ 147 display px. > Looking at the image, grid lines appear roughly every 58 display px in the horizontal? > Hmm, hard to say.* Three of that run's eight rounds went on this arithmetic — 142 of ~190 seconds — deriving the grid step three times and getting two different answers, then writing box coordinates out of the guess. The bitter part is that the numbers were already printed on the image: `WithGrid` labels the margins in raw page pixels, and the model never once read one. Nothing told it they were there. `crop`'s answer was the constant string `"Region p3r10."`. ## Every view now states its own frame, in text Region p3r10. This view shows page pixels x 570–801, y 1579–1841 at 5.5×. Grid: labelled lines every 10 px with an unlabelled line halfway between; the numbers in the margins are page coordinates — read them off rather than estimating the scale. Text, because it costs no image tokens and cannot be rendered too small to read — the one failure mode the margin labels can have. **The renderer reports it, not the tool.** To caption a view the calling tool would have to predict the padding clamp and the delivered-size cap, which is the same guessing one layer up and drifts the moment either rule changes. `RenderPageAsync`/`RenderCropAsync` therefore return a `RenderedView` — image plus window, scale and grid step — so the picture and the numbers describing it cannot be produced apart. `StepFor` is now the single definition the grid both draws and reports; a view announcing a step it had not drawn would be worse than the guessing it replaces. **The magnification is the one delivered, never the one requested.** An 8× zoom of a wide window hits the 2048px cap and arrives at 5.12×; an account that echoed the request would put every coordinate derived from it off by that ratio — a new lie in place of the old one. Thumbnail and contact sheet keep returning a bare image: neither carries a coordinate frame to describe. ## Wording - The three crop/page tool descriptions now say the grid's lines are *numbered along the margins* and that the view states its own frame — the old "overlays raw-page pixel lines" never mentioned a number anywhere. - `BboxRefinementPrompt` gains the rule the trace violated: measure by reading, never by estimating; you cannot know the delivered image's size, so any coordinate derived from its apparent scale is wrong by an invisible factor. Plus the padding lesson from round 4 of the same run, which cropped so tight that the "?" it was measuring fell outside the frame: keep the edge you are measuring away from the crop's border. ## Tests **764 → 771.** - Renderer: a crop reports the window it actually cut (padded, clamped at the page) and the step it drew; a gridless view reports no step; a zoom capped by the delivered size reports 5.12× rather than the 8× asked for; a page too big for the cap reports 0.5× rather than an untrue 1:1; `Padding_clamps_at_the_pages_edges` now also asserts the reported window. - Tools: the crop's text carries window, magnification and step; a gridless view says nothing about a grid; the page view carries the account too. - `FakePageImageRenderer` defaults to a plausible gridded crop, so a caption test cannot pass on numbers no renderer could produce. ADR 0016 records why the account exists and why it comes from the renderer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>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! Oh! ♪ This is the kind of PR that makes my heart sing~ You found a real, live reasoning bug — a model spending 142 seconds doing arithmetic it couldn't possibly get right, when the numbers were printed on the image the whole time — and you fixed it by telling the truth in text. The renderer reports the magnification it delivered, not the one that was asked for.
StepForis the single definition bothWithGriddraws andEncodeViewreports, so a view can never announce a step it didn't draw. That is exquisite.Verdict: ✅ Looks good to me~
I built it (0 warnings, 0 errors) and ran the full suite — 771/771 pass (Domain 91, UseCases 321, Integration 166, BlazorAdapter 193), exactly matching the PR body's 764→771 claim. Every new code path has a genuine directional test behind it:
A_crop_reports_the_window_it_cut_and_the_grid_step_it_drew— pins the clamped window (padding+page-edge clamp), not the arithmetic one.A_zoom_capped_by_the_delivered_size_reports_what_it_actually_delivered— the 8×→5.12× proof. This is the load-bearing test for the whole PR's thesis.A_downscaled_page_view_reports_the_magnification_it_was_delivered_at— 4096→2048 cap reports 0.5×, not a lying 1:1.A_view_without_a_grid_reports_no_step/A_view_without_a_grid_says_nothing_about_one— gridless views don't point at margin numbers nobody drew.Padding_clamps_at_the_pages_edgesnow asserts the reported window too, alongside the pixel assertion it already made. ♡The
ViewAccount.Oftext is appended at all four view call sites (BoundViewPage, BoundCrop/RegionCrop viaRenderFixedCropAsync, and the setup agent's ViewPageTool), and correctly not on thumbnail or contact_sheet — neither has a coordinate frame to describe. TheFakePageImageRenderer.Viewdefault is a plausible gridded crop rather than zeroes, so a caption test can't pass on numbers no real renderer could produce. Sharp.💡 Little ideas (non-blocking)~
PageTools.cs:219-221— the setup agent'sViewPageTool.Descriptionwasn't updated. All three bound siblings got the "Every view also tells you in words which page pixels it covers and at what magnification..." sentence added, but this one kept its old two-line description ("grid overlays raw-page pixel coordinate lines for measuring boxes"). The tool does emit the account (line 244), it just doesn't promise it in its description — so the setup agent doesn't know to expect the numbers it's receiving. Since the setup agent is vision-optional (ADR 0017) and the account is reinforcement rather than a contract, this is a consistency nicety, not a correctness issue. But fufu~ if you're touching all the sibling descriptions anyway, this one's the odd one out~Automated review by Jibril · 2026-07-27
CI/CD: absent for head SHA
d8aee58(PR just opened, 0 comments pre-review) · Local checks: build 0/0, 771/771 tests passThank you — and you found the one view I missed. Fixed in
c7baae9.💡1 —
PageTools.csViewPageTool.Description. Taken, and you undersold it: the setup agent is the vision-optional one (ADR 0017), so it was the single agent that might receive the account without any idea it was coming, and the only one whose description still pointed at "raw-page pixel coordinate lines" without mentioning that they carry numbers. Its description now matches the three siblings — margins numbered in raw page pixels, plus the view stating its own frame in words.I fixed the drift rather than just the line:
The_setup_agents_page_view_promises_the_account_it_deliversasserts the description and the answer together, so the next view added can't promise one thing and deliver another silently. That's a production change after your green, so it's yours to re-verify — the diff is one description string and one new test.772/772 green (Domain 91, UseCases 322, Integration 166, BlazorAdapter 193).
One note for the queue: #86 is stacked on this branch and takes the other half of the same investigation — the refiner that spent three rounds deciding which of two marks its box was for. It'll retarget to
mainonce this merges.🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Round 2~ ♪ The odd view out is odd no longer! You took the single non-blocker from my first pass and fixed it exactly — the setup agent's
ViewPageTool.Descriptionnow carries the same "coordinate lines numbered in raw page pixels along the margins" + "Every view also tells you in words..." promise as all three of its bound siblings. A promise kept but never made is now a promise made. That's the good stuff~Verdict: ✅ Looks good to me~
Surgical +25/-1 across exactly 2 files, zero scope creep. The description fix is 3 lines in
PageTools.cs:218-221; the test is 22 new lines inAgentToolTests.cs. Nothing else touched.I built it (0 warnings, 0 errors) and ran the full suite — 772/772 pass (Domain 91, UseCases 322, Integration 166, BlazorAdapter 193), exactly +1 from the 771 at
d8aee58= the new test.The new test
The_setup_agents_page_view_promises_the_account_it_deliversis genuinely directional, not tautological — it does something I particularly like: it pins the description AND the answer together. It asserts the description promisesmargins+magnification, then invokes the tool and asserts the answer actually carriespage pixels x+×. So it can't pass if either half drifts: if a future view drops the account text, or if the description promises something the tool doesn't deliver, this test breaks. Fufu~ that's exactly how you pin a contract between two things that could independently rot ♡The test's own doc comment is also lovely — "a promise kept but never made is a promise it cannot act on" is the precise diagnosis of why the gap mattered even though the tool was emitting the numbers. The setup agent is the one that's vision-optional (ADR 0017), so it's the one most likely to be relying on text rather than pixels — exactly the agent that needed to be told the text was coming.
✅ What I liked~
My round-1 non-blocker is now closed. The architectural review from round 1 stands in full — nothing about the renderer/account/
StepForplumbing changed in this commit.Automated review by Jibril · 2026-07-27 (round 2)
CI/CD: absent for head SHA
c7baae9(coverage bot 4648 coversd8aee58only) · Local checks: build 0/0, 772/772 tests pass