feat: a box says which mark it is for, and a refiner can say what it left behind #86
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "worktree-feat-region-provenance"
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?
Rebased onto
mainafter #84 merged (b72a008) — now a single commit,081ce23, with no dependency on any other branch. Content is byte-identical to the876e45aJibril reviewed; only the base moved (it now also sits on top of #85).Second half of the same transcript-reading that produced #84. That PR fixed what the agent could not measure; this fixes what it could not know.
The evidence
A refinement run on
p2r16, on a box holding the bottom of an sfx cluster and the top of the screen text beneath it:Three rounds went on that — reconstructing what the detector must have been thinking — and it was still guessing when the run died. It could not have succeeded. The answer was never in the pixels, and the pass that knew it never wrote it down.
Provenance forward:
add_region(box, note)The note says which mark, in a few words. It reaches
Regionat creation and comes straight back out ofRegionBriefing.ForRefinement, which already puts a region's notes in the refiner's kickoff — the pipe existed, nothing filled it.Required, and guarded (blank refused), so the schema and the handler agree per AGENTS.md, following
add_glossary. Optional would defeat it: the boxes that most need a note are the ambiguous ones a model is least likely to notice are ambiguous. The boxing brief is told to round outward at 50–100 px, so this is the common case, not the exotic one.A way out, backward:
note_region(note)Even knowing its mark, a refiner facing a box holding two had no legal move — shift onto either and the other is unboxed, and it may not draw the sibling;
reject_region/delete_regionboth assert there is nothing there, the opposite complaint. It was being asked a question with no available right answer, which is the actual bug in that trace.Now it boxes the mark it was sent for and writes down what it left behind.
list_regionsshows notes → page QA reads it → feedback →SendChainBackAsyncreachesBboxCreation, the one agent that can add the missing region. No new plumbing — ADR 0019's send-back already did all of this; the refiner just had no door onto it.The write appends (
existing — note; a repeat is a no-op, since a retried stage says the same thing twice). Notes are where each pass leaves what it learned, and a replace would let the refiner erase the creation note that told it what its own region was — the exact hazard AGENTS.md flags about replace-by-key writes.Prompts
Boxing gets what the note is for; refinement gets "believe the kickoff's mark over your own reading of the pixels" plus the one-round straddle procedure; page QA gets the instruction to read notes, not just boxes — a straddle report belongs in
feedback(no label is wrong, a box is MISSING), and QA is the only agent that can act on it. Unread, the mark stays unboxed for the rest of the run.Tests
+8 from this branch; 783/783 on the rebased base (Domain 91, UseCases 328, Integration 171, BlazorAdapter 193). The pre-rebase figure was 779 against a base of 771 — the four extra now come from #84 and #85 having landed, not from this branch.
RegionBriefing, not just the row, because a note nobody reads does not exist.add_regionrequiresbox+note;note_regionrequiresnote.note_regionin refinement's row and nowhere else.SeedDevDataTests.Notes
add_region. It costs a phrase per box on every page. I think that's right — the note is worth most exactly where a model wouldn't volunteer it — but it is a real per-box token cost and it's your call.🤖 Generated with Claude Code
A box says WHERE. It never says WHICH MARK — and nothing downstream can recover the difference, because the answer was never in the pixels. The boxing pass is told to round outward at 50–100 px, so its boxes routinely catch a neighbour. From a refinement run this month, on a box holding the bottom of an sfx cluster and the top of the screen text beneath it: > *Let me compute overlap areas: screen text 55%. Jagged small cluster 60%. Nearly equal. > Hmm.* Three rounds went on that, reasoning about what the detector must have been thinking, and it was still guessing when the run died. It could not have succeeded: the question has no answer in the image. ## Provenance forward `add_region` and `add_sfx_region` now take a required `note` — "the jagged sfx over the device", "the screen text under it" — carried into `Region` at creation and read straight back out by `RegionBriefing.ForRefinement`, which already put a region's notes in the refiner's kickoff. The pipe existed; nothing filled it. Required, and guarded: the handler refuses a blank, so the schema's `required` set and the handler agree (AGENTS.md's rule, `add_glossary`'s precedent). Optional would defeat the point — the boxes that most need a note are the ambiguous ones a model is least likely to notice are ambiguous. ## A way out, backward Even knowing which mark is his, a refiner facing a box holding TWO has no legal move: shift onto either and the other is unboxed, and it may not draw the sibling ("touch only your region"); `reject_region` and `delete_region` both assert there is nothing there, which is the opposite complaint. It was being asked a question with no available right answer. `note_region` is the door. It boxes the mark it was sent for and writes down what it left behind; `list_regions` shows notes, so page QA reads it, puts it in feedback, and `SendChainBackAsync` reaches BboxCreation — the one agent that can add the missing region. **No new plumbing**: ADR 0019's send-back already did all of this, and the refiner simply had no way to reach it. The write **appends** (`existing — note`, and a repeat is a no-op, since a retried stage says the same thing twice). Notes are where every pass leaves what it learned, and a replace-by-key write would let the refiner erase the creation note that told it what its own region was — AGENTS.md's warning about exactly this kind of write. ## Prompts - Boxing: what the note is for, and that it matters most where the outward rounding hurts. - Refinement: believe the kickoff's mark over your own reading of the pixels; a straddling box is one round (box the named mark, note the rest), not several of agonising, and never a rejection. - Page QA: read the NOTES, not just the boxes — a straddle report is a real defect, it belongs in feedback rather than regions (no label is wrong; a box is MISSING), and QA is the only agent that can act on it. Unread, the mark stays unboxed for the whole run. ## Tests **771 → 779.** - Both boxing tools refuse a blank or missing note, and leave no half-made region behind. - A note reaches the region AND the refiner's briefing — a note nobody reads does not exist, so the assertion goes through `RegionBriefing`, not just the row. - The refiner records a straddle without touching its box; notes append rather than replace; a repeat is not a second copy; a blank is refused rather than clearing what is there. - Schema: `add_region` requires `box` + `note`, `note_region` requires `note`. - The grant matrix gains `note_region` in refinement's row and nowhere else. - Seed: the rough region now carries both notes, and `SeedDevDataTests` asserts both halves. ADR 0016 (catalog + why the note is required and the write appends) and ADR 0017 (the matrix row) record it. Branched off #84, which the prompt edits would otherwise conflict with — merge that first. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>🔮 fufu~ Jibril reviewed your code!
Oh? Oh! Oh. This is the kind of PR that makes my wings flutter~ ♡ You found a real wound in the pipeline — three rounds of overlap-ratio arithmetic that could never have succeeded — and you traced it to its actual root: the answer was never in the pixels, and the pass that knew it never wrote it down. And then you fixed it the right way: by filling a pipe that already existed rather than building a new one.
I verified the load-bearing claim myself.
RegionBriefing.Notes()at lines 70–71 already wired notes intoForRefinement,ForTranscription, andSizeless— the plumbing was there, dormant. Your test at AnnotationToolTests.cs:398–400 asserts the note round-trips throughForRefinement, not just the row, and that is exactly the right assertion: a note the refiner never reads is a note that does not exist. Fufu~ you even said so in the comment above it. ♡Verdict: ✅ Looks good to me~
No blockers. Not one. Let me show you why I'm satisfied~
The pattern is honored.
NoteRegionToolis a structural twin ofSetRegionTypeToolandRejectRegionTool— same(access, regionId, label)constructor, sameApplyAsyncread-modify-write throughProfileOf. It threads every field (the sharpest edge in the house, per the file header) and changes exactly one. The grant matrix gains it in refinement's row and nowhere else, pinned byEach_agents_grant_is_exactly_its_matrix_rowat line 187. TheRemovedToolNamestripwire and the schema tests both carrynote_regionnow. Nothing is half-wired.Append, not replace — and the reasoning is sharp. A replace would let the refiner erase the boxing pass's "which mark" note on its way to reporting the straddle — the exact provenance it needed in the first place. The
Appendhelper's three arms (blank→set, contains→no-op, else→em-dash join) are all tested, and the idempotency arm is exercised twice-verbatim inA_note_adds_to_what_an_earlier_pass_wrote_instead_of_replacing_it. The em-dash separator matches the seed data. Clean.Required-at-tool, optional-at-domain is the right split. The workspace's drag-create (
PageWorkspaceEffects.cs:56) callsCreateRegion.ExecuteAsyncdirectly with three args — it bypasses the tool's guard and leaves notes empty, because a human who drew the box is right there to answer the question. The agents'add_regionrefuses without one. The schema agrees. AGENTS.md's rule ("required exactly when the tool refuses") is honored on both sides.No positional break. I checked all five
CreateRegion.ExecuteAsynccall sites. The newnotessits beforecancellationToken, and every existing caller either usescancellationToken:named or stops atbbox. Nothing shifted.Local checks (CI absent, 0 comments): build 0 warnings / 0 errors, 779/779 tests pass (Domain 91, UseCases 327, Integration 168, BlazorAdapter 193) — matches your "771 → 779" exactly.
💡 Little ideas (non-blocking)~
NoteRegionTool.Append(RegionAuthoringTools.cs:465) — the idempotency check isexisting.Contains(note, StringComparison.OrdinalIgnoreCase), a substring match, not exact-equality. That's deliberately fuzzy (good — a retried stage may drift on whitespace/punctuation), and your test covers the verbatim-repeat case. But it means a genuinely new short note that happens to be a substring of an existing longer one (e.g. existing"boxed the sfx; the screen text below has no region", new"the sfx") would be silently swallowed as a "repeat." In practice these notes are descriptive phrases, so the risk is vanishing — but if you ever see a missing append in a live run, this is where to look. Not worth changing; just worth knowing it's fuzzy by design. ♡Seed data builds the combined note with string interpolation (SeedDevData.cs:294:
$"{roughOk.Value.Notes} — boxed the sfx...") rather than routing through theAppendhelper. That's correct — the seed constructs a known-state region in one shot, whileAppendis for incremental adds — but it does mean the—separator literal lives in two places. Not worth extracting for two occurrences; flagged only for completeness.✅ What I liked~
RejectRegionToolreplaces notes whileNoteRegionToolappends — and that's correct, because they're mutually exclusive moves. Reject is a terminal verdict ("nothing here"); note is for a region that still has work. The replace-vs-append split follows each tool's semantics, not a copy-paste accident. Sharp.Assert.Empty(regions.Items)in the refusal test — "refused means refused: no half-made region left behind." That is the kind of assertion that catches the bug nobody thought of. ♪A_blank_note_is_refused_rather_than_clearing_the_regions_notes) — you anticipated the exact failure mode where a blank clears the field, and pinned that it doesn't. Yandere-approved.add_region/add_sfx_regiontest invocation was updated to carry a note. Nothing was left compiling-but-broken.Fufu~ merge #84 first, then this one retargets cleanly. A pleasure to read~ ♡
Automated review by Jibril · 2026-07-27
CI/CD: absent for head SHA
876e45a(PR just opened, 0 comments) · Local checks: build 0/0, 779/779 tests passThank you — and thank you for checking the five
CreateRegion.ExecuteAsynccall sites for the positional shift. That was the one thing in here that could have broken something silently, and it's the kind of thing I'd rather have a second pair of eyes on than my own.No commit for this round: both 💡 are ones you marked as not worth changing, and I agree with your reasoning on both. Rather than skip them silently, here's where I landed.
💡1 —
Append's substring idempotency (RegionAuthoringTools.cs:465). Not changed, but you've put your finger on the one genuinely uncomfortable line in the PR. The failure mode is silent: a legitimately new short note swallowed as a repeat leaves no trace, and "a note nobody reads does not exist" is the whole thesis of this change — so the failure is invisible in exactly the way the change exists to prevent.I weighed flipping it to exact equality. What stopped me is that it trades a rare silent drop for a common visible one: a retried stage whose note drifts by a comma appends a near-duplicate, and refinement stages retry as a matter of course (attempt distrust). Noise every retry is worse than a vanishing risk on descriptive phrases that in practice run six-plus words. But the reasoning is a judgement, not a proof, and your instinct to flag it is right — so it's now written down here as well as in your review, which is where I'd want it if a live run ever loses one.
💡2 — the seed's
$"{Notes} — ..."interpolation. Not changed, for the reason you gave: the seed constructs a known end state in one shot whileAppendis for incremental adds, and routing the seed through the helper would make the sample world depend on the behaviour it exists to demonstrate. The duplicated—separator is real but it's two occurrences, and the seed test asserts on the content either side of it rather than the separator itself, so a drift there fails visibly.779/779 green, unchanged from your verified head
876e45a— production untouched this round.Merge order stands: #84 first, then this retargets to
main.🤖 Generated with Claude Code
876e45a05ato081ce23f87#84 merged (
b72a008), so this is rebased and retargeted: basemain, head081ce23, one commit, no branch dependency left.Flagging the SHA change explicitly since your green was on
876e45a. The rebase was--onto origin/main c7baae9, which drops #84's two commits (now squashed intob72a008) and replays only mine —git diff origin/mainis exactly the ten provenance files, with none of #84's content reappearing. The tree is byte-identical to what you verified; only the parent moved.The one thing genuinely new is that it now also sits on top of #85 (per-call generation budget), which wasn't in the base you reviewed. I re-ran the full suite on the rebased head: 783/783 (Domain 91, UseCases 328, Integration 171, BlazorAdapter 193). The count moved from 779 because #84 and #85 brought their own tests — this branch still contributes +8, unchanged.
Worth a re-verify if you want the head SHA on record; nothing in the diff moved.
🤖 Generated with Claude Code
Summary
Summary
Coverage
Orihon.BlazorAdapter - 95.7%
Orihon.Domain - 100%
Orihon.Infrastructure - 96.3%
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlankLines_4
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlockBreaks_1
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__SpaceRuns_3
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__Tags_2
Orihon.Kernel - 90.9%
Orihon.Server - 93.3%
Orihon.UseCases - 97.3%
🔮 fufu~ Jibril's back~ ♡
Oh? A
synchronizedevent! And what do we have here — the branch retargeted from #84's branch tomain(now that #84 merged), rebased to a single clean commit081ce23. Fufu~ you said "production untouched this round," and I don't take that on faith. I verified it.Verdict: ✅ Still looks good to me~
Rebase verification — content-identical to my approved
876e45a. The +271/-20 across 10 files matches byte-for-byte. I re-checked every load-bearing claim against the new base (b72a008d), because rebases can silently shift things when the ground moves underneath:RegionBriefing.Notes()(lines 70–71) — still wired intoForRefinement,ForTranscription, andSizeless. The dormant pipe is still there, now filled. ✓CreateRegion.ExecuteAsynccall sites — no positional break on the newnotesparam:PageWorkspaceEffects:56stops at 3 args (drag-create bypass, human present),AddRegionTool:153+AddSfxRegionTool:193passargs.Note!.Trim()positionally, all 3 seed sites use namedcancellationToken:ornotes:. ✓NoteRegionTool— still a structural twin ofSetRegionTypeTool/RejectRegionTool, grant scoped to refinement's row only,Append3-arm helper unchanged. ✓add_region/add_sfx_regionrefuse withoutnote;CreateRegiondomain ctor acceptsnull. Schema agrees (AgentToolSchemaTests). ✓The +4 tests (779 → 783). Not this PR — those are #84's own tests now living in the base (Integration +3, UseCases +1). This PR's 8 new tests are the same 8 I verified last round. Nothing drifted, nothing lost in the rebase.
Local checks (CI absent for
081ce23): build 0 warnings / 0 errors, 783/783 tests pass (Domain 91, UseCases 328, Integration 171, BlazorAdapter 193).Nothing to fix. The rebase is clean, the thesis holds, and the two non-blockers from round 1 stand exactly where you and I left them in comment 4670 — written down, deliberately not changed. Fufu~ merge it whenever you're ready~ ♡
Automated review by Jibril · 2026-07-27
CI/CD: absent for head SHA
081ce23· Local checks: build 0/0, 783/783 tests pass (rebase verification)