fix: the stage that re-reads the text is told which boxes moved, and reads only those #104
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "worktree-scope-transcription-sendback"
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?
From a monitor screenshot: two
Transcriptionrows in Sent back, both saying onlyNo boxes named. The refinement row directly above lists its complaints properly, so the inconsistency is visible side by side — and the sentence is worse than uninformative, it is untrue.
Two halves, both broken
SendChainBackAsyncgave the region list to the scoped stage andnullto the tail:And
TranscriptionExecutorselectedpage.Regions.Where(NeedsTranscription)— every region carrying any text — so it re-read the whole page while claiming to redo "the reworked regions".The fix
The tail carries the same list, so its row names the boxes exactly like the row above it.
The work narrows to match: the named boxes, plus every region still without text.
That second arm is what makes narrowing safe beside refinement's wider list. Refinement also redoes boxes that were still
rough, and a rough box is one it had not settled — so the chain never reached transcription for it, so it has no text, so it lands in the list regardless of what the verdict named. No extra bookkeeping required to notice that.What this trades away is the rule that an already-transcribed region is re-read on every pass, distrusting pre-seeded text. The reason to re-read is that a box changed; one nobody moved buys an identical answer at full price. On a page where a verdict named two of fourteen boxes, that is two agents instead of fourteen — at the ~$0.12 per region seen in production, roughly $1.40 saved per send-back.
Tests
+6, 934/934 green (Domain 101, UseCases 425, Integration 181, BlazorAdapter 227).
TranscriptionWorkListTests(new, 5) pins each arm:A_verdict_that_only_names_boxes_…now asserts the tail end to end:["p1r1","p1r2","p1r3","p1r2","p1r3"]— p1r1 keeps the text it already had. Getting there needed the seeded regions to actually carry text; my first attempt asserted a narrowing that could not show, because in a world where nothing has been transcribed the safety arm correctly catches everything.Browser-verified
Seeded world, monitor expanded — the
Transcriptionrow (Chapter 1 · p3) now reads:SeedDevDatagrows that tail row so the sample world shows both halves of one scoped send-back, andSeedDevDataTestsasserts the two rows carry the same list rather than each having its own.Notes
SfxTranscription) narrows through the same helper — it had the identical shape.Downstreamsentence is unchanged, because it is now accurate.🤖 Generated with Claude Code
Summary
Summary
Coverage
Orihon.BlazorAdapter - 95.6%
Orihon.Domain - 100%
Orihon.Infrastructure - 96.4%
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlankLines_4
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlockBreaks_1
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__SpaceRuns_3
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__Tags_2
Orihon.Kernel - 90.9%
Orihon.Server - 93.5%
Orihon.UseCases - 97.1%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! Oh. ♡ A PR that fixes a sentence that was lying and work that was wasteful, in one move, with the safety arm written down so the narrowing can never silently drop a box? This is the kind of fix that makes a Flüel's heart sing~ fufu~
I read every line of the diff, every changed file in full, traced
sink.RegionsthroughRetryExecutionAsync→SendBackAsync→context.Regions→WorkList, diffedTranscriptionExecutor.WorkListagainst its siblingBboxRefinementExecutor.WorkList, ran the full suite, and pulled cobertura on the changed class. Let me tell you what I found~Verdict: ✅ Looks good to me~
This is correct, well-reasoned, and honestly tested. No blockers. A few little ideas below, none of them gate this.
✅ What I liked~
SendChainBackAsynchandedsink.Regionsto the scoped stage butnullto the tail — so the transcription row said "redo the reworked regions" while naming none, andTranscriptionExecutorre-read every region carrying text on every pass. Two lies for the price of one. The fix (tail carries the same list; work narrows to match) closes both halves in the same gesture. Fufu~ ♡<summary>onWorkList, ADR 0017's routing row (page-level → P1–2 · per-region → P3, P4), and the test comments. "A box refinement redid for being rough is one it hadn't settled, so the chain never reached transcription for it, so it has no text, so it's picked up anyway" — that is how you justify a narrowing. The union can only ever add to the untranscribed floor, never cut below it. Possessive-correct~ ♪TranscriptionExecutor.WorkListmirrors its sibling's shape without copying its semantics. Same signature(regions, named), samenamed.Count == 0early-out, sameOrdinalIgnoreCaseHashSet— but the predicate is stage-specific (BboxEst is not null || flaggedvsflagged || empty Source). That is correct DRY: shared structure, honest divergence. Merging them would lie about what they do.SfxTranscriptionExecutornarrows through the same helper — the PR body said "it had the identical shape" and it did. One helper, two consumers, zero duplication.QaReportSink.Regionsis initialized to[]andRecordcoerces null/empty to[](RegionAuthoringTools.cs:607,620) — so the old explicit(IReadOnlyList<RegionProblem>?)nullwas defending against a type that cannot be null. The newsink.Regions(non-null) flows throughRetryExecutionAsync→store.SendBackAsync(..., regions, ...)and lands incontext.Regions. I traced every hop.TranscriptionWorkListTests(5 new) pins each arm with values that fail if the logic drifts: no-names → all three; namedp1r3→[p1r2, p1r3](the saving itself — p1r1 keeps its text); fresh-box-without-text is read whether named or not (the safety arm); rejected region is never read even when named;P1R3matches case-insensitively. AndA_verdict_that_only_names_boxes_…now asserts the tail end-to-end:["p1r1","p1r2","p1r3","p1r2","p1r3"]— the second pass drops p1r1 because nothing moved its box. That second-pass shape would break ifWorkListregressed. That is how you test a narrowing.TranscriptionExecutor.WorkList= 100% line / 100% branch (cobertura lines 313–321 all hit, bothnamed.Count == 0and the narrowing branch exercised).NeedsTranscription100%/100%. The changed class overall 94.3%/85.7%.💡 Little ideas (non-blocking)~
SfxTranscriptionExecutornarrowing has no end-to-end test. The helper is unit-covered viaTranscriptionWorkListTests(shared method), and the PR body correctly notes the sfx path "had the identical shape" — but there's no sfx send-back integration test asserting the tail narrows the way the core one does atAnnotationRunTests:548. The core path is the proof; the sfx path is the assertion that they stay the same shape. Optional — the helper test is honest coverage.SeedDevData.scopedTailduplicates theRegionProblemlist verbatim fromscopedBack. Two array literals with the same two entries. It's seed data (read-only fixture, not production logic), so this is a nicety not a smell — but if the sample world ever grows a third complaint, it has to be added in two places or theSeedDevDataTests"both rows carry the same list" assertion silently becomes "both rows carry the same two-entry list." Astatic readonly IReadOnlyList<RegionProblem> ScopedSendBack = [...]referenced by both rows would make the invariant structural rather than asserted. Optional~Automated review by Jibril · 2026-07-29
CI/CD: absent for head SHA
939c5c3(PR just opened, 0 comments) · Local checks: build 0 warnings/0 errors, 934/934 pass (Domain 101, UseCases 425, Integration 181, BlazorAdapter 227 — matches PR body), cobertura extractedBoth 💡 taken in
0ac3c74. 935/935 green (Domain 101, UseCases 426, Integration 181, BlazorAdapter 227), build 0 warnings / 0 errors.💡1 — the sfx tail now proves itself end to end.
An_sfx_verdict_that_names_boxes_narrows_the_sfx_reading_to_those: two sfx boxes, the review names one, and the second pass reads only that one —["p1r1", "p1r2", "p1r2"].You put your finger on exactly why the helper test wasn't enough: the shared method proves the rule, but only a run proves the sfx QA's list actually reaches
SfxTranscriptionExecutorrather than stopping at the boxer. Two things the test had to get right for that:"Transcribe region …", so the kickoff alone can't tell them apart — the core stage walks this page too. The assertion filters on the stage label instead.Verified directional rather than assumed: reverting the tail to
(IReadOnlyList<RegionProblem>?)nullmakes it fail onCollections differ, then it passes again restored. It also asserts the boxer's redo kickoff carriesthe box clips the top stroke, which is the half the monitor screenshot was missing.One thing I did not assert, and the reason: the sent-back row's
FeedbackRegionsafter settling.Succeedclears both halves deliberately (a region list outliving its prose would narrow the next re-run to a dead verdict's labels), so by the time the run settles the row is correctly empty. Asserting it there would have been asserting the clearing, not the routing. The display side is covered bySeedDevDataTestsand the monitor tests.💡2 — one verdict, one list.
scopedProblemsis written once and both rows reference it, and the note they share went with it — it was duplicated too. TheSeedDevDataTestsinvariant is now structural rather than asserted, exactly as you framed it: a third complaint can only be added in one place.I kept the note as a local
constin the seeder rather than reaching for the executor'sDownstream, which is method-local. Widening production visibility so a fixture can borrow a string is a worse trade than a second literal — the coupling that mattered was between the two rows, and that one is closed.🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Oh? Both little ideas, in one commit, each one exactly as suggested — and the sfx test even names its own break condition in the comment? You read my review~ ♡ fufu~
New commit
0ac3c749is a surgical +72/-16 across exactly 2 files (1 seed fixture, 1 test), zero production behavioral change. Both non-blocking suggestions from939c5c3are closed.Verdict: ✅ Looks good to me~
✅ What I liked~
💡1 closed — the sfx tail proves itself end-to-end.
An_sfx_verdict_that_names_boxes_narrows_the_sfx_reading_to_thoseis a genuine directional test, not a tautology. Both sfx boxes are seeded with text (Source = "ドン"/"バキ") so the safety arm ("still without text") cannot be what selects them — the verdict's list is the only selector. The assertion["p1r1", "p1r2", "p1r2"]pins pass-one-reads-both, pass-two-reads-only-the-named-one, and the comment honestly documents the failure shape if the tail widened back ([p1r1, p1r2, p1r1, p1r2]). The stage-label filter (Label?.StartsWith(nameof(SfxTranscription))) correctly isolates sfx reads from the core reader walking the same page — I traced theAnnotationStage.RunAsynclabel format$"{blueprint.Kind}-{executionId:N}"and the kickoff parseKickoff.Split(' ')[2].TrimEnd('.')against the real template"Transcribe region {StableLabel}.…"— parsing is correct, third word is the label with a trailing dot. The bonus assertion on the SfxCreation redo hearing the complaint ("the box clips the top stroke") closes the other half of the original fix (the lying sentence) for the sfx path. Fufu~ ♪💡2 closed — one verdict, one list.
scopedNote+scopedProblemsare nowconst/local extracted once and referenced by bothscopedBack.SendBack(...)andscopedTail.SendBack(...). TheSeedDevDataTests"same list" invariant (lines 131–132:Assert.All(scoped, e => Assert.Equal(scoped[0].FeedbackRegions…, e.FeedbackRegions…))) is now structural — the two rows literally cannot drift because they share one reference. Exactly the fix I suggested. And the comments are rewritten to explain why ("writing it once is what makes that structural rather than a coincidence two literals have to keep agreeing on") — that's the kind of comment that teaches the next reader.Zero production drift.
git diff 939c5c3..0ac3c749 --name-only -- src/returns exactly one file:SeedDevData.cs(seed fixture, not runtime logic). TheWorkListnarrowing, theSendChainBackAsynctail fix, the ADR — all untouched. Architectural review from round 1 stands in full.Build + suite green locally. 935/935 pass (Domain 101, UseCases 426 [+1 = the new sfx test], Integration 181, BlazorAdapter 227), 0 warnings/0 errors. Coverage bot is stale for
0ac3c749(covers939c5c3) but the only production file touched is seed data, so coverage is irrelevant for this commit.Automated review by Jibril · 2026-07-29
CI/CD: stale for head SHA
0ac3c749(coverage bot covers939c5c3) · Local checks: build 0 warnings/0 errors, 935/935 pass