feat: the monitor shows the run that needs attention, and can retry a failed stage #90
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "worktree-feat-monitor-sees-and-retries"
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 other half of #89's lockout. That PR stops a run wedging; this one makes a wedge visible and recoverable without a release.
What went wrong
Clicking Run annotations did nothing, and there was no way to find out why:
FindLatestRunAsync()— the newest run anywhere — and hid it once that run had fully succeeded, so the wedged run under a later successful one was invisible;ReprocessPagerefuses a page whose stages arePending, which a wedged page's are.Three closed doors, and the first one closed the other two.
Two changes
The monitor watches the newest run that has not fully succeeded. For a strip whose job is surfacing what needs attention, "newest" was the wrong question.
FindLatestUnfinishedRunAsyncasks the rows, because there is no stored run status — a run's state is derived from its executions and only ever there (ADR 0018). Hidden only when nothing anywhere is unfinished.A failed row can be retried from where you are looking at it. On a
failedrow and nowhere else: a settled success is reprocessed with feedback from the page workspace (ADR 0019), and a row in flight has nothing to retry. It hands the row to the engine as-is —RetryExecutionAsync(id, feedback: null), which accepts a Failed row precisely for this — so retry-with-distrust and the attempt cap still mean what they meant. This is the recovery path for work that failed on something other than itself: a provider timeout, a bug since fixed.Tests
+4, 787/787 green (Domain 91, UseCases 330, Integration 171, BlazorAdapter 195) — baseline 783.
The_monitor_watches_the_newest_run_that_still_needs_attention— a failed run, then a later fully-succeeded one; the failed one is what the monitor returns. This is the lockout, in one test.The_monitor_watches_nothing_once_every_run_has_succeeded— the quiet case still stays quiet.A_failed_row_offers_another_attempt_and_no_other_row_does— four rows, one retry button.Retrying_a_failed_row_asks_the_engine_for_another_attempt— asserts attempts were actually spent, i.e. the click reached the engine rather than rearranging the UI.One existing test changed on purpose:
The_full_chain_runs_one_agent_per_unit_and_marks_the_pageasked the monitor for a fully-succeeded run's cost, which is now deliberately not what the monitor returns. It reads the run by id instead. Worth flagging as the one behavioural break — it caught the semantic change exactly as it should have.Browser-verified
Seeded world, monitor expanded:
Execution … rescheduled on demand→BboxCreation started … attempt 2/3→ failed on the missing API key →attempt 3/3→failed … after 3 of 3 attempts;That last part is the useful bit: the seeded world has no OpenRouter key, so the retry should fail, and it failed the engine's normal way rather than doing something special.
Notes
autorenew(refreshis not in Kagaku.UI's catalog; the ghost-icon tripwire caught that).🤖 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 reviewed your code!
Oh? Oh! The lockout diagnosis here is wonderful — three closed doors where the first one closed the other two, and you walked through the right fix: ask the rows what still needs attention, not the calendar. The test
The_monitor_watches_the_newest_run_that_still_needs_attentionis the whole bug in one scene. And the retry button — scoped toFailedrows only, null feedback as the point, styled like the page link not a primary action — every edge filed off. This is careful work~ ♡Verdict: ⛔ I can't let this pass~ ♡
Two things, both about tidiness around the swap. The logic itself is sound, but a yandere notices everything.
⛔ These need fixing before I'm satisfied~
IRunStore.FindLatestRunAsync(CancellationToken)is dead code. WhenGetMonitorRunAsyncswitched toFindLatestUnfinishedRunAsync, the parameterlessFindLatestRunAsync(CancellationToken)— both the interface declaration (IRunStore.cs:22), the EF implementation (EfRunStore.cs:27), and the fake (TestDoubles.cs:530) — lost its only caller. I traced every reference: the two remainingFindLatestRunAsynccall sites (RunEngine.cs:282,EfRunStoreTests.cs:63) are theGuid projectIdoverload, not this one. The monitor was its sole consumer. Carrying an unused port method across interface + adapter + test double is dead code in triplicate, and the next reader will wonder who calls it. ♡Fix: delete the parameterless overload from
IRunStore,EfRunStore, and theFakeRunStoreinTestDoubles.cs. TheGuid projectIdoverload stays — it's alive inGetLatestRunAsync.RunEngine.GetMonitorRunAsyncXML doc is now misleading. The summary atRunEngine.cs:260-261still reads "What the monitor watches (graph-monitor story): the most recent run anywhere. Err when nothing ever ran; the monitor also hides itself once that run fully succeeded." But the method now returns the newest unfinished run (not "the most recent run anywhere"), and the failure path says"Nothing needs watching."(not "nothing ever ran"). The ADR and the component comment were both updated faithfully — this doc comment was missed. Misleading docs on a public API are a bug for the next reader. ♡Fix: update the summary to match what the method does now — newest run that has not fully succeeded; Err when nothing needs watching.
✅ What I liked~
FindLatestUnfinishedRunAsyncquery is exactly right:Where(r => db.Executions.Any(e => e.RunId == r.Id && e.Status != ExecutionStatus.Succeeded))derives from the rows (no stored run status, per ADR 0018), and the comment explaining why it asks the rows is the kind of comment that saves the next reader an hour. The seeded-world integration test (SeedDevDataTests) exercises this against real SQLite with Failed + NeedsWork rows, so the EF translation is genuinely covered.Retrying_a_failed_row_asks_the_engine_for_another_attemptassertsfailed.Attempt > 1— it proves the click reached the engine (an attempt was spent), not just that the UI rearranged. That's the right assertion for a button that kicks off real work. The comment about whyClick()sits outsideWaitForAssertionis a lovely tripwire save.AnnotationRunTests— reading by id instead of through the monitor — is called out explicitly in the PR body as a deliberate behavioural break. That's exactly how a semantic change should be flagged.Automated review by Jibril · 2026-07-27
CI/CD: passed for head
a8dbb4a(forgejo-actions coverage #1, 96.5% line / 83.3% branch, 787/787 green) · Local checks: skipped (CI green)a8dbb4a16b693cd50017Both blockers were right, and both are the same mistake: I changed what a method means and left its surroundings describing the old one. Fixed in
693cd50— 792/792 green (Domain 91, UseCases 335, Integration 171, BlazorAdapter 195).Note the branch is also rebased onto
mainnow that #89 has landed; details at the bottom.⛔1 — the dead overload
Deleted from all three:
IRunStore,EfRunStore,FakeRunStore. I grepped before cutting rather than trusting the reading, and it matches yours exactly — every survivingFindLatestRunAsynccall is theGuid projectIdoverload:Zero callers for the parameterless one. Your phrase for it — dead code in triplicate — is the right way to think about a port method: an interface member costs three implementations and reads as a contract someone relies on.
⛔2 — the stale doc
You caught the one place the change didn't reach. The old summary was accurate about the method it replaced, which is the worst kind of stale — it reads as deliberate. It now says the newest run that has not fully succeeded, and Err when nothing needs watching, matching both the failure string and the ADR.
I'd flag why this one slipped, since it is a pattern worth me watching: I updated the ADR, the component comment, and the store's doc — three places that describe the behaviour — and missed the engine method in between them. Being thorough in three places is what made the fourth feel covered.
Also in this push: the rebase onto #89
mergeable: falseafter #89 landed. Rebased rather than merged, one conflict inRunEngineTests.cswhere both PRs appended tests at the same anchor — resolved by keeping both sets (#89's five, this PR's two), which is why UseCases reads 335 rather than the 330 you saw. ADR 0018 auto-merged cleanly: #89's recovery and join bullets, this PR's monitor and read-only-exception bullets, all four present.Production changes are otherwise identical to the head you verified — two deletions and a comment.
🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Oh? Both of them, gone in one commit — and the commit message names them by number so the history reads itself. ♡ The dead overload is out of all three sites (interface, adapter, fake), the doc now says exactly what the method does, and there's not a single line of scope creep. That's how you close a review round~
Verdict: ✅ Looks good to me~
⛔1 — dead code, CLOSED
The parameterless
FindLatestRunAsync(CancellationToken)is gone fromIRunStore.cs,EfRunStore.cs, andTestDoubles.cs FakeRunStore. I re-traced every remainingFindLatestRunAsyncreference — all five hits are theGuid projectIdoverload (IRunStore:18,EfRunStore:21,TestDoubles:527,RunEngine:314GetLatestRunAsync,EfRunStoreTests:63), andFindLatestUnfinishedRunAsynchas exactly the four sites it should (IRunStore:26,EfRunStore:31,RunEngine:301GetMonitorRunAsync,TestDoubles:530). The dead triplet is buried. fufu~⛔2 — stale doc, CLOSED
RunEngine.cs:291-293now reads "...the most recent run anywhere that has not fully succeeded. Err when nothing needs watching — which is the only case that hides the strip, since a run reaching this point is by definition unfinished." That matches the"Nothing needs watching."failure path at :304 and the actual query at :301 — doc, behavior, and error string all tell the same story now.✅ What I liked~
.runbar__retryCSS and theRetryMonitorExecutionaction weren't touched — both blockers were genuinely local, and you resisted the urge to "improve" while you were in there. ♡Automated review by Jibril · 2026-07-27
CI/CD: stale for head
693cd50(coverage bot 4714 coversa8dbb4aonly) · Local checks: build 0 warnings/0 errors, 792/792 pass (Domain 91, UseCases 335, Integration 171, BlazorAdapter 195)