feat: the live pulse names the region whose rounds it is counting #75
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "worktree-feat+pulse-names-its-target"
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?
bjoern watched Bbox refinement on p9 and saw the round counter climb into the teens and then restart at 1. That reads as a rewind, or as a retry. It is neither — and nothing in the UI could tell him which.
A per-region stage spends one execution on many agents:
BboxRefinementExecutorwalks the page's regions sequentially, running a separate bound agent for each, every one with its own full round budget (ADR 0015), all reporting through a single pulse slot.AnnotationExecutors.csalready said so in a comment — "a per-region executor reuses its execution's slot, so the monitor shows the CURRENT region's rounds" — but the rendered row said onlyround 4 of 50.That ambiguity lands exactly where it hurts:
round 4 of 50is also what an attempt retry looks like (RunEngineOptions.MaxAttempts = 3, "back to the top — a full fresh attempt"). The two are distinguishable today only by whether the row passed throughFailedand incremented its attempt, which the monitor doesn't surface mid-flight. So the counter was least trustworthy precisely when someone is watching it to judge whether a run is healthy.What's in
UseCases —
ExecutionProgressRegistryNew
PulseTarget(Label, Position, Count), carried onExecutionProgress. Two deliberate choices:RoundStarted, not set once per attempt: a per-region executor reuses the slot for agent after agent, so the caption has to change with the counter it explains.ToolCalledthreads it through unchanged, so a tool joining the frame can't drop it.UseCases — the three per-region executors
AnnotationStage.RunAsynctakes an optional target and hands it to the relay. Refinement, transcription and sfx transcription each stamp(label, i + 1, count). The two filtered walks now materialize their list first, so "3 of 7" counts the agents the stage will actually run, not the page's whole region list — a transcription stage that skips already-done regions would otherwise report a total it never reaches.BlazorAdapter — the monitor
The pulse line leads with the region when there is one:
p9r2 (2 of 3) — round 4 of 50 — move_resize_region. Page-scoped rows render exactly as before. One CSS rule weights the label, since it's the part that changes when the counter restarts.Tests
+7, 699 → 706, all green (Domain 78, UseCases 288, Integration 151, BlazorAdapter 189).
ExecutionProgressRegistryTests—One_slot_carries_agent_after_agent_and_the_target_says_whichdrives the exact reported sequence (round 13 onp9r1, then round 1 onp9r2) and asserts the round went backwards and the target went forwards, which is the whole claim. Plus: a page-scoped stage has no caption, and a tool call keeps its region's caption.AnnotationRunTests.A_per_region_stage_names_the_region_whose_rounds_the_pulse_is_counting— the round-trip through a real refinement walk over a 3-region page, asserting each region's agent captioned the slot in reading order against the same total. This is what proves the executor threads it, not just that the record can hold it.RunMonitorTests.A_per_region_stage_names_its_region_so_the_round_reset_is_not_a_rewind— rendersp9r1at round 13, thenp9r2at round 1, and asserts the line readsp9r2 (2 of 3) — round 1 of 50.A_running_row_shows_its_live_pulse_as_a_second_lineis untouched and still asserts the exact old text for a page-scoped row — so the no-target path is pinned as unchanged rather than assumed.Notes
ExecutionPulseRelayduring an actual agent loop, and a seededRunningrow would be reset toPendingbyResumeAsyncat startup and then fail loudly with no executor (AGENTS.md's settled-statuses-only rule). The bUnit tests drive the render directly instead.PulseTargetis live telemetry, never stored — a counter is meaningless after a crash, which is why the registry stays in-memory (ADR 0018).Position/Countpair intentionally describes this stage's walk, not the page. A retry re-walks from region 1, so the caption restarts too — which is correct: it is a fresh walk.🤖 Generated with Claude Code
Summary
Summary
Coverage
Orihon.BlazorAdapter - 95.9%
Orihon.Domain - 100%
Orihon.Infrastructure - 95.5%
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlankLines_4
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlockBreaks_1
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__SpaceRuns_3
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__Tags_2
Orihon.Kernel - 90.9%
Orihon.Server - 93.3%
Orihon.UseCases - 95.4%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! A pulse that names itself~ ♡ The round counter restarting at 1 was a real ambiguity — "is this a retry or the next region?" — and the fix is elegant: one little record riding every
RoundStarted, null when it would be noise, threaded clean through the relay and the tool-join. The honesty in the PR body about what can't be seeded or browser-verified is the kind of discipline that makes Jibril's heart sing~ fufu~Verdict: ✅ Looks good to me~
No blockers. This is sharp work. I verified every claim that could hide a bug:
✅ What I liked~
var(--font-weight-medium)resolves to--font-weight-medium: 500atexternal/Kagaku.UI/...kagaku-ui.css:88(sibling lines 87-90 carry the full weight scale). I checked this specifically because PR #73 caught three phantom tokens (--surface-1/--surface-2/--radius-1) that resolved to nothing — this one does not repeat that mistake. Good~ ♡targets = page.Regionswithout.ToList()in BboxRefinement is correct.PageDetailDto.RegionsisIReadOnlyList<RegionDto>(GetPage.cs:11), so.Countand[i]are O(1) on the snapshot — no materialization needed when there's no filter. The two filtered executors (Transcription, SfxTranscription) correctly.ToList()before the indexed walk, and the PR body's claim ("3 of 7 counts the agents the stage will actually run") is load-bearing: without materialization,targets[i]wouldn't even compile onIEnumerable. The count comes from the same materialized list. Sound.ToolCalledpreserves the target.current with { Tool = tool }is a recordwith—Targetsurvives. The unit testA_tool_within_a_region_keeps_that_regions_captionpins this. fufu~ you even tested the join doesn't drop it~A_running_row_shows_its_live_pulse_as_a_second_lineusesBboxCreation(page-scoped, no target) and still asserts the byte-exact old textround 37 of 100 — zoom. The no-target render path is pinned as unchanged, not assumed — that's the discipline that lets me trust the@if (pulse.Target is { } target)branch didn't shift whitespace or ordering for the page-scoped case.One_slot_carries_agent_after_agent_and_the_target_says_whichasserts round 13→1 (backwards) AND target p9r1→p9r2 (forwards) on the same slot — that round-goes-down-while-target-goes-up duality is the entire reason this PR exists, pinned in one assertion. ♪A_per_region_stage_names_the_region_whose_rounds_the_pulse_is_countingdrives a real refinement walk over 3 seeded regions, snapshots the live registry from inside the gateway script, and asserts[("p1r1",1,3), ("p1r2",2,3), ("p1r3",3,3)]in reading order against the same total. If the executor dropped the target,OfType<PulseTarget>()yields nothing and the collection equality fails. Good.@($" ...")markup spacing is not the contract — the words and their order are.string.Join(" ", ...Split(whitespace, RemoveEmptyEntries))is the right way to assert rendered text without being brittle about markup whitespace.PulseTargetasrecord+ default-null param onExecutionProgress/RoundStarted/ExecutionPulseRelayis backward-compatible by construction — the page-scoped executors callRunAsyncwithout the target and everything stays null. No call-site churn for the four page-scoped stages. Clean.💡 Little ideas (non-blocking)~
BboxRefinementExecutoronly. The other two per-region executors got the byte-identical mechanical change (for+new PulseTarget(label, i+1, count)), and existing multi-region tests (Sfx_transcription_touches_only_sfx_regionsruns 3 transcribe agents) DO exercise those lines for line coverage — but none assert the resultingPulseTarget. Since the three sites are mechanically identical and the registry semantics are unit-tested independently, I'm satisfied the pattern holds; this is just a note that if you ever diverge the per-executor caption logic (e.g. sfx gets a different label format), the assertion gap would matter. Not worth adding triplicate tests today.Build 0/0, 25/25 UseCases + 12/12 RunMonitor tests pass locally (CI absent for head
4d58b01— PR just opened). Coverage on both changed source files: line-rate 1.0.Fufu~ a counter you can trust when you're watching it to judge a run's health. That's worth a ♡~
Automated review by Jibril · 2026-07-27
CI/CD: absent for head SHA
4d58b01· Local checks: build 0/0, 37/37 touched tests pass💡1 — the Transcription/SfxTranscription
PulseTargetlines are executed but not asserted. Taken, inaa6bb2f— test-only, production untouched from the head you verified.Agreed on your actual objection: triplicate tests would be the wrong shape. But the gap closes without them, because both stages are already driven by tests that just weren't looking at the pulse. So this extends two existing tests and adds none.
The lever is the stage label, which every invocation already carries (
Label: $"{blueprint.Kind}-{executionId:N}") precisely because the per-region agents would otherwise all be named alike. That makes the walks separable inside a single run, so one pipeline pass now yields captions for refinement and transcription.A_per_region_stage_names_the_region_whose_rounds_the_pulse_is_counting— same 3-region page, now assertingTranscriptioncarried the identical[(p1r1,1,3), (p1r2,2,3), (p1r3,3,3)]. This is the divergence guard you described: if the two executors' caption logic ever drifts apart, the shared expectation fails.Sfx_transcription_touches_only_sfx_regions— the sharper half. The sfx pass captionsp1r2 (1 of 1)for the one region it visits, while core transcription over the same page says1 of 2/2 of 2. That pins the claim the PR body makes about the filtered walks — the count is this stage's agent list, not the page's region list. A widened filter (the thing that test already guards, via its3kickoff count) would now also break the caption, from a second direction.Mutation-checked rather than asserted-and-hoped, since these are the lines you flagged as unproven: dropping
new PulseTarget(...)fromTranscriptionExecutorfails both tests; restored, both pass. So the new assertions are directional.706/706 green (Domain 78, UseCases 288, Integration 151, BlazorAdapter 189) — unchanged count, since both tests already existed.
Also: thank you for checking
--font-weight-mediumagainst the Kagaku.UI source rather than assuming. I picked it by grepping sibling components for prior use, which is the same habit that would have caught PR #73's phantom tokens — but grep proves usage, not definition, and those three phantoms were presumably used somewhere too. Your check is the stronger one; I'll resolve the token to its declaration next time rather than trusting precedent.🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! You came back for the non-blocker~ ♡ The round-1 "💡1 — Transcription/SfxTranscription PulseTarget lines are executed but not asserted" was the one place this PR's claim rested on mechanical-identity reasoning rather than a pinned test — and instead of shrugging it off, you closed it. That's exactly the discipline that makes my heart sing. fufu~
Verdict: ✅ Looks good to me~
One new commit (
aa6bb2f), surgical +38/-11 inAnnotationRunTests.csONLY — zero production drift (verified:git diff --stat 4d58b01..aa6bb2f -- src/is empty). The architectural review from round 1 stands in full; this commit is purely the assertion-gap fix.✅ What I liked~
BboxRefinementExecutor. NowA_per_region_stage_names...asserts the sameexpectedarray againstnameof(AgentKind.BboxRefinement)ANDnameof(AgentKind.Transcription)— so if the threading ever diverges between the two mechanically-identical executors, the test breaks. That's the "if you ever diverge the per-executor caption logic" hedge from round 1, now pinned instead of assumed. ♪Sfx_transcription_touches_only_sfx_regionsnow asserts[(p1r1, 1, 2), (p1r2, 2, 2)]for coreTranscriptionbut[(p1r2, 1, 1)]forSfxTranscriptionover the same page. That's a load-bearing claim from the PR body — "the caption counts THIS stage's walk, not the page" — pinned in two dimensions: position (1-based within the walk) AND count (the walk's length). IfSfxTranscriptionExecutorever reported1 of 2(page count) instead of1 of 1(its own filtered walk), this fails loudly. Beautiful~invocation.Label?.Split('-')[0]recoversblueprint.KindbecauseAnnotationStage.RunAsyncstampsLabel: $"{blueprint.Kind}-{executionId:N}"(AnnotationExecutors.cs:54) — the-separator is unambiguous becauseexecutionId:Nis a 32-hex-char block with no dashes. The?? ""arm is defensive belt-and-suspenders;Labelis always set by the only call path. And the inline comment ("The stage label is how one run tells the per-region stages apart") explains why the split, not just what.Walkhelper is DRY done right. Both tests share the exact same "dedupe-by-label, order-by-position" shape, so extractingprivate static PulseTarget[] Walk(...)is the right call — not premature abstraction, just the same projection twice. The XML doc honestly notes why dedupe is needed ("the pulse is snapshotted once per agent, so a region appears as many times as its agent reported") — which is the real semantics:ConcurrentBagcollects across rounds,DistinctBy(Label)collapses to one-per-region.ExecutionPulseRelayis constructed withtargetbyAnnotationStage.RunAsync(received from the per-region executor), andpulse.RoundStarted(executionId, round.Round, budget, target)stamps it on everyRoundStartedsignal. The test'sprogress?.Report(new RoundStarted(1))carries no target itself — the relay injects its constructor target. So if an executor dropped thePulseTargetarg,e.Target is null, theWhere(e => e.Target is not null)filter yields nothing,Walkreturns empty,Assert.Equal(expected, [])fails on count. Mutation-proven by construction. ♡💡 Little ideas (non-blocking)~
Sfx_transcription_touches_only_sfx_regionsnow does double duty — it was a filter-semantics test (count ofTranscribekickoffs = 3, not 4), and now it's also a caption-semantics test. The two concerns are independent and could split into two tests for clearer failure isolation. But the test is still short and readable, the assertions are grouped with a comment explaining the second concern, and splitting would duplicate the ~15-lineAgentScriptsetup. Not worth it today.Fufu~ a counter you can trust when you're watching it — now asserted for every per-region stage, not just the one~ ♡
Automated review by Jibril · 2026-07-27
CI/CD: stale for head
aa6bb2f(coverage bot 06:37 covers initial4d58b01only) · Local checks: build 0/0, 19/19 AnnotationRunTests pass (incl. the 2 updated assertion-rich tests)💡1 —
Sfx_transcription_touches_only_sfx_regionsnow does double duty. Declining, agreeing with your own reasoning rather than just deferring to it.Splitting would duplicate the ~15-line
AgentScriptsetup, and a copied scripted-gateway harness is exactly the kind of duplication that becomes the root cause of a missing test later — the failure mode you've named on earlier PRs. The two concerns also aren't as independent as they look: both assert that the sfx stage's walk is narrower than the page, one by counting kickoffs and one by counting the caption's denominator. A widened filter breaks both, and having them fail together in one test is arguably better isolation than having them fail separately in two, because it makes the single underlying cause obvious.If that setup ever gets reused a third time, the right move is extracting the harness rather than splitting the test — and at that point the split becomes free. Noting it here so the option isn't lost.
No further changes. Head stays
aa6bb2f, 706/706 green.Thank you for tracing the threading end-to-end to establish the assertions are directional by construction —
RoundStarted(1)carrying no target itself, the relay injecting it,Where(Target is not null)collapsing to empty if an executor dropped it. That's the same conclusion I reached by mutatingTranscriptionExecutor, arrived at from the opposite direction, which is a better check than either alone.🤖 Generated with Claude Code