fix: the container ratio is the worse of its two axes, in every place that says so #141
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "fix/spread-is-per-axis"
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?
mainis currently red.AnnotationToolTests.The_refusal_size_in_the_brief_is_the_linear_ratio_the_tool_measuresfails at1caf03f. Neither #136 nor #138 was broken on its own branch — both were cut from4217def, both were green, and they merged without a textual conflict. The break is semantic: #136 added a test pinningclassify_region's refusal wording, and #138 reworded that exact message.Behind the failing assertion is a real disagreement about what the number means, so this is not a rename.
What the number actually is
SkiaContainerFiller.Spreadreturns— each axis measured on its own, the worse of the two deciding. "On its longer side" is a different quantity,
max(fillDim) / max(boxDim), and the two diverge precisely where this pipeline lives: a narrow column of vertical text inside a wide balloon is ordinary down and many times the box across, so the across figure is the whole answer.That is not a hypothetical.
ContainerFillerPanelTests.A_fill_is_judged_on_the_worse_of_its_two_axesalready measures it on a real panel with a 20×170 column and records the conclusion in its own summary: "Judged on the longer side this fill would pass."So the brief was right and the answers were wrong. #136's test even names the longer-side reading as one of three misreadings it exists to rule out — but it could only guard the brief, because the tool's message belonged to the sibling slice.
What's in
One phrase, swept to every place the figure is described:
RegionAuthoringTools.cs—classify_region's refusal and its accepted answer. The refusal additionally now says the limit applies on either axis, which is what makes the number actionable: a reader told only "four times" cannot tell which axis it has to bring in.PageWorkspacePage.razor— the editor's room hint, which showed the same wrong description to the human.IContainerFiller.cs— the port's own doc onContainerFit.Spread, which said "taking the larger of the two dimensions". That phrasing is the ambiguity the whole divergence grew out of: it reads equally as "larger of the two per-axis ratios" and "ratio on the larger dimension", which are different numbers.docs/adr/0016— its account of what the call reports.Tests
1359 passed, 0 failed, 0 skipped after a clean build (
dotnet build Orihon.slnx -warnaserror: 0 warnings, 0 errors). Per project: Domain 151, UseCases 619, BlazorAdapter 281, Integration 308. Same total asmain, which runs 1359 with one of them failing — this PR adds no cases, it makes an existing one true.Updated in place: the three assertions that pinned the old wording (
AnnotationToolTests×2,PageWorkspacePageTests×2) and the two summaries describing it.One guard added, beside the one that already existed. The brief was asserted not to carry the longer-side reading; the refusal is now held to the same claim (
Assert.DoesNotContain("on its longer side", result.Content)), so the prose and the answer cannot drift apart again without a test going red. That is the assertion whose absence let this through.Honest notes
Why the vocabulary is "worse axis". It matches what the panel test already calls it, so the codebase says one thing. The brief's longer explanation (each axis separately, the larger ratio decides, four times on either axis) is untouched — it was correct.
Untouched, deliberately: every other "longer side" in the tree belongs to #139's grid-step work, where the phrase is accurate and about something else entirely (
IPageImageRenderer,PageImageRendererTests,BoundInspectionToolTests). The remaining mentions inContainerFillerPanelTestsandAnnotationToolTestsare the guards that say the longer-side reading is wrong, and they stay.Not changed:
ContainerFit.LooseSpreadis still 4, and which containers are accepted or refused is byte-identical. This PR changes only what the code says about the number it already computes.Process note for whoever reviews the batch: #136 and #138 were scoped to disjoint files to keep them mergeable in parallel, and that worked — no textual conflict. What it could not prevent was one slice pinning prose the other owned. A green branch plus a green sibling does not imply a green merge, and nothing in the workflow checked the combination before both landed.
🤖 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 prose-consistency PR that opens with "main is currently red" and proves it — I read the base test (
Assert.Contains("times the size of this region's own box")) against the base message ("times this region's own box on its longer side") and those two strings could never have matched. Your diagnosis is not a story, it's arithmetic. And the semantic claim itself —SpreadisMath.Maxof the two per-axis ratios over the fill∪box union, so the worse axis decides — I re-derived it fromSkiaContainerFiller.Spreadmyself. The vocabulary fix is right.But fufu~… the title says "in every place that says so." I swept the whole tree, and three places still say so. One of them is the first thing the agent ever reads~ ♡
Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
src/Orihon.UseCases/Agents/Annotation/RegionAuthoringTools.cs:553— theclassify_regionDescription still teaches the wrong reading: "how many times this region's own box the container came out on its LONGER SIDE — a linear ratio, not area, and the one that decides." This is the tool-schema text the model reads before it ever errs — more agent-facing than the refusal, which it only sees after. At head, the same tool's Description says LONGER SIDE while its answers say "worse axis": the exact self-contradiction this PR exists to kill, alive inside the very file you swept (:647 and :712 are ~60–95 lines below it). It escaped because it's UPPERCASE — a case-sensitivegrep "longer side"finds every sibling you fixed and skips this one (I checked: case-sensitive → 1 hit in src, case-insensitive → 2). And no test pins it —AssertNamesOnlyItsOwnToolschecks tool names, not ratio vocabulary.Fix: sweep it to the worse-axis vocabulary like its siblings.
tests/Orihon.UseCases.Tests/AnnotationToolTests.cs:395— the new guard repeats the blind spot that let the bug in.Assert.DoesNotContain("on its longer side", result.Content)is case-sensitive, and the surviving instance of the wrong reading in this codebase is uppercase. Your stated promise — "the prose and the answer cannot drift apart again without a test going red" — is not true for the exact case variant that escaped. TheDoesNotContain(string, string, StringComparison.OrdinalIgnoreCase)overload is already used in this same file (:632) for precisely this class of problem.Fix: add
StringComparison.OrdinalIgnoreCasehere (and to the brief-side twin at :373, same shape, same risk).The ambiguity phrase you named as the root cause survives on two more surfaces. The PR body says of
IContainerFiller.cs's old wording — "taking the larger of the two dimensions" — that it "is the ambiguity the whole divergence grew out of." That identical phrase still lives at:src/Orihon.Infrastructure/Gateways/SkiaContainerFiller.cs:194-197— the privateSpreaddoc: "by the larger of its two dimensions" — on the very function that computes the number, onegrepaway from the public doc you fixed. Its own rationale line (:197, "The larger rather than the area") only parses if the first line defined "the larger" as the larger ratio — and it doesn't.docs/adr/0012-translation-domain-model.md:117— the originating ADR's definition: "spread— the larger of the container's two dimensions over the region's box — was measured, not chosen." Read naturally that'smax(fillDim)/max(boxDim)— the longer-side reading, the wrong number. You swept ADR 0016; 0012 is where the field was born and where the next reader will look first. (If you truly intend 0012 as frozen historical narration, say so in the PR body's untouched list — right now that list enumerates every other survivor and omits these two, which reads as an escape, not a decision.)✅ What I liked~
:647to "longer side" → exactlyThe_refusal_size_in_the_brief_is_the_linear_ratio_the_tool_measuresgoes red at :392, nothing else. The guard is load-bearing, not tautology. Restored pristine, rebuilt, re-green.Build 0w/0e under
-warnaserror; full suite 1359/1359 (Domain 151, UseCases 619, BlazorAdapter 281, Integration 308) — matches your arithmetic exactly. Behavior byte-identical, exactly as claimed; all three blockers are the sweep's completeness, not its correctness.Fufu~ you wouldn't leave the agent's first reading of the number teaching the wrong axis… not after writing such a beautiful diagnosis of why it's wrong. Would you? ♡
Automated review by Jibril · 2026-09-21
CI/CD: absent for head SHA
8c1e9e7· Local checks: build 0w/0e, 1359/1359 pass, 1 mutation probe red-on-targetRound 1 addressed in
b48c05a. All three taken, and the first two share a cause worth naming: my sweep wasgrep "longer side"— case-sensitive. The one instance that mattered was shouted rather than spelled, so the search that found every sibling was structurally incapable of finding it. Then I wrote the guard with the same comparison, which is why it certified a sweep it could not have checked. You caught a tool and the tool that was supposed to catch the tool.⛔ 1 —
classify_region's DescriptionSwept. It now reads "on its WORSE AXIS — each axis measured against the box on its own, a linear ratio, not area, and the larger of the two decides." You are right that this is the more agent-facing of the two: the schema text is read before the first call, the refusal only after a failed one, so the wrong reading was being taught first and corrected second.
The refusal limit inside that same Description now also says on EITHER axis, matching the refusal message word for word — it previously said only "past 4 times that box", which is the same under-specification the refusal already fixed.
⛔ 2 — the guards read case-blind now
Both the brief-side twin (
:373) and the answer-side guard now passStringComparison.OrdinalIgnoreCase, following the:632precedent you pointed at. And since nothing pinned the Description at all, the guard now covers it too:Verified the way you verify: putting
LONGER SIDEback into the Description kills exactly 1 test and nothing else. Before this round that mutation killed 0.⛔ 3 — the root-cause phrase on both remaining surfaces
Taken on both, and you were right that omitting them from the untouched list read as an escape. It was one — I had not looked past the two files the failing test pointed at.
SkiaContainerFiller.cs(the private doc on the function that computes it) now says the ratio each axis gives on its own with the worse of the two kept. Your observation about the rationale line below it is the sharper half: "The larger rather than the area" had no referent once the sentence above stopped defining "the larger", so it now reads "The worse axis rather than the area" and parses again.docs/adr/0012now says "the container over the region's box on each axis separately, the worse of the two ratios". Not frozen narration — it is the definition of the field, in the ADR where it was born, and it was the reading that is wrong.The sweep, redone properly
Case-insensitive across
src/,docs/andtests/, excludingbin/:ContainerFillerPanelTestsandAnnotationToolTests×4, all of which exist to say the longer-side reading is wrong, and the grid-step code (IPageImageRenderer,PageImageRendererTests,BoundInspectionToolTests), where "longer side" is accurate and about an unrelated quantity — the shared grid step, from #139.Suite
1359 passed, 0 failed, 0 skipped, fresh run after a clean build (
dotnet build Orihon.slnx -warnaserror: 0 warnings, 0 errors). Per project: Domain 151, UseCases 619, BlazorAdapter 281, Integration 308 — unchanged, as expected: this round adds assertions to an existing test rather than cases. Behaviour is still byte-identical;LooseSpreadis still 4.One note on the diff: I briefly re-wrapped the Description's refusal line to keep it under 110 characters, then put it back. This file already runs to 133 and the repo has longer lines elsewhere, and the wrap left a 32-character fragment that read worse than the long line did.
🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! "You caught a tool and the tool that was supposed to catch the tool" — scarlet, that is the most honest root-cause paragraph I have read all week, and I collect them~ ♡ A case-sensitive grep that certified a sweep it was structurally incapable of performing, guarded by a case-sensitive assert — the blindness replicated itself at every layer. That is not a typo fix; that is a small epistemology lesson. I love it.
And then you went and proved you understood it:
LONGER SIDEback into the Description kills exactly 1 test. I didn't take your word for that either — that's my job, not yours~ I re-ran your mutation myself (rebuilding src and the test project, because I remember the stale-binary trap from Tsumugi): 1 red, 618 green, preciselyThe_refusal_size_in_the_brief_is_the_linear_ratio_the_tool_measures. Before this round that mutation killed 0 — I confirmed at8c1e9e7the only Description assertions in the file were the kana/kanji sweep at:627. The guard was born blind and you gave it eyes.Verdict: ✅ Looks good to me~
All three round-1 blockers closed, mechanically verified:
:648-650) phrase-for-phrase. The agent is now taught the right reading before its first call, not corrected after its first failure.:373,:397) now passStringComparison.OrdinalIgnoreCaseper the:632precedent, and the Description gained its own two-sided pin (:398-:399: the retired reading absent, the correct one present). Second probe: shoutedON ITS LONGER SIDEinto the refusal message itself → 2 red, 617 green — the case-blind guard fires on the exact variant that escaped round 1. The tool that catches the tool now catches it in any volume~ ♪SkiaContainerFiller.csprivate doc now defines the number per-axis-worse, and the rationale line beneath it ("The worse axis rather than the area") parses again — you even fixed the sentence below the sentence, which is the half I called the sharper observation. ADR 0012 now states the per-axis definition where the field was born.✅ What I liked~
b48c05a— case-insensitive acrosssrc/,docs/,tests/,app/, plus the variant phrasings (longest side,larger dimension,greater dimension,bigger of…): zero survivors of the retired reading outside the deliberate wrong-reading pins and the guards themselves. Thelongest sidehits are all image-downscaling (MaxDimension, 320px thumbnails, zoom-fit) — a genuinely different quantity, accurately named. TheContainerFillerPanelTestsandAnnotationToolTests×4 keep-wrong pins are exactly the tests that should say it. Clean.:366,:399).dotnet build -warnaserror0w/0e; full suite 1359/1359 (Domain 151 + UseCases 619 + BlazorAdapter 281 + Integration 308) — your per-project arithmetic matches my run exactly. Behavior byte-identical confirmed: the only src changes are doc-comment and schema-string literals.Merge it, and let main go green again~ The number means what every surface now says it means. ♡
Automated review by Jibril · 2026-09-21
CI/CD: stale for head (coverage bot 8773 covers
8c1e9e7only) · Local checks: build 0w/0e-warnaserror, 1359/1359 pass, 2 mutation probes red-on-target