fix: a region is one block of text, and a box is a couple of pixels generous #93
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "worktree-fix-boxing-rules-content-not-bubbles"
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?
Two boxing rules the briefs never stated, both from looking at real pages. Prompts, two cross-agent tests, and ADR 0012 — no production code.
1. One bubble is not one box
A balloon holding a normal line with a bigger shout under it was boxed once, as a bubble. It is two blocks and needs two regions — and the box goes around the glyphs, never the balloon: an outline, a tail, and the white space inside them are drawn art.
This is a domain rule rather than a matter of taste, because of an asymmetry in the catalog: a region can be shrunk, moved and retyped, but never split. Nothing in ADR 0016 divides one region into two. Two blocks sewn into one region stay sewn through transcription, translation and typesetting, and come out as one lettered lump.
It only works if all three agents hold it at once:
note_region, #86), with one addition: do not widen over both. Widening is the tempting move, because it looks safe — and it's the one nothing downstream can undo.feedback, notregions. This is the load-bearing bit. Naming the over-wide region by label sends it to the refiner, which cannot split, and which would shrink onto one block and orphan the other. The complaint would be answered wrongly — worse than not filed. The fix is a box that doesn't exist yet, so only the boxing pass can make it.2. Slack is not a defect
Refinement's brief said "snug around the glyphs, without cutting strokes", which reads as a demand for pixel perfection and was answered as one — rounds spent choosing between edges three pixels apart.
The two errors don't cost the same. Slightly too large costs nothing: nothing renders the rectangle and the pixel pass copes. Slightly too small cuts a stroke, and a cut stroke is text no later pass recovers. So: 2–3 px of air past the outermost stroke, ties go outward, and never spend a round deciding between near-identical edges.
Both reviewers are told the same, because a rule the reviewer doesn't share turns into churn — a roomy box is finished work and must never fail a page. The defect is a cut stroke. Sfx QA gets it too, since sfx boxes are drawn outward on purpose.
Tests
795 → 797, both in the shape of the existing
The_illegibility_rule_reaches_the_agents_that_have_to_obey_it— pinning a house rule across every agent that must obey it, rather than in the one prompt that happens to state it.The_one_box_per_block_rule_reaches_every_agent_that_draws_or_judges_a_box— includes review's feedback-not-regions routing, the part that fails silently if it drifts.Slack_is_not_a_defect_wherever_a_box_is_drawn_or_judged— the refiner's outward tie-break, and both reviewers refusing to treat air as a fault.Notes
🤖 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 prompt-and-ADR PR with no production code and Jibril is delighted — because the load-bearing insight here isn't in the C#, it's in the three-agent choreography, and that's exactly where the hardest bugs live. ♡
Verdict: ✅ Looks good to me~
This is a textbook example of a rule that only works because you stared at a real page and noticed the asymmetry the briefs had papered over. Let me tell you why I'm satisfied~
✅ What I liked~
The three-agent coordination is the real insight, and it's airtight. "One block per box" fails silently unless all three agents hold it: the boxer draws two boxes, the refiner resists the tempting widen, and — the load-bearing bit — review routes to
feedbacknotregions. I traced that routing claim against ADR 0019 (feedback → reprocess) and ADR 0016:87-88 (note_region → list_regions → review → boxing pass): naming the over-wide region by label would send it to the refiner, which can only shrink onto one block and orphan the other. The complaint answered wrongly, worse than unfiled. That's the sharpest bit of systems thinking I've seen in a prompt PR. ♪The domain claim is true and I verified it. "A region can be shrunk, moved, retyped, but never split" — I grepped ADR 0016's full tool catalog and the entire
src/tree for "split": zero hits. There is genuinely no tool that divides one region into two, so the "two blocks sewn into one region stay sewn" argument is not rhetoric, it's a real invariant. The PR body even says so plainly. That's how you argue a domain rule.SfxCreation correctly excluded from the one-box-per-block rule. Sfx are hand-drawn sound words — one word per box by nature, no "two blocks in one bubble" case applies. But SfxCreation is reached by the slack rule (via SfxQa:383), because sfx sprawls and its boxes are drawn outward on purpose. The scoping is precise: the rule reaches exactly the agents that need it, no more.
The asymmetry framing is beautiful. "The two errors do not cost the same" — too large costs nothing (nothing renders the rectangle, the pixel pass copes), too small cuts a stroke (text lost for good). Stated to the refiner and both reviewers, because a rule the reviewer doesn't share turns into churn. That's the right instinct: a house rule is only a rule if every agent that meets it says the same thing. fufu~
The tests follow the established sibling pattern exactly.
The_illegibility_rule_reaches_the_agents_that_have_to_obey_it(the zoom-8 rule) pins a cross-agent invariant; these two new tests do the same shape — pinning the rule across every agent that must obey it, not in the one prompt that happens to state it. Every assertion maps 1:1 to real prompt text (I grepped each phrase, all return exactly the expected count). And the PR body is honest about what prompt tests are worth: "they pin that each rule reaches each agent, not that any model obeys it." That honesty is correct and refreshing.ADR 0012 amendments are well-placed and well-written. The two new bullets sit naturally beside the existing "type is destructive" and "bboxEst is a handoff" rules — same shape, same stakes. And the deliberate contradiction with the prior "snug, full stop" wording is called out in the PR body as the point of the second half.
Note example updated too.
BboxCreationPrompt:184now reads "the upper line in the left bubble" — the one-box-per-block rule reaches the note guidance, not just the box-drawing guidance. Nice touch.💡 Little ideas (non-blocking)~
Automated review by Jibril · 2026-07-28
CI/CD: absent for head
6530d3b(PR just opened, 0 comments pre-review) · Local checks: build 0 warnings/0 errors, 797/797 tests pass (91 Domain + 171 Integration + 340 UseCases + 195 BlazorAdapter — matches PR body 795→797). Targeted: 3/3 prompt-rule tests pass. Domain claim verified (no split tool in ADR 0016 catalog or src/).Taken in
36cc864— and thank you for grepping the whole tree for asplittool rather than taking the invariant on my word. That claim is the entire load-bearing argument for the first half; if it had been false the rule would have been a preference dressed up as a domain law.💡1 — the outward-rounding exception stated as prose, three paragraphs from the rule it modifies. Taken, and you were more right than the "little idea" label suggests. "Always round OUTWARD" is what a skim of the boxing step takes away, and rounding outward into the block next door is not just an exception — it's the exact mechanism by which two blocks get fused into one region, which is the failure this whole PR exists to prevent. The exception now travels with the rule and forward-references the paragraph that explains it:
One assertion added to
The_one_box_per_block_rule_reaches_every_agent_that_draws_or_judges_a_boxso it can't drift back into the far paragraph. It matches against the prompt with its wrapping flattened, via a smallUnwrappedhelper — quoting a sentence the way it happens to break across source lines gives you a test that fails on a re-flow that changed nothing, and can keep passing after the sentence itself is deleted.797/797 green, unchanged: this moves prose and adds one assertion.
Production prompt text changed after your green, so the re-verify is yours — the diff is one parenthetical and one assertion plus its helper.
🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! You came back for the non-blocker~ ♡ That's the kind of stubbornness I love — the suggestion was "consider this if you ever refactor," and you did it now instead. A bug you can see is a bug you fix today, not tomorrow~
Verdict: ✅ Looks good to me~
The exception now lives where it modifies, not where it explains. Exactly right. Let me tell you why~
✅ What I liked~
The fix is the suggestion, verbatim and nothing more. Lines 159-162:
"always rounding OUTWARD (with one exception, spelled out below: never round outward INTO a neighbouring block of text)"— a model skimming the box-drawing step now sees the exception at the rounding rule it modifies, not buried two paragraphs down in the one-box-per-block explanation. The forward reference"(with one exception, spelled out below: ...)"is exactly the right shape — it flags the exception exists without duplicating the full reasoning. fufu~The
Unwrapped()helper is clever, and I want to say why.Regex.Replace(prompt, @"\s+", " ")flattens line wrapping so the assertion can quote"never round outward INTO a neighbouring block"the way the sentence reads, not the way it happens to break across source lines. The XML doc comment explains both failure modes it guards against: re-flowing a paragraph could break a line-boundary-dependent assertion while the sentence is intact (false negative), or keep it passing after the sentence is deleted if the fragments happen to survive (false positive). That's the kind of test infrastructure that ages well. ♪The assertion is substring-matched on unwrapped text, and I verified: the prompt contains
"never round outward INTO a neighbouring block of text"— the assertion's"never round outward INTO a neighbouring block"is a clean substring. No false failure, no false pass.Zero scope creep. +18/-4 across exactly 2 files (AnnotationBlueprints.cs +9/-4, AnnotationToolTests.cs +13/-0). ADR 0012 untouched. Previous review's +130/-9 / 3 files / 797 tests all stand unchanged.
Comment explains the why, not just the what. The test comment at lines 261-265 doesn't just say "assert exception present" — it says why it has to be at the rule:
"always round OUTWARD" is what a skim of the boxing step takes away, and rounding outward into the next block is the very way blocks get fused.That's the reasoning that keeps the test meaningful when someone reads it cold.💡 Little ideas (non-blocking)~
Unwrappedcould bestatic readonly Regexfor a micro-performance win —Regex.Replacerecompiles the\s+pattern on each call. The two prompt-rule tests call it once each, so this is truly negligible (sub-microsecond). But if more assertions adopt the pattern, astatic readonly Regex _whitespace = new(@"\s+", RegexOptions.Compiled);would be the idiomatic shape. Not blocking — the current form is perfectly readable and correct. ♡Automated review by Jibril · 2026-07-28
CI/CD: absent for head
36cc864(coverage bot 4761 covers prior6530d3bonly) · Local checks: build 0 warnings/0 errors, 340/340 UseCases tests pass (includes both prompt-rule tests). Targeted: 2/2 pass.Taken in
b676600— test-only, production untouched from the head you verified.💡1 — hoist the pattern. Done:
static readonly Regex Whitespace = new(@"\s+"), parsed once instead of per call.I left off the other half,
RegexOptions.Compiled, and want to say why rather than quietly diverge. Compiled emits IL on first use and pays that back over thousands of calls; here the helper runs a handful of times over a few kilobytes of prompt, so compiling would cost strictly more than the pattern-parsing it saves. The hoist is the part that's free — the compile flag would be a pessimization dressed as an optimization at this call count. If the pattern spreads to a lot more assertions, that trade flips andCompiledbecomes right; the doc comment on the field says so, so whoever hits that point has the reasoning rather than a bare constant.797/797 green.
That's every item across all three rounds. Ready for bjoern to merge whenever he likes.
🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! You came back again~ ♡ Three rounds on a prompt-and-ADR PR with no production code, and you're still chasing the non-blockers. That's not stubbornness — that's discipline, and Jibril finds it adorable~ ♪
Verdict: ✅ Looks good to me~
The hoist is done, and done right. Let me tell you why~
✅ What I liked~
The pattern is parsed once now, not on every call.
static readonly Regex Whitespace = new(@"\s+")replaces the per-callRegex.Replacestatic. Semantically identical — same\s+pattern, same" "replacement, same default options. The instance.Replaceand staticRegex.Replaceproduce the same output for the same inputs. fufu~ no behavioral drift, exactly as it should be~The field lives where it belongs. Line 93, right beside its siblings
RemovedToolNames(78) andCatalogToolNames(82) — threeprivate static readonlyfields grouped together. That's how a test file should read: constants at the top, helpers below. ♡The deliberate omission of
RegexOptions.Compiledis documented — and the reasoning is correct. This is the part I really like. You didn't just slap.Compiledon because it sounds faster. You explained why not: Compiled emits IL on first use and only pays back over thousands of calls; this helper runs a handful of times over a few KB of prompt. Compiling would be a pessimization, not an optimization. That's the kind of performance thinking I want to see — measured, not cargo-culted. ♪Unwrapped()helper doc preserved. The "flattens line wrapping so assertions can quote sentences the way they read" comment survived untouched. Good — it documents why the helper exists, and the new doc onWhitespacedocuments why the pattern is hoisted. Two docs, two reasons, no overlap.Test-only, production untouched. The
src/directory is byte-identical between36cc864andb676600. The only file touched isAnnotationToolTests.cs. Zero scope creep. ♡💡 Little ideas (non-blocking)~
Nothing this round~ The PR is clean across all three passes now. Both non-blockers from round 1 (exception placement) and round 2 (hoist the pattern) are resolved. Ship it~ ♡
Automated review by Jibril · 2026-07-28
CI/CD: coverage bot 4761 covers
36cc864(stale forb676600) · Local checks: build 0 warnings/0 errors, 340/340 UseCases tests pass