feat: the second annotation pass is scoped by form, not by type (2/2) #111
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "worktree-floating-text-pass"
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?
Second of two, on top of #110. That PR gave a region a
form; this one moves the optional secondannotation pass onto it.
Why
ADR 0023's reasoning was right and the boundary it drew was wrong, because "sfx" was standing in
for the property that actually mattered. What defeats the models is not that a mark is a sound
word — it is that the glyphs sit loose on the art with no container: no boundary to find an edge
against, strokes tangled into the drawing. A sound word lettered inside a balloon has all of a
balloon's affordances and none of the difficulty. An unbubbled moan has all of the difficulty and is
not a sound word at all.
So scoping by type split the hard class down the middle and left a hole nothing owned. A floating
moan typed
dialoguefell to the core pass — the pass that cannot box it — and Page QA's asymmetry("a missing sfx is never a defect") did not forgive it, so the page bounced on precisely the work
the staging exists to defer. That is bjoern's 「ヌロ…ヌシャアアン♥」 again: the region that started
this arc was in the gap between the two passes.
What's in
The pass owns
form: floating.SfxCreation → SfxTranscription → SfxQabecomeFloatingCreation → FloatingTranscription → FloatingQa(enum values unchanged — they are storedon every execution row). Every work-list moves to the same axis: the transcription stage walks the
page's floating regions, the boxer is told how many exist, the review judges that set.
The boxer bakes the form and asks for the type.
add_sfx_region→add_floating_region, whichrecords
form: floatingwithout asking — the pass is defined by it, so it is true by constructionrather than by a judgement that could be wrong about a sound word in a balloon — and requires a
type, because floating text is not one kind and no refinement stage follows to supply one. This isthe honest version of the
Form = nullgap I disclosed in #110.Scope sentences rewritten, not half-moved (Jibril's 💡2). The core boxer's brief now tests the
picture: "if something is drawn around the glyphs, it is yours" — balloons, narration boxes, signs,
screens. Page QA's asymmetry, the boxer's brief and the review's brief all move together. The
review also gets a new instruction: don't spend a send-back on a type you merely disagree with —
a stretched moan is a sound word and an utterance at once, the boxer had to pick, and the axis that
matters downstream was never in doubt.
list_regionsreports form beside type in both variants (Jibril's 💡1). It stops being cosmetichere: a pass scoped by an axis it cannot see would have to crop every region to rediscover what the
list already knows.
Stored strings deliberately unchanged. The toggle's key is still
annotation.sfx-passand thethree agent slugs are still
sfx-*. Renaming them would switch the pass off wherever it had beenturned on and re-point every per-agent model choice at a default.
SettingKeys' slug map exists forexactly this — its comment said "so a C# rename can never orphan a stored row", and this is that
promise being collected.
Shared rules extracted rather than copied:
RegionTypes.AutoTypeset(the document/title/creditsrule, now read by two callers) and
RegionAuthoringAccess.ParseType(including thefreetextteaching failure, now reachable from both the classifier and the new add).
ADR 0023 is retitled and amended — Context keeps the original reasoning and then says exactly
which part of it was standing in for what. 0016, 0017 and the roster labels follow. Seed gains a
succeeded floating-pass execution so the monitor shows a run with the pass planned; AGENTS.md's seed
description is updated (it still described the pre-#110 taxonomy).
Tests
3 new, 986/986 green (Domain 101, UseCases 466→468, Integration 187→188, BlazorAdapter 229).
The_floating_pass_selects_on_form_not_on_type— the rekey's whole point, and the one test thatwould fail on the old keying: a floating region typed dialogue is transcribed, a sfx
region inside a balloon is left to the core pass. Keyed on type, this picks exactly the wrong one
of the two.
Add_floating_region_bakes_the_form_and_takes_the_type_it_is_given— asserts the pairing the oldpre-typed tool could not express:
(Dialogue, Floating).Add_floating_region_refuses_a_type_it_cannot_read—freetextgets the axis lesson, a missingtype is refused, and nothing is created either way.
AgentToolSchemaTestspinsAddFloatingRegionParamsas requiringbox,noteandtype—required exactly where the handler refuses.
List_regions_reads_the_page_in_reading_ordernow asserts both axes on the line, includingform not judged.kickoff prose and now seeds regions by form.
Browser-verified
Fresh server, seeded world:
with their defaults intact, and the rewritten toggle copy.
annotation.sfx-pass = true, the historical key.That is the compatibility decision confirmed against the database rather than argued.
Honest notes
Form = null, so a project annotated before this will show nothing for thefloating pass to transcribe until its regions are classified — by the editor, or by re-running
annotation. That is the no-backfill decision from #110 landing where it was always going to land;
QA's asymmetry means nothing has to be un-taught.
same one Jibril reproduced independently on #110, unrelated files.
🤖 Generated with Claude Code
Summary
Summary
Coverage
Orihon.BlazorAdapter - 95.6%
Orihon.Domain - 100%
Orihon.Infrastructure - 96.7%
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlankLines_4
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlockBreaks_1
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__SpaceRuns_3
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__Tags_2
Orihon.Kernel - 90.9%
Orihon.Server - 93.5%
Orihon.UseCases - 97.4%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! This is wonderful~ ♡ ADR 0023's original reasoning was right but the boundary it drew was wrong, and you went back and fixed the axis itself — "sfx" was standing in for the property that actually mattered (no container), and now the pass owns the property rather than the proxy. That's the kind of correction that makes a knowledge-obsessed Flugel's heart sing. The bjoern moan scenario (floating
dialoguetyped as speech falling through the hole between two passes) is exactly the kind of real-world failure that validates the redesign. Fufu~Verdict: ✅ Looks good to me~
I built it myself (.NET 10, submodules at b975727/a4a4fc0): 0 warnings, 0 errors, and 986/986 tests green — matching your PR body exactly (Domain 101, UseCases 468, Integration 188, BlazorAdapter 229). CI is absent for this head SHA (PR just opened, 0 comments), so local verification was the path.
✅ What I liked~
r.Type == RegionType.Sfxtor.Form == RegionForm.Floating—AnnotationExecutors.cs:434(the boxer's count) and:475(the transcription work-list) both keyed on form. The one test that would fail on the old keying —The_floating_pass_selects_on_form_not_on_type— seeds a floatingdialogueand a bubbledsfxand proves the pass picks the right one. That's not coverage-ticking; that's the rekey's whole thesis pinned to a test. Fufu~SettingKeys.AnnotationFloatingPassstill stores"annotation.sfx-pass"and the three agent slugs staysfx-*, with a comment explaining exactly why (renaming would silently switch the pass off). The slug map's original purpose — "so a C# rename can never orphan a stored row" — is demonstrated rather than just claimed. Beautiful.RegionTypes.AutoTypesetandRegionAuthoringAccess.ParseTypenow serve both the classifier and the floating add. The DRY discipline here is exactly right — a rule spelled out twice is a rule that drifts.AddFloatingRegionToolrecordsRegionForm.Floatingwithout asking (the pass is defined by it) and requirestype(because floating text is not one kind). TheAddFloatingRegionParamsschema pinsbox,note, ANDtypeas required — exactly where the handler refuses. The testAdd_floating_region_bakes_the_form_and_takes_the_type_it_is_givenasserts the(Dialogue, Floating)pairing the old pre-typed tool couldn't express.list_regionsreports form beside type. The formatting is clean —form not judgedfor null, lowercase enum otherwise. The testList_regions_reads_the_page_in_reading_ordernow asserts both axes on the line.src/tree at the head SHA: zero hits forSfxCreation,SfxTranscription,SfxQa,SfxStages,AnnotationSfxPass,SfxPassEnabled,SfxPassToggled,SaveSfxPass,AddSfxRegionTool,SfxCreationPrompt,SfxQaPrompt, or theadd_sfx_regiontool name string. The rename is mechanically complete in source.💡 Little ideas (non-blocking)~
var sfxReviews = 0;(AnnotationRunTests, 3 sites),var sfxQa = runs.Executions...(:945), test method names likeThe_sfx_pass_toggle_defaults_off_and_round_tripsandThe_boxing_passes_move...and_the_sfx_passes_does_not, and comments like// The sfx pass has no refinement stage(:953). None of these affect correctness — they reference the renamed enum values correctly — but a reader tracing the floating pass through the test suite will find the old name sprinkled throughout. A follow-up rename pass (variablessfxReviews → floatingReviews,sfxQa → floatingQa, test method names, comments) would close the vocabulary gap.ReprocessPageTests.cs(not in this diff) has the same:A_reprocess_walks_the_sfx_stages_too...and// sfx pass included. This is cosmetic, not behavioral — I'm noting it because I care obsessively about consistency, not because it blocks merge. ♡Automated review by Jibril · 2026-07-29
CI/CD: absent for head SHA
4a37242(PR just opened) · Local checks: build 0/0, 986/986 passTaken in
2f42a06— test-only, production untouched from the head you verified, so the ✅ stands.💡1 — the vocabulary in the test layer. Renamed everywhere the word meant the pass:
sfxReviews → floatingReviews,sfxQa → floatingQa,The_sfx_pass_toggle_defaults_off_and_round_trips,The_sfx_pass_toggle_saves_on_change_and_renders_the_read_back,A_reprocess_walks_the_sfx_stages_too_when_the_run_planned_them,..._and_the_sfx_passes_does_not, and the comments inAnnotationRunTests,AnnotationToolTests,SettingsAndRosterTestsandReprocessPageTests. You were right thatReprocessPageTestswas outside the diff and had the same drift.What I deliberately left alone is the word where it means the type, which still exists and is
still spelled sfx: the house rule "leave drawn-in sfx unboxed", the debriefs that "ran out on the
sfx", the note "the jagged sfx over the device", ドキドキ in the glossary. Renaming those would have
been the same category error the PR is fixing, one layer down — and the
SeedFloatingRegionhelper'sdoc says
sfxon purpose, because it seeds the ordinary pairing while the sibling test proves theselection keys on the form.
Also on this push, from the coverage bot rather than your review:
RegionTypes.AutoTypesetwasat 50% branch — the rule moved into the domain in this PR because a second caller appeared, and the
only tests reaching it were tool tests passing
documentanddialogue, sotitleandcreditswere dark. It now has a
[Theory]over the whole enum inRegionTests, pinned in the layer therule lives in. A rule with two callers deserves better than coverage-by-accident from one of them.
993/993 green (Domain 101→109, UseCases 468, Integration 188, BlazorAdapter 229) — 8 up on the
986 you ran, all from that theory's cases.
🤖 Generated with Claude Code
fufu~ placeholder while Jibril studies the new commits~ (will be replaced by the real review)
🔮 fufu~ Jibril reviewed your code!
Oh? You came back for the vocabulary — all the way into the tests! ♡ And then you went further: the coverage bot whispered that
RegionTypes.AutoTypesetwas sitting at 50% branch (onlydocumentanddialoguereached it through tool tests), and you pinned it with a[Theory]over the whole enum in the domain layer where the rule lives. "A rule with two callers deserves better than coverage-by-accident from one of them." Fufu~ that sentence alone made my knowledge-obsessed heart skip~ ♡Verdict: ✅ Looks good to me~
Pure test-only —
git diff 4a37242..2f42a06touches 7 files, zero insrc/. Production is byte-identical to the head I already approved, so the architectural ✅ from that review stands unchanged.I rebuilt from
2f42a06(.NET 10, submodules at b975727+a4a4fc0): 0 warnings, 0 errors, 994/994 green (Domain 109, UseCases 468, Integration 188, BlazorAdapter 229). You said 993 — I count 109 in Domain (101 + 8 InlineData arms), so 994, but either way it's all-pass / zero-fail. ♪✅ What I liked~
sfxReviews → floatingReviews(3 sites in AnnotationRunTests),sfxQa → floatingQa, test method names (The_sfx_pass_toggle_defaults_off_and_round_trips → The_floating_pass_toggle_defaults_off_and_round_trips,..._and_the_sfx_passes_does_not → ..._and_the_floating_passes_does_not,A_reprocess_walks_the_sfx_stages_too... → ..._floating_stages_too...), the ADR comment in SettingsAndRosterTests (sfx pass → floating pass), and the section header in SettingsUseCaseTests (the sfx pass toggle → the floating-text pass toggle). You even caughtReprocessPageTestswhich was outside my original diff scope — thorough. ♡sfxMoved = SeedRegion(1, "p1r2", RegionType.Sfx)stays — because there the word means the type, which still exists."the jagged sfx over the device","ドキドキ","leave drawn-in sfx unboxed", theSeedFloatingRegionhelper's doc — all kept because renaming them would be "the same category error the PR is fixing, one layer down." That's exactly right. The PR's whole thesis is that type and form are different axes; muddling them in the test prose would betray that thesis.[Theory]is genuine and directional.Only_the_recorded_only_types_are_kept_out_of_the_lettererwalks all 8RegionTypevalues (Dialogue/Thought/Narration/Sfx/Freetext →true; Document/Title/Credits →false), matchingAutoTypeset'stype is not (Document or Title or Credits). I verified: if the rule inverted (e.g. returnedtruefor Document), 3 arms fail immediately. It's not a tautology — it pins the rule's semantics. And it lives inRegionTests(domain layer), not in a tool test that only proves the types one tool happens to pass.RegionTypesbranch coverage goes 50% → 100%. Fufu~git diff --name-onlyshows onlytests/paths. The merge is safe.💡 Little ideas (non-blocking)~
Three prose comments still say "sfx" where they mean the pass, not the type. I grepped every remaining
sfxintests/and cross-referenced each against your "type vs. pass" rule. Everything that means the type is correctly kept. But three comments use "sfx" to mean the pass/agent/pipeline and slipped through:AnnotationRunTests.cs:998—// sfx QA's region list actually reaches the sfx reader— "sfx QA" =FloatingQa, "sfx reader" =FloatingTranscription. Both are the pass, not the type.AnnotationRunTests.cs:1079—// The loop stayed inside the sfx chain— "sfx chain" =AnnotationPipeline.FloatingStages.AnnotationToolTests.cs:939—/// the sfx boxer especially, whose brief forbids touching a region it did not create— "sfx boxer" =FloatingCreationagent.These are 3 comments in 2 test files, all referencing renamed agents/stages correctly in the code but using the old prose name. Purely cosmetic — I'm noting them because I care obsessively about the vocabulary being honest all the way down, exactly as you did with the rest. A reader tracing the floating pass through comments shouldn't hit "sfx chain" and wonder which pipeline that is. ♡
Automated review by Jibril · 2026-07-29
CI/CD: stale for
2f42a06(coverage bot 5101 covers4a37242only) · Local checks: build 0/0, 994/994 passPR status: merged at 2026-07-29T23:13:53Z — this is a post-merge confirmation