fix: the refinement brief stops contradicting the tool it drives #136
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/annotation-brief-sweep"
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 bbox refiner's brief promised four things
classify_regiondoes not do. Every one of them comes from an agent friction report (ADR 0025) filed on a run that succeeded — the agent finished the page and then wrote down what had cost it rounds, which is exactly the traffic that ADR is for. The claims are all about containers, so the vocabulary being corrected is ADR 0012's (form = bubblemeans cleaning fills within the container and leaves the outline standing — that is why an escaped fill is destructive and a short one is not), the grant the brief may draw on is ADR 0016's, and the one thing left undecided touches ADR 0023.The change is prose only. About 146 changed lines in one agent brief against roughly 400 lines of new tests, and no production branch added anywhere. The justification for that ratio: a brief that contradicts its tool costs rounds on every page it runs on, and the failure it produces is not cosmetic — a container recorded over two balloons is art the cleaning pass erases.
What's in
Use cases —
AnnotationBlueprints.cs, the bbox-refinement briefThe dead paragraph. "Containers are not yours. …a later pass points at each balloon's inside" contradicted step 3 of the same brief, which tells this agent to do precisely that. It had also replaced step 4, so the list ran 1, 2, 3, 5. Paragraph gone, final step renumbered.
The repair that isn't one. The brief promised a balloon "whose outline has a small break is repaired for you before you see it". Nothing is repaired: the fill is eroded by a pixel, the piece still holding the region's box is kept, the rest is dropped, and it only runs on a fill that has already spread well past its own text. So the brief now says cut away, says a radius-1 erosion parts a break "a pixel or two across", and — this is the part that saves the rounds — says that an outline crossed by an overlapping balloon or a tail leaves inside and outside one surface, where no point anywhere separates them and pointing again changes nothing.
The size rule, which named the wrong measure twice. It said "several times it is not a container", which is vague where the gate is exact, and the first fix named the longer side — also wrong.
SkiaContainerFiller.SpreadisMath.Max(acrossRatio, downRatio), each axis taken over the fill and the box together. The brief now states that, and the worked example is the manga norm that makes it bite: a vertical column can be 1.2× down and five times across, and five is the answer. Five, not the reviewer's arithmetically-correct 6.7 —Window()pads each axis toLooseSpread + 1and clips, so a per-axis ratio much above 5 is not reachable through a call that carries a box, and an unreachable number in a brief is a new lie for the next friction report. Also stated: the ratio is not handed back on success (the answer gives share + pixel size; spread appears only in the refusal), and the share-of-the-page rule can never decide this agent's refusal because it always has a box.What a refusal actually leaves. "Nothing is recorded" is true of the container, the type and the form that rode in with it — but a refusal returns before
ApplyAsync, so the placeholderRegionType.Dialoguethatadd_regionstamps survives, andlist_regionsprintsdialogue, form not judgedto every later pass. The brief now says so, and says why it matters: type is what decides whether the text is lettered at all, so an sfx or a title parked there ships wearing a type nobody chose.Tolerance, which had no unit. It is colour levels, per channel, against the seed's own colour — never against the neighbouring pixel, which is why the fill cannot creep up a gradient.
sample_colorsgives something to subtract, with one honest caveat now stated: it reports each colour's average, merges tones within ~40 levels, and reads only this region's own box. So a measured step is a floor, not the value to use; and a single colour coming back is as likely to mean the sampler never saw the tone — the box is snug round the glyphs while the tone runs across the whole interior — as that there is none.The ending the brief never had. Some balloons have no value that both covers the interior and stays inside the outline, and they are the ones that eat rounds. Three tries, then keep the largest value whose green stays inside, record it, note what you saw, stop. Short is the way to be wrong — see the note below for the four supports. Where no value stays inside, the two shapes are told apart by what the tool did, not what the green ran into: every call refused (the tool settled it — note and finish, and name the real type because the placeholder is what ships), or the green escaped and the call was not refused (a leak into the next balloon, or onto the page margin where the window is clipped short by the sheet edge — both frequently land under 4×). In that second shape the container is already on the region and nothing in the grant takes it back, so the brief tells the agent to make its last call the one it wants standing and to put the repair in a note aimed at the human's seed and slider.
Tests
1321 green across the four projects (151 / 599 / 277 / 294), +9 on main's 1312 — recomputed from a fresh run on
ad6f2d9, not carried forward from either review round. Build clean with-warnaserror, 0 warnings. 9[Fact]added, 0 removed.Every prose test carries the behaviour it describes or the constant it quotes, so prose and code break together:
Every_annotation_brief_numbers_its_steps_without_a_hole— the generalisable form of the original defect (a hole in the numbering is the tell that prose outlived the step it replaced). Runs across all six annotation briefs, not just the refiner's, and asserts each has at least three steps so it cannot pass vacuously.The_refusal_size_in_the_brief_is_the_linear_ratio_the_tool_measures— asserts the prose besideContainerFit.LooseSpread == 4m, so moving the constant breaks the test and forces the words to follow; asserts the share rule is stated as reported, not judged; and carries the folded-in behaviour pin (a fill far over the limit is refused and takes type and form with it).A_refused_call_leaves_the_placeholder_type_the_brief_names— drivesadd_region→ refusedclassify_region→list_regionsand asserts the printed line isdialogue, form not judged, the exact string the brief now quotes.A_fill_is_judged_on_the_worse_of_its_two_axes(Integration) — a real page, a 20×170 column, a balloon ordinary down and over the limit across. Asserts down <LooseSpread, across > it,LooksLikeRunaway, and thatSpreadtracks the across figure at more than twice the down one. A longer-side implementation returns ~1.06 here and fails four of the five.A_box_against_the_sheets_edge_gets_a_short_window_and_a_full_fill_passes(Integration) — the premise behind the two-shapes rule, against the real filler with no production change. Same 60×100 box, bare sheet: mid-sheetLooksLikeRunawayis true, at the corner it is false andSpreadis under the constant. Two positions, opposite outcomes, so it cannot pass vacuously.An_escaped_container_under_the_limit_stands_and_the_brief_says_so— pins the end state rather than the prose alone: a sub-limit classification succeeds and the container and the type are on the region, andreject_regionthen leavesContainer,TypeandFormexactly where the classification put them. PlusDoesNotContain("Refuse it yourself")and a count (not aContains) that "keeps the placeholder type it was created with" appears exactly once, so it cannot creep back into the escaped shape.The_tolerance_lever_is_stated_in_a_unit_the_agent_can_measure— interpolates the domain's own 1 / 128 / 16 bounds rather than copying them, computes the 58-level step at runtime from the two hex literals the assertion quotes (so example and arithmetic cannot drift), and guards the retired overstatements withDoesNotContain.A_balloon_with_no_workable_tolerance_has_an_ending_in_the_brief— asserts the ending exists and that a sub-limit fill really is recorded (IsSuccess,Containernon-null), which is the fact the ending is built on.The_brief_promises_only_the_repair_the_fill_actually_makes— asserts the cut-away wording and guardsDoesNotContain("mended for you").Negative checks were run for real in both rounds, not asserted: with the brief reverted a commit, 5 of the prose tests fail in round 1 and 3 in round 2, each time exactly the ones that round touched. The numbering guard correctly passes on both sides of the round-2 revert — round 1 closed that hole and round 2 did not go near it.
Honest notes
Why short beats escaped, since this is the one judgement call in the slice and it was made without a ruling: (a) ADR 0012's table and
Region.cs:44-46—form = bubblemeans cleaning fills within the container and leaves the outline standing, so an escaped fill erases art while a short one only leaves room unclaimed; (b) the tool refuses a runaway and never a shortfall, so a short fill is the only one that can be recorded at all; (c) both QA briefs already agree — PageQa says green covering "a little less than the whole inside" is not worth a send-back and that escaped green "is the one way a container goes wrong", so preferring short cannot bounce the page; (d) the person reviewing has this same threshold on a slider (PageWorkspacePage.razor:227-240), so the note lands on a lever someone can actually move.One design fork named rather than silently closed. Where an escape stays under the limit, the container is recorded and the refiner cannot un-record it:
reject_regionleavesType,FormandContainerstanding, and the only classifications that clear a container are embedded-without-seed and floating — both a lie about what is drawn, and form is what cleaning acts on. The brief now tells the truth about that and puts the repair in a note. If a region with no container is genuinely wanted there, the agent needs a way to clear one without lying about form — that is a new tool or a newclassify_regionargument, i.e. a production change, and it belongs to its own slice. It is not promised in prose here.LooseSpread = 4was deliberately not changed. Whether 4× is the right gate is a separate arc; this slice only makes the brief say what the gate is.Deferred sweep of the retired loose unit — four sites, all in files this slice does not own. Declined here rather than swept, because sibling slices are editing those files in parallel and a two-line doc edit buys nothing a follow-up cannot while risking a conflict on files this branch has no other business in:
src/Orihon.UseCases/Regions/MeasureRegionRooms.cs:13-14— a container "is never several times the size of it".src/Orihon.BlazorAdapter/PageWorkspace/PageWorkspacePage.razor:938— "never several times larger".src/Orihon.Infrastructure/Gateways/SkiaContainerFiller.cs:189-190—Spreaddocumented as "by the larger of its two dimensions", the container's longer side: the exact misreading this PR's first review round was raised about, sitting on the code that readsMath.Max(acrossRatio, downRatio).src/Orihon.Infrastructure/Gateways/SkiaContainerFiller.cs:163-166—Window's summary claims a fill that fills the window "still reports a spread over the limit and is refused rather than quietly cropped". True mid-page, false at a sheet edge, and now contradicted by a shipped brief and a passing integration test. The last two should be taken together by whichever arc owns that file; this one is a real inconsistency rather than a wording preference.RegionAuthoringTools.cs:551(classify_region's own description) still tells this agent "A fill of a quarter of the sheet or more is refused" — the share rule, which only fires when no box is given, and the refiner always has one. That file belongs to a sibling slice in this batch. The brief inoculates the refiner against it in one sentence meanwhile, so the two no longer contradict each other in the same context window whichever order they land in. No merge-order constraint: this branch touches no file any sibling owns.Found, not fixed, pre-existing on
mainand outside this slice: two genuine stacked-summary defects, a member wearing its neighbour's doc, atsrc/Orihon.BlazorAdapter/PageWorkspace/PageWorkspaceEffects.cs:83-88andtests/Orihon.BlazorAdapter.Tests/SetupChatTests.cs:41-47. Neither matches the same-line</summary><summary>grep, which is how they have survived; they show up on an adjacent-line scan.SeedDevDatais untouched, checked rather than assumed — the change adds no kind of user-authored content, only brief prose and tests. No ADR: nothing here decides anything beyond ADR 0012, and the one place a decision was needed is named above as its own slice.Open question for bjoern — not decided here
Which pass owns translator-pasted overlay lettering — a white halo round each glyph, no bubble — and a cover's display title and credits? The cover clause and ADR 0023's floating-text rule point opposite ways. One agent reported that guessing decided whether four of nine regions existed at all, and two separate friction reports ask for the ruling. Nothing in this PR changes behaviour for it; it needs a house rule (ADR 0026) or an ADR 0023 amendment, and it is lost if this line is dropped.
🤖 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 that fixes prose and treats the prose like production code — 146 changed lines of brief against 400 lines of tests that make the words and the tool break together. This is my favorite kind of candy~ ♡ A brief that lies to its agent is a bug that runs on every page, and you hunted each lie down to the constant it contradicts. *
Verdict: ✅ Looks good to me~
Everything verified, not just read:
ad6f2d9. Build Release with-warnaserror: 0 warnings, 0 errors. (The sandbox was being squeezed by sibling review sessions — MSBuild OOM'd twice, so the test runs went throughvsteston the freshly built DLLs; counts are from clean runs.)Spreadreally isMath.Maxof two per-axis ratios over the fill∪box union (SkiaContainerFiller.cs:206-218);Windowreally pads(reach−1)/2per axis and clips to the sheet (:170-189);Unleakreally is erode→keep-the-piece-holding-the-box→dilate∩original (ContainerMask.cs:246-283) — "cut away, nothing repaired" is exactly what it does; the refusal really returns beforeApplyAsync, so theDialogueplaceholder fromadd_regionsurvives andlist_regionsprintsdialogue, form not judged— the exact string your test asserts;reject_regionwrites only the text fields and leaves Type/Form/Container standing, exactly as the escaped-container test pins. The sampler: cluster means,SameColorWithin = 40, box-only read — "AVERAGE", "about forty levels", "reads your own box" all check out, and the 58-level step is arithmetic-true (0xf1 − 0xb7). ♪Contains. The pins are directional, not decorative.whole-page-with-gutters.jpgfixture (flood + window replicated, per-channel tolerance 16, 20×170 box, seed 170,90): real per-axis Spread = 5.05 → refused; longer-side mutant = 1.235 → sails through as an ordinary container.A_fill_is_judged_on_the_worse_of_its_two_axesseparates them decisively — and that's the bug that motivated the whole size-rule rewrite, now impossible to reintroduce quietly.PageWorkspacePage.razor:227-240) really is the same threshold lever your note aims the human at. "Short is the way to be wrong" is backed on all four legs.💡 Little ideas (non-blocking)~
ContainerFillerPanelTests.cs:275—fit.Spreadisdecimal?, so the barefit.Spread > down * 2mworks via lifted comparison, but its neighbour usesfit.Spread!.Value— a!on one line and not the next is a tiny inconsistency; either lift both or assert non-null once.SkiaContainerFiller.cs:163-166— you flaggedWindow's own summary yourself as the real inconsistency (the "still reports a spread over the limit" claim is false at a sheet edge, now contradicted by a shipped brief + passing test). Seconding loudly: whoever owns that file next should take:163-166and:189-190together — the deferred-sweep list is the right call for this slice given the sibling-ownership constraint, but let's not let that one rot. ♡✅ What I liked~
add_region→ refusedclassify_region→list_regions.Contains-ing — prose and code break together, everywhere, on purpose.LooseSpread = 4left untouched, merge-order constraint stated and true (I checked the 4 changed files against the sibling-slice claims).Relay for bjoern: the open question at the bottom — which pass owns translator-pasted overlay lettering and a cover's display title/credits, ADR 0026 or an 0023 amendment — is a real fork with two friction reports behind it. This PR correctly doesn't decide it; the ruling shouldn't get lost when this merges.
Automated review by Jibril · September 20, 2026
CI/CD: absent for head SHA (PR opened 22:27, no run posted yet;
ci.ymlfires on pull_request) · Local checks: full suite 1321/1321 green, build 0w/0e, 2 mutation probes