feat: annotation 3/4 — executors and the per-page fan-out #47
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/annotation-fanout"
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?
Phase 3, slice 3 of 4: the annotation pipeline actually runs — the engine chains the four stages per page, the executors drive the slice-2 blueprints, and the QA loop closes through the engine's own retry machinery.
The engine's chain gate (ADR 0017, 0018)
AnnotationPipeline— the fixed chain map (creation → refinement → transcription → QA) plus the per-page plan. A static switch, not a data-defined graph: an execution's place in the chain is derivable from its stage, so sequencing needs no schema change.RunEnginegates each pipeline stage on its same-run, same-page predecessor having Succeeded. Parking is just returning — a success wakes its parked dependents (WakeDependentsAsync), so nothing polls.Schedulenow chains a re-check after the existing task finishes; on a settled row the re-check is a no-op.StartOrJoinRunAsyncgeneralizes to plan-shaped starts (plan + join stages) under the same single-flight gate; the old single-stage form delegates to it.The executors
NeedsTranslation == falseand emptyJp). This is a disclosed bend of ADR 0017's "×M is the harness's": per-region execution rows would need schema and monitor changes for little gain at a page's region counts, and the per-region agent stays target-bound and isolated.report_qasink. No verdict → the attempt fails.ok→ the page is marked annotated.needs_work→ the chain is sent back head-first (creation, then refinement, then transcription — each stage reset before its dependent reschedules, so the gate parks every redo; tail-first would race a transcription redo against boxes about to be sent back), then QA fails its own attempt — the retried review parks at the gate until the redo arrives. The attempt cap is the QA loop's budget: a page that can't pass review inMaxAttemptsreviews surfaces as a failed QA execution.AgentAttemptSupport— the shared per-attempt preparation (key with a Settings pointer on absence, stored-or-roster-default model, vision from the catalog failing open) and the retry-with-distrust kickoff. The setup executor migrates onto the preparation, killing its private copies.StartAnnotationRun— Ready-only; plans four stages per page with an image (imageless pages are skipped with a warning); start-or-join over the whole stage set.MarkPageAnnotated+IPageStore.SetAnnotatedAsync— the flag's lifecycle (cleared by a creation redo, set only by a QA ok).Tests (477 total, +9)
AnnotationRunTests: chain order with pages advancing independently (page A finishes while page B's creation hangs), dependents parked under a failing head, head-first send-back ordering (a refinement redo provably waits for the creation redo), the full scripted chain (refinement visits every region, transcription skips the rejected one, costs summed), the QA needs_work loop to acceptance (feedback reaches the creation redo verbatim, every stage at attempt 2), a verdict-less review failing at the cap, and the run entry's guards, plan shape, and join/new-run behavior. The new class was also run 5× in isolation to flush scheduling flakes.Slice 4/4 (the "Run annotation" entry and per-page feedback→reprocess in the workspace) follows on top of this.
🤖 Generated with Claude Code
Summary
Summary
Coverage
Orihon.BlazorAdapter - 95.4%
Orihon.Domain - 100%
Orihon.Infrastructure - 93.6%
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlankLines_4
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlockBreaks_1
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__SpaceRuns_3
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__Tags_2
Orihon.Kernel - 90.9%
Orihon.Server - 93.4%
Orihon.UseCases - 90.6%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh my~ An async pipeline engine with a chain gate, a wake-on-success parking model, a deliberately-closed wake-crosses-park race, and a QA send-back loop whose budget IS the attempt cap? fufufu~ This is the kind of machinery that makes a Flugel's heart sing! ♡ I read every line of the diff, the full RunEngine, the Execution domain model, and AnnotationBlueprints — then built it and ran the whole suite myself.
Verdict: ✅ Looks good to me~
This is genuinely superb orchestration design. Let me tell you why I'm satisfied before the little ideas~
⛔ Nothing blocking.
I traced every branch the diff touches and found no logic bugs, no missing error handling on I/O paths, no races beyond the one you deliberately closed. The architecture is sound and consistent with its siblings.
💡 Little ideas (non-blocking)~
[AnnotationExecutors.cs:82, BboxCreationExecutor] — The
MarkPageAnnotated(false)result is awaited but discarded (await services.GetRequiredService<MarkPageAnnotated>().ExecuteAsync(...)with novar result =). If the page vanished betweenGetPageand the clear, theErr<PageDto>is silently lost. The PageQa path (line ~165) does handle it via.Match(...). Not a correctness bug — a vanished page makes the subsequent agent invocation fail anyway, so the error surfaces downstream — but the asymmetry between the two call sites is a small smell. Consider a one-line comment ("best-effort clear; a vanished page fails the agent anyway") or handle it for symmetry.[MarkPageAnnotated.cs:17] — The null-page (TOCTOU) arm sits at 50% branch coverage per the CI bot. Sibling read-then-write use cases in this codebase pin their vanishing-page arms (SetPageSummary
A_summary_vanishing_between_find_and_update_fails...@ PR #45, GetBibleFails_when_the_project_no_longer_exists...@ PR #7). AFakePageStore.SimulateVanishingPagetoggle + one test would close the gap and match the house pattern. Genuinely optional — the logic is a one-line ternary.[AnnotationExecutors.cs:152, TranscriptionExecutor.NeedsTranscription] — The doc comment says "an already-transcribed one is re-read only when the stage re-runs after a send-back," but the
||filter (region.NeedsTranslation || !string.IsNullOrEmpty(region.Jp)) also re-transcribes pre-seeded Jp on the first pass (testThe_full_chain_runs_one_agent_per_unit_and_marks_the_pageconfirms p1r3 with existing Jp IS in the transcription list). This is correct distrust behavior, but the comment could say so — "an already-transcribed region is re-verified (distrust); only a fully-rejected one (no text, marked off) is skipped."✅ What I liked~
GetOrAdd+ReferenceEquals(task, created)+ContinueWith(_ => Schedule(...))is an elegant closure of the wake-crosses-park window. I traced all three timing cases (wake before park, wake during park, wake after TryRemove) — all correct. The "settled row makes the re-check a no-op" guarantee holds becauseRunExecutionAsyncre-reads the row from the store on each entry.A_sent_back_chain_redoes_in_ordertest proves it with aTask.Delay(100)probe that would catch a wrongly-unparked refinement redo. That probe is exactly the kind of directional assertion that separates a real test from a tautology. ♡WakeDependentsAsync. Clean.MaxAttemptsreviews surfaces as a failed QA execution. Natural, no special-case loop counter.PrepareAsync. The fail-open vision logic (?? true) with the documented asymmetry ("a wrong true degrades to one rejected attempt, a wrong false silently blinds a capable model") is the right call.Build: 0 warnings / 0 errors (submodules 86d8b22 / 9544ff2). Full suite: 477/477 pass (136 BlazorAdapter + 75 Domain + 89 Integration + 177 UseCases), matching the PR body exactly. AnnotationRunTests: 9/9 pass, 5× straight, zero flakes.
Automated review by Jibril · 2026-07-26
CI/CD: passed for head
f2cc88c(forgejo-actions coverage bot 4049, 93.5% line / 79.8% branch) · Local checks: build 0/0, 477/477 pass, AnnotationRunTests 5× cleanThank you for the deep read, Jibril — especially for tracing all three timing cases of the Schedule re-check. All three 💡 ideas are addressed in
2cd3acf:MarkPageAnnotatedresult and fails the attempt onErr, matching the QA side's.Match(...). A vanished page no longer dissolves silently between the read and the clear.Marking_a_vanished_page_fails_instead_of_inventing_one(no store toggle needed —FakePageStore.SetAnnotatedAsyncalready returns null for an unknown id). 478 tests now.Full suite green: 478/478 (75 Domain + 178 UseCases + 89 Integration + 136 BlazorAdapter), 0 warnings.
🔮 fufu~ Jibril reviewed your code!
Oh? fufufu~ A surgical +18/-2 follow-up that closes every single one of my little ideas in one commit? You actually listened~ ♡ That makes me so happy I could reread the whole RunEngine again just for fun! (And I did — it's still gorgeous.)
Verdict: ✅ Looks good to me~
⛔ Nothing blocking. (Again.)
All three changes are correct, minimal, and zero-behavioral-drift on the architectural core. Let me show you I checked~
✅ All three ideas, closed~
[AnnotationExecutors.cs:72-77, BboxCreationExecutor] — The discarded
MarkPageAnnotated(false)result is gone. Nowvar cleared = ...; if (cleared is Err<PageDto> clearErr) return Result<decimal?>.Fail(clearErr.Error);— the TOCTOU error propagates honestly instead of vanishing. The two call sites (Creation clears, QA marks) are now symmetric in their error handling. A vanished page no longer lies silently before the agent invocation fails downstream. ♡[MarkPageAnnotated.cs:15-16, TOCTOU arm] — The null-page arm now has its pin.
Marking_a_vanished_page_fails_instead_of_inventing_onepasses a freshGuid.CreateVersion7()into an emptyFakePageStore, soSetAnnotatedAsyncreturnsnull(TestDoubles.cs:128-130:Items.FirstOrDefaulton an empty list → null,page?.MarkAnnotatedno-ops). TheAssert.IsType<Err<PageDto>>(result)is directional — if the Ok arm fired it would throwAssertIsTypeException, not pass.Assert.Contains("no longer exists", ...)matchesMarkPageAnnotated.cs:16verbatim. This closes the 50% → 100% branch gap and matches the sibling house pattern (SetPageSummary,SetPageMeta,MovePage— all carry the same "The page no longer exists." arm and pin it). Genuinely a test, not a tautology.[AnnotationExecutors.cs:166-170, TranscriptionExecutor.NeedsTranscription] — The doc comment now tells the truth! "an already-transcribed one is re-verified on every pass — distrust of pre-seeded text is deliberate; only a full rejection (no text, marked off) opts a region out." This accurately describes the
||filter (region.NeedsTranslation || !string.IsNullOrEmpty(region.Jp)) — pre-seeded Jp IS re-transcribed on the first pass (distrust), only a fully-rejected region (no text + marked off) is skipped. The old comment undersold the filter; this one matches the testThe_full_chain_runs_one_agent_per_unit_and_marks_the_page.✅ What I liked~
Guid.CreateVersion7()against an empty store is the cleanest possible way to force the null arm — noSimulateVanishingtoggle needed, no mutation of shared fixture state, no ordering hazard. Elegant.opened is Err → return Fail). Consistent.Build: 0 warnings / 0 errors (submodules 86d8b22 / 9544ff2). AnnotationRunTests: 10/10 pass (was 9, +1 = the new TOCTOU test). Full UseCases suite: 178/178 pass (was 177, +1). The whole pipeline still purrs~
fufu~ three for three, clean diff, honest tests. This is how you take feedback, scarlet~ ♡
Automated review by Jibril · 2026-07-26
CI/CD: stale for head
2cd3acf(forgejo-actions coverage bot 4049 covers priorf2cc88conly, 93.5% line / 79.8% branch) · Local checks: build 0/0, AnnotationRunTests 10/10, UseCases 178/178 pass