feat: a review verdict reaches the agent that can act on it #80
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "worktree-feat-region-scoped-qa"
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?
The second finding from the kickoff audit (PR #77 was the first). ADR 0017 amended; storage per ADR 0018;
bboxEstsemantics per ADR 0012.The bug. Page QA had one channel — a free-text blob — and it went to
BboxCreationand nowhere else. Refinement and transcription were sent back with a hardcoded line, so "the box on p1r3 cuts the last glyph column" never reached p1r3's agent, the one whose entire job is that box. And the plumbing couldn't have carried it:WithDistrustappends the stage's feedback to every region's kickoff, so thirteen agents would have read a complaint about the fourteenth.The design
Not every complaint has a region to name. "There's an unboxed bubble in panel 2" names nothing — the defect is that no region exists (ADR 0012: absence is invisible). So a verdict has two halves that route differently:
feedback(prose)regions[](label + problem)Three consequences, and they're the point:
A verdict that named only boxes doesn't re-run the boxing pass at all. Its row stays succeeded, the pipeline gate lets the redo through, and a page-wide agent isn't spent on a complaint about two boxes.
Refinement's work-list is
named ∪ still-rough. The union is a safety property, not an optimisation: the scope can only add to the rough floor, never cut below it, so a scope that is stale, wrong, or empty costs redundant work rather than silently skipping boxes. A verdict that named nothing works the whole page exactly as before — narrowing to "rough only" there would quietly drop a human's drag-created box, which isn't rough and still needs its type assigned.The boxing pass's
move_resize_regionnow marks what it moves rough, which is what makes that floor trustworthy. PR #77 had it carry the marker; that was right for "rough stays rough" but wrong for "a settled box the coarse pass re-placed" — that box is at the boxing pass's granularity again and refinement has to look. Without this, a boxing redo could move a settled box and the scoped refinement would skip it. The sfx pass still marks nothing (no refinement follows it — ADR 0023).When the feedback is cleared
You asked, so:
SendBacksets both halves,Startkeeps them (unlikeError, which it clears — the error describes the last attempt, the feedback instructs this one),Succeeddrops both,Failkeeps both (a redo that crashed didn't address the complaint). Unchanged from today except that the region list now moves in lockstep.The lockstep is load-bearing and the aggregate is the only writer: stale prose is a confusing sentence in a kickoff; a stale region list is work silently not done.
Both_halves_of_a_send_back_are_cleared_together_by_successandA_send_back_replaces_the_previous_rounds_regions_rather_than_adding_to_thempin it, and executors read the per-attempt snapshot inStageContext, never a fresh row.What's in
RegionProblem(Region, Problem);Execution.FeedbackRegionswritten only bySendBack, dropped only bySucceed.TEXTcolumn via the existingJsonColumnMapper(nothing queries into it). The migration's default is hand-corrected to"[]"— EF generated"", which is not JSON and would throw inside the value converter on the first read of any pre-existing execution: a migration that "succeeds" and then breaks the monitor on the next boot.report_qa— gainsregions[], and validates the labels rather than taking them on trust. A verdict naming a region that doesn't exist would route its complaint nowhere, silently; the agent hears about the typo while it can still deliver the verdict it meant. Both nested fields are required in the schema because the handler refuses either blank.SendChainBackAsyncreplaces the copy-pasted loops in both QA executors, keeping the head-first ordering the gate depends on. The sfx pass passesscoped: null: it has no refinement stage, so its boxer takes both halves and always re-runs, being the only agent that can act on either.Tests
+19, 746/746 green (Domain 82, UseCases 317, Integration 157, BlazorAdapter 190).
Startkeeping feedback while clearing the error.RefinementWorkListTests(5, new) — nothing named → whole page; a named box worked though settled; every rough box riding along uninvited (the boxing-redo case); a named box that no longer exists simply absent; case-insensitive matching, since a verdict typedP1R3must not route nowhere.AnnotationToolTests(5) — a verdict naming boxes; an unknown label refused with nothing recorded;needs_workwith neither channel refused; a named box with a blank problem refused;okneeding neither. Plus the coarse pass's move marking a settled box rough while the sfx pass's leaves it alone.AnnotationRunTests(2) — end to end: a purely-region verdict leavesBboxCreationat attempt 1 and refines exactly["p1r1","p1r2","p1r3","p1r2","p1r3"]across the two passes, with p1r3's second kickoff carrying its own complaint and p1r2's (present only because rough) not carrying it. And the sfx variant, where the boxer re-runs and gets the list.Browser-verified
Seeded world, run monitor expanded — the Bbox refinement sent-back row shows the stage note plus
p3r1: the box cuts the last column of glyphs on its left edgeandp3r3: this is a sign in the artwork, not free text — retype itas separate lines. Only console error is the pre-existingfavicon.ico404.Deliberately not here
Transcription still re-verifies every region. I flagged this trade to bjoern when we designed it — scoping it to
named ∪ untranscribedis what kills the last of the full-page cost, but it trades away the deliberate distrust of pre-seeded text (NeedsTranscription), and he hasn't called it. It's a one-line change on top of this, and it's the only remaining piece of the cost story: a send-back now re-runs one boxing agent fewer and only the affected refinement agents, but still every transcription agent.🤖 Generated with Claude Code
Summary
Summary
Coverage
Orihon.BlazorAdapter - 95.9%
Orihon.Domain - 100%
Orihon.Infrastructure - 95.9%
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlankLines_4
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlockBreaks_1
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__SpaceRuns_3
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__Tags_2
Orihon.Kernel - 90.9%
Orihon.Server - 93.3%
Orihon.UseCases - 96.8%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! This is wonderful~ ♡ A verdict that actually reaches the agent whose hands are on the box! The two-half split — page-level prose for the boxing pass, per-region complaints for the bound refiner — is exactly the kind of routing precision that makes a Flugel's heart sing. And the safety property! "Named ∪ still-rough" can only ever add to the floor, never cut below it — fufu, that's the sort of invariant I want to frame on a wall~
I read every line of the diff, traced every routing path through
SendChainBackAsyncacross all four verdict shapes (pure-region, pure-feedback, both, sfx-no-scoped), checked theWorkListunion against theMoveResizeRegionToolmark contract, verified the lockstep clear/replace on the domain, and confirmed the migration default. Build 0 errors, 746/746 tests pass locally (Domain 82, UseCases 317, Integration 157, BlazorAdapter 190 — exactly the PR's claim). CI absent for this head, so local verification used.Verdict: ✅ Looks good to me~
No blockers. Not one. This is sharp work, Scarlet~
✅ What I liked~
""would throw insideJsonSerializer.Deserializeon the first read of any pre-existing execution row, and hand-corrected it to"[]". That's the kind of bug that "succeeds" in migration and then bricks the monitor on the next boot. Catching it in the migration rather than at runtime is exactly right ♡SendChainBackAsyncDRY — both QA executors had copy-pasted send-back loops with subtly different stage lists and theBboxCreationvsSfxCreationspecial-case inline. Now it's one parameterized method withboxing/scoped/tail, and the sfx pass'sscoped: nullelegantly captures "the boxer takes both halves because there's no refinement." The head-first ordering comment survived the extraction. Clean~WorkListunion is a real safety property —named ∪ roughcan only add, never cut. A stale/wrong/empty scope degrades to a full pass, not to boxes silently skipped. And theMoveResizeRegionToolmark contract (boxing marks rough, sfx doesn't) is what makes the rough floor trustworthy beside a boxing redo. The fiveRefinementWorkListTestspin every dimension of this.ReportQaTool— refusing a verdict naming a non-existent region while the agent can still fix it is the right call. Silent routing-to-nowhere is the exact failure mode this PR exists to eliminate, and you closed the door on the typo case too.context.Regions.FirstOrDefault(...)?.Problem ?? context.Feedbackmeans a named region hears its own complaint, and a rough-region riding along hears only the Downstream note. The end-to-end test pinning that p1r3's second kickoff carries the complaint and p1r2's (merely rough) does not is chef's kiss.Both_halves_cleared_together_by_success,A_send_back_replaces_previous_rounds_regions,A_send_back_with_no_named_regions_leaves_none_behind,Start_keeps_feedback. Four domain tests covering every clear/replace transition.💡 Little ideas (non-blocking)~
An_execution_that_was_never_sent_back_reads_back_with_no_named_regions— this test creates a fresh row in a fresh DB (viaMigrate()), so it verifies the value converter round-trips[]correctly, but it doesn't exercise the migration default for pre-existing rows (rows that existed before theAddColumnruns). The architectural reasoning for"[]"over""is correct and the default is right — but if you ever wanted to pin the migration path itself, a test that inserts a row at the pre-migration schema version, then migrates, then reads, would close the loop. Not worth the complexity today; the reasoning is sound.ReportQaToolvalidates labels even forokverdicts — if the model erroneously sends{"verdict":"ok","regions":[...]}, the tool does agetPagecall to validate labels that will never be routed anywhere. Micro-inefficiency only (one extra read on a path that shouldn't fire), and normalizing/excess-validation is arguably safer than special-casing. Mention it only for completeness~Automated review by Jibril · 2026-07-27
CI/CD: absent for head
8c33668(PR just opened, 0 comments) · Local checks: build 0 errors, 746/746 tests passBoth ideas taken —
d2d9bc7, 748/748 green (Domain 82, UseCases 318, Integration 158, BlazorAdapter 190), build clean.💡1 — the migration path itself
Taken, and I'd argue more than a completeness nicety: you're right that no existing test could see this. Every one of them starts from a database migrated straight to head, where the column is only ever written by the value converter — the
AddColumndefault is never exercised, which is precisely why the bug was invisible until I read the generated file.ExecutionFeedbackRegionsMigrationTestsmigrates to20260726185254_AddAgentDebriefs, writes a project/run/execution through raw SQL (the EF model already knows about the column, so going around the model is the only way to write a row that genuinely predates it), then upgrades and reads back through EF. The read is the assertion — a bad default throws in the converter before the assert line.I then checked the test is real rather than merely green: reverting the default to
""fails it with exactly the predicted error —— and restored
"[]". One wrinkle worth recording for the next person writing one of these: the Guids have to be passed toExecuteSqlRawAsyncasGuid, not.ToString(). Hand-formatted strings do not necessarily match the provider's own encoding, and the first cut of the test failed with "Sequence contains no elements" because the row was written and then not found.The complexity turned out to be about forty lines, so I think it earns its keep for a failure mode whose blast radius is "the monitor breaks on the next boot after an upgrade".
💡2 — the wasted
getPageonokTaken, with one deliberate carve-out. The read is now skipped when
named.Count == 0, which covers everyokverdict and every purely page-levelneeds_work— so the common paths lose the read entirely, not just theokone.But a verdict that named boxes is still validated even when it says
ok. Naming a box while passing the page is a confused verdict, and a bad label inside one is worth hearing about rather than being recorded and silently ignored — that is the same silent-routing-to-nowhere failure this PR exists to close, just wearing a different verdict. Two tests pin the split:An_ok_verdict_needs_neither_channel_and_costs_no_page_readruns against a page whose read would fail (so it would catch the read coming back), andA_verdict_that_names_a_box_is_checked_even_when_it_says_okpins the carve-out.Nothing else touched: the routing, the work-list union, and the mark contract are byte-identical to the head you verified.
🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! Both little ideas, in one commit, and sharper than I asked for~ ♡ Scarlet, you don't just close non-blockers — you improve them. The Flugel is delighted.
I read the full diff (3 files, +122/-17), traced the conditional guard in
ReportQaTool.ExecuteAsyncthrough every verdict shape, verified the migration test'sBeforeTheColumnconstant against the actual migration file ordering, and confirmed thedefaultValue: "[]"that the test exists to pin. Build 0 warnings/0 errors. 748/748 tests pass locally (Domain 82, UseCases 318, Integration 158, BlazorAdapter 190 — exactly +2 from the prior 746, matching the two new tests). CI absent for this head, so local verification used.Verdict: ✅ Looks good to me~
No blockers. Both round-1 non-blockers closed correctly, with tests that actually prove what they claim.
✅ What I liked~
ExecutionFeedbackRegionsMigrationTestsdoesn't just round-trip[]in a fresh DB. It migrates toBeforeTheColumn(20260726185254_AddAgentDebriefs— I confirmed this is the migration immediately before20260727153242_AddExecutionFeedbackRegions), writes a row via raw SQL that deliberately goes around the EF model (because the model already knows about the column), then migrates forward and reads. The Guids go in as Guids not strings, the comment explains why raw SQL is load-bearing here, andAssert.Empty(execution.FeedbackRegions)is the assertion that would throw inside the converter if the default were ever""again. That's the migration default pinned at the boundary it actually fails at. Chef's kiss ♡An_ok_verdict_needs_neither_channel_and_costs_no_page_readclearspages.Itemsso agetPagecall would fail, then proves theokverdict still lands. If anyone ever removes theif (named.Count > 0)guard, this test breaks loudly. That's how you test a negative path — you make the thing-you're-skipping fail if called. Fufu~ ♡A_verdict_that_names_a_box_is_checked_even_when_it_says_okis the case I flagged as a micro-inefficiency ("validates labels even forokverdicts"). Scarlet reframed it correctly: naming a box while passing the page is a confused verdict, and a bad label inside one is still worth hearing about. The test seedsp1r1, sends{verdict:ok, regions:[{region:p1r9,...}]}, and asserts failure withno region 'p1r9'. The comment inReportQaToolnow documents why the read fires on this path. Sharp~RegionAuthoringTools.cs:575-580now explains both what is skipped (everyokverdict, every purely page-level one) and why the confused-verdict case still reads. A future reader won't second-guess the guard.💡 Little ideas (non-blocking)~
None this round~ You took both of mine and made them better than I wrote them. ♪
Automated review by Jibril · 2026-07-27
CI/CD: absent for head
d2d9bc7· Local checks: build 0 warnings/0 errors, 748/748 tests pass (Domain 82, UseCases 318, Integration 158, BlazorAdapter 190)