fix: the region tools say what they actually did #138
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/classify-region-answers"
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?
Four defects from agent friction reports (ADR 0025), all filed on runs that succeeded — the agent reached a good answer and told us what it had cost. All four are the same shape: the tool did something the agent could not see. Nothing here changes which containers are accepted; every change is in what the answer says. Touches the tool catalog of ADR 0016 and the container/seed/ink vocabulary of ADR 0012.
What's in
The refinement briefing (
RegionBriefing.ForRefinement) — the kickoff handed over the box and the type, then asked the agent to judge an earlier pass's work it had no read for. It now names the recorded form, the container's seed and effective tolerance in page pixels, and the ink. A region with no container of its own is told so explicitly, because the alternative is an agent spending crops hunting for a fill that was never painted.Points now round on the way out where boxes still round outward: a box must contain what it names, a point must name the pixel it was written down as. Verified over every width to 2000 —
(decimal)p/w*wtruncates top-1for 695358 pairs and never does under rounding. Pinned by the 790/1200 seed.Transcription deliberately gets none of this: the transcriber reads glyphs out of a box, and form/seed/ink change none of its answers. Sizeless pages get none either — every view and write already fails there. Both pinned.
The runaway refusal closed with "Crop the region with
annotatedto see what filled." On refusal the method returnsFailbeforeApplyAsync, nothing is stored, and only a stored fill is ever tinted — so that view was guaranteed empty. The sentence is gone, and aDoesNotContain("annotated")guard keeps it gone.Acceptance and refusal now quote the same number. Acceptance quoted a share and a size; refusal quoted the spread. An accepted fill therefore could not be judged against the line that refuses one. Both quote the spread now, and both the refusal and
classify_region'sDescriptionname the limit and its unit: four times the region's box on its longer side, a linear ratio, not area.% of the pageis kept in the accepted answer on purpose — it is still the criterion where no box was given, and the refusal quotes it in that arm, so dropping it would have re-created the very unit mismatch this slice removes. The spread is named as the one that decides.ContainerFitgained anAim(gateway layer). The filler's search steps up to 24 px off the point it was given whenever that point found no surface holding the region's box — which is what makes two points a couple of pixels apart answer with unrelated containers, and a container come back not holding its own seed.SkiaContainerFiller.Locatenow returns the point it actually filled from, and the fit carries it whenever that is not the point given.Aimis null when the fill started where it was pointed: the filler is the only place that knows whether the search moved, and a consumer comparing its own seed against an always-set aim would have to round-trip its pixel through the normalized frame and would report phantom0 pxmoves.Both the accepted and the refused answer name the distance and the new point. The accepted arm adds "your point stays the one on record, and it fills to this same container every time" — the deliberately weaker claim:
Locate's best-effort exit returns a point that did not qualify, so "re-seed there and you get the same container" would have been another unverifiable sentence, which is the defect this slice exists against.The editor (
PageWorkspacePage.RoomHint) now says the same things to the person: the spread on its longer side, and a note explaining the seed dot that sits outside the green.That note states the fill's own point and no distance. A distance would pair two generations of state — the seed is saved and the rooms re-measured in two dispatches, so after the save lands and while (or if) the re-measure does not, both buffer and store hold the new seed while
Roomsstill holds the old fit. Nothing in the state records which seed a fit came from, so the only value that cannot lie is one computed from the fit alone. The agents' answer keeps its distance, because there it is computed inside one call from the point that call was handed. If the distance is wanted for the person too, the honest way is to carry the measured seed beside each fit inPageRoomsMeasured— state plumbing this slice does not own.Docs —
LabeledBox.Fillsaid the container is computed from its seed; it is computed from beside the seed wherever the seed missed, and that was the last claim site insrc/saying otherwise. ADR 0016'sclassify_regioncatalog entry names the spread as the deciding number, with aStatusamendment line for the half that is new.Tests
+15 tests (9 + 4 + 2 across three rounds), plus assertions bolted onto 5 existing tests. Fresh full run after the final edit: Domain 151, UseCases 600, BlazorAdapter 281, Integration 295 = 1327 passed, 0 failed, 0 skipped. Build clean under
TreatWarningsAsErrors— 0 warnings, 0 errors.Briefing:
Refinement_is_told_the_form_container_and_ink_an_earlier_pass_recorded— asserts the three recorded facts reach the refiner, and pins the pixel round-trip at[337, 790].A_defaulted_tolerance_is_named_by_the_number_the_fill_uses— asserts the null-tolerance arm prints the number the fill actually uses, and that the no-outline ink arm stays clean of an outline clause.A_region_with_nothing_recorded_is_told_that_too— asserts all three absent arms, andDoesNotContain("neighbour")so the retired false claim cannot come back.A_rough_box_still_carries_what_an_earlier_pass_recorded— asserts a rough box keeps the recorded half, because boxing marks every box it touches rough, including one an earlier refinement had settled.Transcription_is_told_nothing_about_the_container_or_the_ink— a regression guard on a deliberate design choice; its target exists, so it is a live guard.Tools:
A_refused_fill_names_the_limit_it_passed_in_the_unit_the_accepted_one_uses— asserts the refusal quotes the spread in acceptance's own words, and that "annotated" is gone.A_fill_that_started_somewhere_else_says_where— asserts the moved aim and the on-record sentence.A_refused_fill_also_says_where_it_started— asserts the refusal names the move and that the on-record sentence stayed out of it (it recorded nothing).25% is where a fill has stopped describing a containerclause.Gateway:
A_rescued_aim_is_named_and_a_kept_one_is_not— asserts both arms of the null convention.An_answer_that_never_held_the_box_still_names_where_it_filled_from— covers the best-effort exit.A_rescued_fill_is_the_same_fill_every_time_it_is_measured— asserts the determinism the new accepted sentence claims, and thatContainerFit's value equality survived the new field.Assert.Null(fit.Aim)on an existing test.Editor:
An_accepted_fill_is_measured_in_the_agents_unit_as_wellandA_fill_that_escaped_is_measured_for_the_person_in_the_agents_unit— assert both arms print the spread on its longer side.A_fill_that_started_somewhere_else_says_so_to_the_person_too.A_seed_the_rooms_have_not_caught_up_with_is_not_measured_against_the_old_fill— fails the next measurement and asserts the note still names the fill's own point and contains nopx away. Verified as a real pin: with a distance temporarily restored it fails withTEMP 251 px away, exactly the number that describes nothing.All four
Locateexits are exercised (point kept, ring candidate qualifying, best-effort, windowless fallback). BothAimarms, both refusal arms, both acceptance arms and both ink arms are asserted. The editor tests were verified against a reverted razor; the source-revert checks used a per-filegit apply -R, nevergit stash.Honest notes
AnnotationBlueprints.cs:305(BboxRefinementPrompt) still tells the refiner "Containers are not yours… the green you see painted over the art is its work, so a container that looks wrong is not something to fix here — and nothing here can." That contradicts its own step 4 twenty lines above, the grant that hands itclassify_regionwith a seed, and now the new briefing sentence.AnnotationBlueprints.csbelongs to the sibling prose slice in this batch and is untouched here. Merge-order note: if that slice lands first, nothing here conflicts; if this lands first, the prose slice inherits the contradiction to resolve. The finding is precise enough to hand over as-is.LooksLikeRunawaycannot fire fromclassify_region:coverisfound.Value.Bbox,RegionDto.Bboxis non-nullable, soSpreadis null only whenFindAsyncalready failed — i.e. the region is gone and theApplyAsynctwo lines later fails too. That is why theDescriptionnames the spread alone while the refusal still quotes the share in the arm that can print it. It is asserted anyway, because the fake-filler tests print it.AnnotationBlueprints.cs:285already promised "The answer also tells you how large the container came out against this region's own box." That was false before this branch and is true now, with no edit to the blueprint — a sibling file became correct by someone else's change.SkiaPageImageRenderer.Tintplus a way to carry an unstored fit to the renderer.SkiaContainerFiller.Fit) — it changes which pixel the flood starts from for roughly a third of (pixel, width) pairs, which is behaviour, and this slice is reporting-only;Point()'s doc records instead that the distance is measured from the pixel the caller named. Naming the 25% limit inclassify_region'sDescription— see note 2. Dropping the recorded sentences from the rough arm — see the test that pins why.PageWorkspacePage.razor,docs/adr/0016andIPageImageRenderer.cs. Each was taken at an explicit review request; none is on the do-not-touch list.PageImageAccess.cs:210carries a related unqualified claim and was not touched (do-not-touch list); it is also the least misleading site, since it describes which seed the request carries.PageWorkspacePage.razor:1214has an orphaned doc comment stacked aboveCaptionInk's own<summary>(dates to #116), andPageWorkspaceEffects.cs:85-90has the same shape. Both onmain, both left alone rather than guessing an owner.RunMonitorTests.A_burst_of_engine_changes_coalesces_into_one_re_read(RunMonitorTests.cs:372) is a debounce flake under concurrent-worktree load — untouched by this branch, and it passes 281/281 when that project runs alone. It did not reproduce in the final runs. A1326/1rerun with that name is the flake, not this change.SeedDevDatachecked each round and correctly untouched: the aim is computed, never stored, and nothing here adds a new kind of user-authored content. No ADR was written — behaviour is unchanged and the new contract is one optional field on a gateway record, documented on the record and pinned by tests.🤖 Generated with Claude Code
Summary
Summary
Coverage
Orihon.BlazorAdapter - 95.4%
Orihon.Domain - 100%
Orihon.Infrastructure - 97.1%
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlankLines_4
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlockBreaks_1
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__SpaceRuns_3
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__Tags_2
Orihon.Kernel - 90.9%
Orihon.Server - 94.4%
Orihon.UseCases - 97.6%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! A PR whose entire reason to exist is that the answer must not say what did not happen — four defects, all filed on runs that succeeded, all the same shape: the tool did something the agent could not see. You even closed the runaway-refusal's phantom prescription ("crop it with
annotated" on a refusal that stores nothing — a diagnostic that could never show anything) and pinned its grave with aDoesNotContain. That is exactly the kind of possessiveness I approve of~ ♡Verdict: ✅ Looks good to me~
I chased this one properly, so here is what I actually did:
e7f3cf1. EveryContainerFitconstruction site grepped:SkiaContainerFiller.Fitis the only one, soAimcannot be forgotten anywhere. All three reporters (accepted answer, refusal, editorRoomHint) carry the move in consistent words; the briefing correctly prints the stored seed, not the aim — a seed is what the record holds and what a re-judge must compare against. That distinction is the sharpest thing in the PR.Locateexit audit: all four exits (point kept →(best, x, y), ring candidate qualifying →(candidate, px, py), best-effort →bestAt, windowless → re-fill at the given point) return their point honestly, and the(aimX, aimY) == (x, y) ? null : Point(...)null-convention means a kept point is never reported as moved. Each exit has a test standing on it — includingAssert.Null(fit.Aim)on the windowless fallback, which is the arm most authors would have forgotten~8% of pairs (6,571 of 80,199) and♪Math.Roundon none. And I mutation-probed the pin at head: replacingMath.Roundwith truncation inPixelPointmakesRefinement_is_told_the_form_container_and_ink_an_earlier_pass_recordedgo red by itself — restored, green again, md5-verified pristine. Not a tautology, fufu[300, 340], aim 0.28→[336, 340]— the test's comment and the assertion agree with arithmetic, and the two-dispatch race rationale for omitting a person-facing distance is documented where the next reader will trip over the question.e7f3cf1at review time): build clean underTreatWarningsAsErrors, 1327/1327 passed, 0 failed — Domain 151, UseCases 600, Integration 295, BlazorAdapter 281. Your PR-body arithmetic matches my run digit for digit, which after last week's "green CI ≠ green suite" lesson is the part that made me happiest~DoesNotContain("neighbour")keeps the retired false claim from crawling back. The briefing's honest note that an unseeded region tints nothing — because an annotated view paints only its own region's container — is the kind of absence-worth-as-many-words-as-presence reasoning I want on every tool answer.Your Honest Notes section is disarmingly good, by the way — the known-unreachable share arm (
Spreadnull only whenFindAsyncalready failed) reasoned through and still asserted via the fake-filler tests, theBboxRefinementPromptcontradiction precisely located and handed to the sibling slice with a merge-order note. Nothing I found contradicts any of it.💡 Little ideas (non-blocking)~
Moved(...)sentence ("the fill reported here is that point's, not yours") rather than on the lever ("point further in"). Agents read tails. A swap would cost nothing; entirely optional.ContainerFit.Aimis already the right shape to carry it. Not this PR's problem~✅ What I liked~
Aim-null-when-kept convention with its three-part comment: "a caller told 'the fill started where you pointed' on every answer learns nothing from the line" — signal-to-noise reasoning applied to tool prose, not just code. ♡Describedropping its secondPageAsyncround-trip by carrying the page beside the container — a latency fix smuggled politely into a correctness PR, and honestly documented as such.Nothing here changes which containers are accepted, and every sentence the tools now say is one that is true. Merge it~ ♪
Automated review by Jibril · 2026-09-20
CI/CD: absent for head SHA at review time (no coverage bot comment) · Local checks: build 0w/0e, 1327/1327 passed, 1 mutation probe red-on-target