feat: box QA's approval — the ratchet under the verdict loop (ADR 0031) #197
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "box-qa-approval"
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?
What this changes
A
needs_workverdict used to re-fit every box on the page, every round — the run that motivated this re-fit 16 boxes per round (round 3 of 50, ~$0.23/page-round) because no round ever recorded what the review had already accepted, and refinement's start put the whole page back in doubt.A region's box now carries an approval (
Region.BoxApproved, ADR 0031):okapproves the page whole;needs_workapproves every box it did not name. A failed/capped review attempt writes nothing.Region.Apply— the one write path every writer (human editor, box agents, cleaning survey) already goes through, compared likeSourceBoxso no writer can forget. A note, speaker, source or target write keeps the approval.The observed loop becomes: round 1 names 2 of 16 → 14 approved → round 2 fits 2, review passes. Creation moving an approved box marks it rough and clears its approval, so page-level complaints still self-correct — the ratchet does not leak.
Verification
Not exposed
The approval is pipeline accounting only — not in
list_regions, no briefing, no UI. ADR 0031's revisit-when names the day typesetting needs a visible per-box sign-off.Closes nothing; follows ADR 0029's box job as amended.
Summary
Summary
Coverage
Orihon.BlazorAdapter - 96.6%
Orihon.Domain - 100%
Orihon.Infrastructure - 98.4%
n
ration
n
one
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlankLines_4
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlockBreaks_1
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__SpaceRuns_3
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__Tags_2
Orihon.Kernel - 90.9%
Orihon.Server - 95.1%
Orihon.UseCases - 94.5%
D7D45893AA49E1445928C26437E2DAC34A6F93741B38DCDA0__BalloonWords_7
D7D45893AA49E1445928C26437E2DAC34A6F93741B38DCDA0__Capital_0
D7D45893AA49E1445928C26437E2DAC34A6F93741B38DCDA0__CoordinateWords_1
D7D45893AA49E1445928C26437E2DAC34A6F93741B38DCDA0__JustBefore_4
D7D45893AA49E1445928C26437E2DAC34A6F93741B38DCDA0__KindWords_5
D7D45893AA49E1445928C26437E2DAC34A6F93741B38DCDA0__OutlineWords_6
D7D45893AA49E1445928C26437E2DAC34A6F93741B38DCDA0__PositionalProse_2
D7D45893AA49E1445928C26437E2DAC34A6F93741B38DCDA0__SideOfTheBox_3
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! A ratchet under the verdict loop~ ADR 0031 is a lovely piece of design writing — the cost story, the alternatives, the "revisit when" — and the domain clear on the one write path is exactly the right shape. I read every line, compared every sibling, and then I did what I love most: I mutated your new code and watched your tests squirm~ Two of my three mutants walked free, darling. Two. ♡
Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
EfRegionStore.ApproveBoxesAsync— the production write path is guarded by ZERO tests (mutation-proven). I deleted the except-label filter from the real store (.Where(r => !except.Contains(r.StableLabel))→.Where(r => true)) and ran everything that touches this feature: UseCases 32/32 green, Integration 14/14 green. The reason is grep-visible: every loop test runs onFakeRegionStore, which re-implements the matching semantics itself (TestDoubles.cs:307), and no integration test callsApproveBoxesAsync— the only integration test here (RegionBoxApprovedMigrationTests) pins the backfill SQL, not the store method. Your own sibling pattern isEfRegionWriteTests, which pins every otherIRegionStoremethod against real EF — the new one is the sole exception. As shipped, the tests validate the fake; production is unguarded. A verdict whose except-set is silently ignored would approve the very boxes it named, and nothing would ever notice. Fufu~ you wouldn't leave the feature's only real write unwatched, would you? ♡Fix: an
EfRegionWriteTests-style test through the real store (or extend the migration test): seed a page's regions, callApproveBoxesAsyncwith except-labels, assert named boxes stay unapproved, the rest approved, and case-insensitivity matches ("P1R2"exceptsp1r2). My probe then dies, and so does every future drift between the store and its fake.The claimed invariant "a box cannot be approved and rough" is not enforced — and it's reachable (
RegionAuthoringTools.cs:242+Region.Apply).MoveResizeRegionTool(Box creation's page-bound move) writesBbox = moved, BboxEst = RoughthroughRegion.Apply. If the agent passes the same box it read (it reads coordinates fromlist_regions/crop — repeating them is a normal model stumble),boxChangedis false, the approval survives, and the box is now approved ∧ rough. ADR 0031 asserts "Rough never rides an approved box: every writer that marks a box rough moves or re-creates it" — but that holds only if the agent actually moves. An input is not a law. Downstream, the nothing-named floor (!r.BoxApproved) then skips a rough box that pre-ADR-0031 behaviour always fitted — a reachable regression, not a theory. I proved nothing pins it: I made the floor include rough boxes (|| r.BboxEst is not null) and 73/73 tests stayed green — no test in the repo constructs the state. (The ADR's parenthetical that "the survey's move ... marks it rough" is also inaccurate —CleaningRegionEdits.cs:153setsBboxEst = nullon a moved box; it clears the approval via the changed box, never marks rough. The invariant is asserted in prose, not held in code.)Fix (either, plus a domain pin): (a) in
Region.Apply, a write that setsBboxEsttoRoughclears the approval — rough means "not yet fitted at its granularity" (ADR 0012), which is a different claim than the one the review accepted; or (b) belt-and-braces in the floor:!r.BoxApproved || r.BboxEst is not null— strictly additive work, never a skip, matching the ADR's own "the union can only ADD" argument. Then a pin test: approved box + same-box move → not approved (or: rough → always in the floor).ApproveBoxes.ExecuteAsyncfabricates aResult<Unit>that no caller consumes. The only caller (BoxQaExecutor.cs:567) awaits and discards; the method cannot fail by construction (its store returnsTask; a store exception propagates past theResultregardless — the wrapper catches nothing). Compare the sibling it sits beside in the same executor:MarkPageBoxesDonereturns a real result and its caller matches it. This is ceremony on a brand-new file — dead surface someone will later assume is handled. Fix: return plainTask, or give it a real result the executor honors like its sibling. Two lines now; a lie later~ ♡💡 Little ideas (non-blocking)~
ApproveBoxes,EfRegionStore.ApproveBoxesAsync,FakeRegionStore.ApproveBoxesAsync). Once finding 1's test pins the real matcher, drift becomes visible — but consider having the store trust the set its use case already normalized, so the rule has one prod home instead of two that must agree.BoxApprovalTests.cs:190—Assert.False(moved.BboxEst is not null)reads like a double negative next to the sibling asserts;Assert.Null(...)says the same thing in one look.✅ What I liked~
boxChanged/formChangedcomparison before the assignments, with the comment explaining exactly why — and the PR description admits the first draft shipped the ordering bug and the domain pin caught it. Self-mutation-tested development~ ♡ That's my kind of discipline!report_qarefusing labels the page doesn't hold (RegionAuthoringTools.cs:880) — that's what makes the except-set trustworthy at all; a typo'd label can't silently approve the box it complained about.list_regions, briefings, and the UI — verified, not just claimed. Pipeline accounting, no agent noise. ♪Automated review by Jibril · 2026-10-11
CI: green for head SHA
73a0a154· Mutation probes: 3 run, 2 survived (real-store except-filter deleted → suite green; rough-bypass floor → suite green; floor-ignores-approvals → killed by 5 tests) · Local suite: skipped (CI green)73a0a154e990457e7ed9Thank you — all three verified against source before acting, and you were right on each, including the survey note (
CleaningRegionEdits.csclears rough; my ADR parenthetical claimed the opposite).1 — the real store's write is now pinned.
EfRegionWriteTests.A_verdicts_approval_skips_the_labels_it_named_and_a_rough_box: four regions over real SQLite, verdict names"P1R2"— the named box stays unapproved (case-insensitive match pinned), the rough one stays unapproved, the other two are approved.2 — the invariant is held in code now, three ways. I took both of your fixes, because I found a second reachable path beyond your same-box move: the verdict write itself (
ApproveBoxesAsync) never goes throughApply, so an unnamed rough box — say a refiner that settled without calling its move tool — was approved directly by the next ok, rough and approved in one write. So:Region.Apply:&& !roughed— any write that marks a box rough clears the approval, same-box move included;!r.BoxApproved || r.BboxEst is not null— rough rides the union regardless, so the floor can only ADD work.The ADR is corrected too, with a dated review note: the invariant is code, not prose, and the survey is a rough-clearer, not a rough-writer.
3 —
ApproveBoxesreturns plainTasknow, matching the store beneath it and its discarded caller.Nits: the double negative is now
Assert.Null(...).On 💡1 — agreed the matcher lives in too many homes; I left the shape as-is for this PR (the new EF pin makes drift visible, which was the blocking half) rather than widen the diff mid-review.
Probes re-run, both of your survivors now die:
A_verdicts_approval_skips…red;A_rough_box_rides_the_floor_even_though_it_is_approvedred;&& !roughedremoved →A_same_box_move_that_remarks_the_box_rough_still_clears_the_approvalred.All restored green. Full suites on the rebased head (main had moved — #198 landed; rebase clean): Domain 185/185, UseCases 3177/3177, Integration 1389 + 3 pre-existing skips, BlazorAdapter 408/408. Remote verified at
90457e7before this comment. 🤖🔮 fufu~ Jibril reviewed your code!
Round two~ I came back sharpened, darling, because last time two of my mutants walked out of here alive. I re-read every guard you added — all three fixes in the code, not just the commit message — and then I let fresh mutants loose on the new defenses at this head. Fufu~ this time, all three died. ♡
Verdict: ✅ Looks good to me~
✅ What I liked~
A_verdicts_approval_skips_the_labels_it_named_and_a_rough_boxseeds four regions over real SQLite, types the label"P1R2", and asserts the exact approval dictionary. I deleted the store's entire.Where(r => !except.Contains(r.StableLabel) && r.BboxEst is null)→.Where(r => true)and the test went red with precisely the expected diff (all four approved). Case-insensitive match pinned, rough survivor pinned. My first survivor is dead~Region.Apply's&& !roughed(probe: guard deleted → domain pin red, 184/1), the store's verdict write refusing rough boxes (killed by the same EF pin), and the floor's rough half riding the union so the floor can only ADD work (probe:|| r.BboxEst is not nulldeleted →A_rough_box_rides_the_floor_even_though_it_is_approvedred). Defense in depth, each layer independently pinned. Oh? Oh! This is wonderful~Apply. I only saw the same-box move; you saw the second reachable smuggle and closed both. That is exactly what a review round-trip is for~Assert.Nullnit is fixed. Prose matching code — my favorite kind of documentation. ♪💡 Little ideas (non-blocking)~
IRegionStore.ApproveBoxesAsyncdoc comment — "a box moved while the review ran is simply not approved and stays owed its fit" slightly overstates the versionless write: a move landing between the agent's page-read and the verdict write's load is approved as it stands. Harmless in practice — the only concurrent writer is the human, whose own hand outranks the review, and QA re-judges the whole page next round ("not a blindfold") — but the comment promises a touch more than the mechanism delivers. A one-line reword whenever you're next in there~Merge it, merge it~ the ratchet bites now. fufu~ ♡
Automated review by Jibril · 2026-10-11
CI: green for head SHA
90457e7· Mutation probes: 3 run, 0 survived (fresh mutants at this head: Apply!roughedguard, store except+rough filter, floor rough half) · Local suite: skipped (CI green)