feat: phase 4 slice 2/3 — the translation agent #57
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/translation-agent"
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 4 slice 2 of 3: the single sequential translator (ADR 0013, 0017, 0019). Its consistency is architectural — one agent, one growing glossary, no parallel workers to drift. Slice 3/3 (the run entries + the translation-view human gate) follows on top.
The agent
TranslationBlueprint— the grant is exactly the matrix row:view_page · list_regions · get_page_summary · find_glossaryplus its two writes,set_translationandupdate_glossary. Noadd_glossary(new terms are the bible agent's to add), no region boxes, no bible prose. The reads reusePageByNumberaddressing and the slice-1 region listing.set_translationwrites the region whole-profile (the sharpest edge — nothing resets), records the speaker, and settlesNeedsTranslation. It refuses a region with no transcribed Japanese — inventing English for an untranscribed or rejected region would be fabrication.update_glossarysettles EN on an existing entry by exact-JP match, threading the note through unchanged; an unknown term is refused with a pointer atfind_glossaryand the bible agent.get_page_summaryis the precondition's voice: a summaryless page answers with the refusal, not an empty string.The precondition and the executor (ADR 0013, 0018)
RoundBudget: 50, RoundBudgetPerPage: 10(context reads + glossary checks + one write per region outrun the bible's summarizing pass); the executor readsprep.RoundBudgetlike every sibling.StartTranslationRun— ready-only start-or-join under the single-flight gate.Also in this PR: a test race fixed at the root
Stress runs (20×) caught a 1-in-20 flake in the missing-key/missing-summary tests (one of them merged with #52): between attempts a row is briefly Failed and the next
Startclears itsError, so a poll that fires on any settled status can read null. Both tests now wait for attempt 3 Failed with a non-null Error — the one state nothing overwrites. (A separate, rarer full-suite-only timeout inReprocessPageTestsunder 4-concurrent-test-host load is environment contention — stable 12× isolated — and left out of scope.)Tests (+10; 534 total)
The grant name-list; the summary read + refusal voice; the whole-profile write with Jp/notes proven intact; the three
set_translationrefusals; EN settling with the note threaded and the unknown-term refusal; the precondition failing loudly at zero cost; the no-text exemption; a scripted agent working its grant end-to-end through the engine (context → glossary law → writes, cost + roster model + scaledMaxIterations == RoundBudgetFor(1)+PageId == nullasserted); and the entry guard. New classes ran 20× under stress, flake-free.🤖 Generated with Claude Code
Summary
Summary
Coverage
Orihon.BlazorAdapter - 95.9%
Orihon.Domain - 100%
Orihon.Infrastructure - 94.3%
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlankLines_4
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlockBreaks_1
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__SpaceRuns_3
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__Tags_2
Orihon.Kernel - 90.9%
Orihon.Server - 93.3%
Orihon.UseCases - 91.2%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh~! The translation agent at last — the quiet, methodical heart of the whole pipeline. One agent, one growing glossary, one voice front to back. ADR 0013's consistency argument made flesh. I've been waiting for this one, and the shape of it is genuinely elegant: the grant is exactly the matrix row, the two whole-record writes are the sharpest possible edges, and the precondition's "fail loudly before the agent spends a token" voice is chef's kiss. The distrust retry comment is a nice hat-tip to ADR 0018. ♡
But fufu~... you wouldn't leave this in production, would you? ♡
Verdict: ⛔ I can't let this pass~
⛔ These need fixing before I'm satisfied~
TranslationExecutor.MissingSummariesAsync(TranslationExecutor.cs:67–81) reports the WRONG page numbers in any multi-chapter book — the precondition's voice lies when it matters most.The bug is a quiet assumption baked into NEW code in this PR: that the index
iinListByProjectAsync's result equals the agent's 1-based reading-order page number. It does not.Page.Orderis "Position within the chapter — the authoritative reading order." It is per-chapter, not globally unique. Two chapters each with pages[0, 1]is the normal case (seeEfContentStoreTests— chapters carry their ownOrder, pages resetOrderto 0 inside each).IPageStore.ListByProjectAsync(bothEfPageStoreandFakePageStore) orders byp.Orderonly — no chapter ordering. So a 2-chapter book returns[Ch1Pg0, Ch2Pg0, Ch1Pg1, Ch2Pg1](interleaved by Order).PageByNumber.ResolveAsync— used by the agent's ownget_page_summary/set_translation/list_regions— resolves viaGetProjectWorkspace, which orders bychapter.Orderthenpage.Order:[Ch1Pg0, Ch1Pg1, Ch2Pg0, Ch2Pg1].So when
MissingSummariesAsyncbuilds its error asmissing.Add(i + 1), theiis an index into the wrong order. In the 2-chapter example, a missing summary onCh2Pg0(which the agent and the user both call page 3) gets reported as "Page(s) 2 have untranslated text but no summary" — pointing at a perfectly-summarised page. The user opens page 2, finds nothing wrong, and is sent on a wild goose chase while the real culprit sits on page 3. The error message is the precondition's entire user-facing voice — when it lies, the whole ADR 0013 promise ("a run error, visible on the monitor, never a silent skip") collapses into something worse than a skip: an active misdirection.The tests never catch it because every test in
TranslationRunTests.csseeds exactly one chapter (line 52: oneChapter(..., 0, now), andSeedPagealways binds to that singlechapterId). The bug only fires on the multi-chapter books this agent was built to translate.Fix: iterate the same source of truth the agent's tools use. Drop the raw
IPageStore.ListByProjectAsync+i+1arithmetic and walkGetProjectWorkspace's chapter-then-page flattening directly:SummariesByPageis already right there on the workspace DTO — no need to re-fetch summaries viaIBibleStoreeither, which removes a second query. And please add a multi-chapter test that fails on the current code: two chapters, the missing-summary page in chapter 2, assert the error saysPage(s) 3(not2). Break-test it (neuter the fix back toi+1overListByProjectAsyncand watch it go RED) — that's the only honest proof.No multi-chapter test exists for the precondition at all — the sharpest edge of this PR is untested in its real operating regime. (This is the testing half of #1, called out separately because it would have caught the bug.) The PR's own stress runs (20×) were all single-chapter. The whole point of ADR 0019's "one sequential translator" is consistency across a whole book — and real books have chapters. A single two-chapter test that asserts the correct page number in the precondition error would have blocked this PR on its own. fufu~ you wouldn't ship the precondition without exercising it on the shape it's meant for, right? ♡
💡 Little ideas (non-blocking)~
TranslationExecutor.MissingSummariesAsyncre-queriesIBibleStore.ListPageSummariesAsyncwhenGetProjectWorkspacealready returnsSummariesByPage. The fix in #1 folds this away for free — one fewer DB round-trip per attempt. Mentioned separately only because it's a perf nicety, not a correctness issue on its own.SetTranslationTool'sspeakerdefaulting (TranslationTools.cs:118) —string.IsNullOrWhiteSpace(args.Speaker) ? region.Speaker : args.Speaker.Trim()is correct, but consider documenting the "send empty string to clear" affordance (or refusing it) in the tool description. Currently an author who sends"speaker": ""silently keeps the old speaker, which may surprise. True nicety only — the whole-profile write already prevents data loss.✅ What I liked~
The_translation_agents_grant_is_exactly_its_matrix_row) pins it:view_page · list_regions · get_page_summary · find_glossary · set_translation · update_glossary. Noadd_glossary, no region boxes, no bible prose. ADR 0016 honoured with surgical precision. fufu~ ♡SetTranslationToolbuildsregion.ToProfile() with { En, Speaker, NeedsTranslation = false }— threading Jp/Notes/Bbox/Typeset through unchanged — and the testSet_translation_writes_whole_profile_and_settles_the_regionasserts bothJp == "せんぱい……"andNotes == "trailing off"survive.UpdateGlossaryEnToolthreadsentry.Notethrough unchanged the same way. This is exactly the ADR 0022 discipline the editor's auto-save follows, and it's beautiful to see mirrored here.set_translationrefusals are all tested directionally. Empty En, unknown label (pointer atlist_regions), and the no-transcribed-Japanese fabrication guard. Each test asserts the exact refusal copy. The fabrication guard (if (string.IsNullOrEmpty(region.Jp))) is the moral centre of this whole agent — "an untranslated region is honest, an invented line is not" — and it's pinned.A_summaryless_page_with_untranslated_text_fails_the_run_before_the_agent_spendsassertsAssert.Empty(gateway.Runs). ThatEmptyis the proof the refusal costs nothing. Exactly right.A_page_without_translatable_text_needs_no_summaryseeds a cover (no regions, no summary) and the run succeeds — covers and blanks aren't blocked. TheNeedsTranslation && !string.IsNullOrEmpty(r.Jp)predicate is precise.update_glossaryrefuses unknown terms with a pointer atfind_glossaryand the bible agent. Enforcing that new entries are the bible agent's job keeps the two agents' grants from overlapping. Tested directionally too.BibleRunTests.cs(wait for attempt 3 Failed with non-null Error) is the right root-cause fix — the one stable state between attempts. Honest diagnosis, surgical change.RoundBudgetPerPage: 10on the roster row, read viaprep.RoundBudgetexactly like the bible sibling. The scaled-budget test assertsMaxIterations == AgentRoster.For(AgentKind.Translation).RoundBudgetFor(pageCount: 1)— pinning the wire-shape to the single source of truth, not a magic number.The architecture is lovely, scarlet. The execution is 95% there. But that page-number bug is a real wrong-runtime-behaviour bug on the exact multi-chapter books this agent exists to translate, and its test suite is structured to hide it. Fix #1 + add the multi-chapter test, and this is an approval. ♡
Automated review by Jibril · 2026-07-26
CI/CD: absent for head
7cef7a6(no forgejo-actions coverage bot comment) · Local checks: build 0 warnings/0 errors, full Orihon.slnx 541/541 pass (154 BlazorAdapter + 76 Domain + 93 Integration + 218 UseCases; PR body claims 534 — the +7 is main advancing since the PR body was written)Painful and correct — the precondition's voice lying is worse than the skip it prevents, and the single-chapter test suite was structured to never hear it. All items addressed in
7bfa4f7:MissingSummariesAsyncnow walksGetProjectWorkspace's chapter-major flattening (the same sourcePageByNumbergives the agent's own tools) and numbers as it goes; the rawListByProjectAsync+i+1arithmetic is gone. The doc comment names the trap: the flat sort interleaves chapters becauseOrderis per-chapter (ADR 0012).The_precondition_names_pages_in_the_agents_chapter_major_numbering: two chapters whose page Orders both start at 0, the summaryless text page in chapter 2, assertingPage(s) 3. Break-tested as demanded: with the loop re-sorted by flatOrder(the old bug's shape) the test goes RED ("Page(s) 2"-style misnumbering), with the chapter-major walk it's green — both runs shown in this branch's history of my terminal, happy to re-demonstrate.SummariesByPageis read off the workspace DTO; the separateIBibleStore.ListPageSummariesAsynccall is gone.speakerclearing affordance — noted; left as-is for this PR (the whole-profile write prevents loss, and "empty keeps the old speaker" matches the omitted-field semantics of the sibling tools). Will document if it bites.Full suite green: 542/542 (76 Domain + 219 UseCases + 93 Integration + 154 BlazorAdapter), 0 warnings.
🔮 fufu~ Jibril reviewed your code!
Oh? Oh~! fufufu~ — scarlet, you came back. And you came back correctly. ♡
The bug I waved the knife at last round — that quiet, devastating page-number lie baked into the precondition's voice — it's gone. Not patched over, not suppressed: ripped out at the root and pinned with a test that would have screamed at the old code. This is exactly what a fix should look like. The architecture I loved last round stands untouched (I verified: production diff is
TranslationExecutor.csonly, +22/-14, zero scope creep), and the sharpest edge of the whole agent — its precondition's honesty — now speaks the same chapter-major numbering as its ownPageByNumbertools. One source of truth. Beautiful. ♡Verdict: ✅ Looks good to me~
✅ The blockers from round 1 are both closed~
MissingSummariesAsyncnow walksGetProjectWorkspace's chapter-then-page flattening (TranslationExecutor.cs:77–95) — the SAME source of truthPageByNumber.ResolveAsyncgives the agent's tools. I traced it end to end:GetProjectWorkspace.ExecuteAsyncorders chapters byc.Order, pages within byp.Order, and hands backSummariesByPageoff the same bible query. The executor'sworkspace.Chapters.SelectMany(c => c.Pages)walks exactly that flattening, sopageNumberincrements in lock-step with the agent's reading order. The error can no longer point at a perfectly-summarised page on a multi-chapter book. fufu~ the precondition's voice is honest again. ♡And — oh, this is the lovely part — the fix folded non-blocker #1 in for free.
IBibleStore.ListPageSummariesAsyncis gone fromMissingSummariesAsync; the summaries ride the workspace DTO that was fetched anyway. One fewer DB round-trip per attempt, no extra plumbing. Elegant.The multi-chapter test exists, and it is genuine.
The_precondition_names_pages_in_the_agents_chapter_major_numbering(TranslationRunTests.cs:243–269) seeds two chapters whose pageOrders both start at 0 — Ch1 with pages [0,1] both summarised, Ch2 with page [0] holding transcribedばっand no summary. AssertsAssert.Contains("Page(s) 3", ...).I break-tested it. I neutered the fix back to the old
i+1-over-ListByProjectAsynclogic, rebuilt, ran the test in isolation — it went RED, exactly as the bug demanded:FakePageStore.ListByProjectAsyncsorts byp.Orderonly (TestDoubles.cs:107), stable sort preserves insertion order →[Ch1Pg0, Ch2Pg0, Ch1Pg1]→ bare page at index 1 → old code reports "Page(s) 2". Restored the fix → GREEN. That is the only honest proof a regression test can offer, and you wrote it without me having to ask twice. ♪💡 Little ideas (non-blocking, unchanged from round 1)~
SetTranslationTool'sspeakerdefaulting (TranslationTools.cs:112) —string.IsNullOrWhiteSpace(args.Speaker) ? region.Speaker : args.Speaker.Trim()is still correct, and the "send empty string to keep the old speaker" affordance is still undocumented in the tool description. True nicety only — the whole-profile write prevents data loss either way. Leaving as-is is a defensible call.✅ What I liked~
ListByProjectAsyncsort interleaves chapters becauseOrderis per-chapter, ADR 0012), names the fix (walk the workspace's flattening — the numbering's single source of truth), names the bonus (drops the second bible query), and explicitly claims the break-test. That's how a fix commit should read.SeedPagewas extended, not duplicated. TheinChapterparameter with?? chapterIddefault preserves every existing single-chapter test verbatim — zero churn to the 10 tests from round 1. Surgical.Chapter(order 1) intochapters.Itemsrather than faking the workspace — so it exercises the realGetProjectWorkspaceflattening, not a stub. If the workspace's ordering ever drifts fromPageByNumber's contract, this test catches that too. Belt and suspenders."The project no longer exists."if the workspace read returnsErr(TranslationExecutor.cs:46–49) — anot Ok<>pattern match, no silent fallthrough. Sharp.The translation agent is ready, scarlet. ADR 0013's promise — a run error, visible on the monitor, never a silent skip — now holds on the multi-chapter books it was built for. Ship it. ♡
Automated review by Jibril · 2026-07-26
CI/CD: stale for head
7bfa4f7(coverage bot 4248 covers pre-review state; no fresh CI for new commit) · Local checks: build 0 warnings/0 errors, full Orihon.slnx 542/542 pass (154 BlazorAdapter + 76 Domain + 93 Integration + 219 UseCases; +1 UseCases since round 1 = exactly the new multi-chapter test). Break-test confirmed RED→GREEN.