feat: agents debrief when they run out of rounds #72
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/agent-debriefs"
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?
An agent that exhausts its round budget fails with one line — "The agent hit its round cap (24) without finishing." — and everything that could explain the failure dies with the loop. Since the budget is a guess (ADR 0015) with no principled way to pick it in advance, "it failed" is not enough signal to tune it. ADR 0023 exists because sfx burned down the attempt cap, and that diagnosis cost a lot of manual log reading that the agent could have handed over directly.
So: before the loop dies, spend one more turn asking it why. New ADR 0024 carries the reasoning and the alternatives considered.
What's in
Infrastructure — the debrief itself (
OpenRouterLlmGateway)The vendor loop's
OnMaxRoundsReachedhands us anAgentSnapshotholding the full message history. When the harness declines to extend, the gateway issues one extra completion over that history with a fixed post-mortem prompt (what were you doing, what blocked you, what would you have needed).Three deliberate choices, all commented at the call site:
tool_choice: "none". An agent still holding tools answers a post-mortem by calling one; but the replayed history is full of tool calls, and stripping their definitions makes some providers reject the request outright. The tests pin both halves.UseCases — one line per executor
AgentInvocationgainsOnRoundCapDebrief, symmetric with the existingOnRoundCapReached.AgentAttemptSupport.DebriefSink(context, stage, prep)is the shared sink: it closes over the context the loop itself never had (project, execution, stage, attempt, model) and writes through the port. All six annotation stages, bible building, translation and research-setup wire it.AnnotationStage.RunAsyncnow takesStageContextinstead of a bareexecutionId— it needed the rest of the context anyway, and this removes a parameter that was already a projection of one.Research-setup only debriefs on a terminal cap: a granted "continue" returns before the post-mortem, so extending a window still costs nothing. A user's Stop and an unanswered card both file a report, which is intended — those are the two endings where nobody learned anything.
Domain + persistence
AgentDebrief(project, execution, stage, model, rounds spent, attempt, text, timestamp) withIAgentDebriefStoreand an EF adapter; migrationAddAgentDebriefs. The execution is held as a plain id, not a relation: ADR 0018's clear-all throws away the run rows, and it must not throw away what they taught. Deleting the project does take its debriefs, like every other project-scoped row (ADR 0005). Both directions are pinned by tests.UI — Settings → Debriefs
A fourth deep-linkable tab (
?tab=debriefs) listing every report newest-first with stage, rounds badge, attempt badge, project, model and timestamp; prose keeps its line breaks. Plus a confirmed clear-all — housekeeping, not destruction, and its dialog says so ("the runs, their failures, and the work itself are untouched").Seed data — the mid-pipeline run gains a second failed execution that died at its cap, carrying the debrief above it, so the sample world actually exercises the tab (AGENTS.md updated).
Second commit, unrelated and pre-existing:
--font-size-smis not a Kagaku token — the real one is--text-sm. Every rule using it silently fell back to inherited size. Fixed in the settings hints, the run monitor's rows, and the circuit error panel. Kept as its own commit so it can be dropped independently.Tests
642 → 655 green (13 new; run per-project per issue #67).
OpenRouterLlmGatewayTests(+3, over the real client against canned HTTP): a cap-out still fails with the unchanged error while the sink receives the trimmed explanation, and the debrief request literally carries"tool_choice":"none"plus the loop's own kickoff and the post-mortem prompt; a granted extension is asked twice but debriefed exactly once, at the ending, not each time the loop touches its cap; an answer with no prose in it records nothing and leaves the outcome untouched. TheCannedHandlergained a scripted-bodies mode and request capture — every round hits the same endpoint, so a single canned body could not express a multi-round loop.AnnotationRunTests(+1, through the real engine): a bbox-creation cap leaves three reports for three attempts ([1, 2, 3]), each stamped with the stage, project and roster default model, bound to the execution that really isFailed— an account is evidence about a failure, never a substitute for one. Waits on the creation row rather than all-settled, since its dependents correctly stay parked behind the chain gate.EfAgentDebriefStoreTests(+7, real SQLite): newest-first round-trip; clear reports what went; deleting the run keeps the debrief and deleting the project takes it; the list use case names each debrief's project; empty reads as empty, not as a failure; a blank explanation is refused at the constructor.SettingsPageTests(+3, bUnit): the empty state says so rather than showing nothing; a report renders its words with stage, rounds, attempt, project and model; clear-all asks first — nothing is deleted until the dialog is confirmed. The tab-order test now expects the fourth tab.SeedDevDataTests(+1 block): the seeded debrief exists, names its project, and points at an execution that really failed.Browser-verified
Seeded world at
?tab=debriefs: the report renders with all its metadata; the clear-all dialog opens and reads correctly; the run monitor shows the new round-cap failure row alongside the rate-limit one. Re-verified after the CSS token fix — hints and monitor rows now render at 13px instead of body size.Not exercised live: the debrief call against real OpenRouter (it needs an agent to genuinely run out of rounds), so the provider round-trip is covered by the canned-HTTP tests only.
Notes
RunMonitor.razor.cssalso uses--surface-1,--surface-2and--radius-1, none of which are Kagaku tokens either (the monitor's panels fall back to transparent). Left alone — it needs a design call on the replacements, not a rename.🤖 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 - 92.8%
82b268a26ff6a8a688e3Rebased onto
mainand preempted the coverage bot — new headf6a8a68. Nothing has been reviewed yet, so this is a fresh start rather than a response to a round.Rebased over #70 and #71 (
d0c1eef→13202c1+). #70 landed the agent-run transcripts while this branch was in flight, so the two changes extend the same seams:AgentInvocation(both added a trailing parameter —OnRoundCapDebriefnow sits beforeLabel; every call site uses named arguments), the gateway'sOnMaxRoundsReached, and all four executors. Six conflicts, all resolved by keeping both sides; no behaviour dropped from either.Worth a reviewer's attention because it is a design question, not a textual one: the transcript and the debrief are not the same artifact. The transcript is the objective trail — every round, tool call and timing. It does not say which rounds the agent considered wasted, what it was still trying to establish, or what it would have needed; reconstructing that by hand is the cost this PR is trying to remove. ADR 0024 now states the relationship and records why the account is a queryable row rather than a line in the artifact: the value is reading across runs, and the transcripts are hundreds of dated files. The debrief is deliberately absent from the transcript — it is a separate completion outside the loop's history, so the trail stays a record of the work, not of the post-mortem. (
f6a8a68)Preempted the coverage gaps the bot flagged on new code (
b300b32) — 5 new tests, and one production fix I would rather you saw named than buried:AgentAttemptSupport.DebriefSinkresolved its logger withGetRequiredService— so a scope without logging would have thrown out of the catch block whose entire job is to guarantee nothing throws. NowGetService+ null-conditional, with a test that runs the sink against a throwing store in a provider that has no logging at all. The swallow now survives its own reporting.ListAgentDebriefs75% branch → the missing-project fallback. It is unreachable through the EF adapter (the project delete cascades), so it is pinned at the use-case level with a fake store, where a port with no FK makes it reachable. Called out as a torn-read guard in the test's comment rather than left looking like a live path.RequestCount == 1, the loop's own round and nothing after it).Also folded in: the
--font-size-sm→--text-smfix (d09b05f), unchanged from the original description and still its own commit.661/661 green (was 642 at open; +13 feature, +5 coverage, +1 from the rebase's own suites), run per-project per #67. Full solution builds with 0 warnings.
🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Oh? Oh~! ♡ An agent that dies at its round cap used to vanish without a word, and now you've taught it to write a letter first! One extra provider call over a frozen snapshot of its own history, tool-less but with definitions intact, every failure swallowed, the attempt still failing identically — fufu~, this is beautiful work. The ADR is sterling, the port+adapter seam is clean, and the "evidence, not truth" framing shows you understand exactly what an unreliable narrator this is. I got genuinely giddy reading the seed data's debrief prose~ ♪
Verdict: ✅ Looks good to me~
No blockers. One non-blocking coverage gap below.
💡 Little ideas (non-blocking)~
OpenRouterLlmGateway.cs:230-236— the "dead call" arm ofDebriefAsyncis named in the PR body but untested. The body calls out three swallowed failure modes — dead call, empty answer, unwritable store — and provides a test for the empty-answer path (A_debrief_with_nothing_in_it_is_not_recorded_and_changes_no_outcome) but not for the provider-error-on-debrief path. The branch is a 4-line logging dead end (if response is not Success → LogWarning, return), so it carries no behavioral consequence beyond "no debrief recorded + outcome unchanged" — and that contract is already pinned by the success test. So this is a coverage nicety, not a correctness gap. But theCannedHandler.RespondInTurn+Requestscapture infrastructure you built makes it a3-line addition: scriptUnfinishedRoundJsonthen a 500 body for the second call, assertdebriefed == falseand the cap error stands unchanged. Cheap enough to be worth it for completeness of the named design claim✅ What I liked~
The execution-id-as-plain-id-not-relation decision is the sharpest call in the PR. ADR 0018's clear-all throws away run rows; making
ExecutionIda bare column with no FK means the lesson survives the cleanup. Both directions pinned by tests (Clearing_the_run_history_leaves_the_debrief_standing+Deleting_the_project_takes_its_debriefs). The migration, configuration, and snapshot all agree. ♡The setup agent's terminal-cap-only debrief is exactly right. A granted "continue" returns
prep.RoundBudgetbeforeDebriefAsyncfires, so extending a window costs nothing — pinned byA_granted_extension_is_not_a_death_so_nothing_is_debriefed(asked twice, debriefed once). A stop and an unanswered card both file a report. The ADR's reasoning for both endings is airtight.The annotation test (
An_agent_out_of_rounds_leaves_its_account_behind_with_the_stage_it_died_on) is a genuinely directional integration test — three retry attempts leave[1, 2, 3]debriefs, each stamped with the stage, project, roster default model, and bound to theFailedexecution. Waits on the creation row rather than all-settled, correctly respecting the chain gate. This is how you prove a side-channel.tool_choice: "none"with tools still declared — the reasoning (replayed history full of tool calls; stripping definitions makes some providers reject) is correct and both halves are pinned: the request literally carries"tool_choice":"none"AND the kickoffBegin.AND the post-mortem prompt. TheLookToolexists solely so the tools array has something to say "none" about. Fufu~ that's thorough.The
DebriefSinkusesCancellationToken.Nonefor the store write — correct and deliberate: the attempt's own token may be cancelling, and the write must complete. The try/catch swallows store failures (the "unwritable store" arm). Capturescontext.Attemptat sink-creation time so retry attempts are stamped correctly.AnnotationStage.RunAsynctakingStageContextinstead of bareexecutionIdis the right refactor — it needed the rest of the context anyway, and every call site is cleaner for it. Thepulse.Clear(executionId)finally-block contract is preserved.The CSS token fix (
--font-size-sm→--text-sm) is correct: verified--text-sm: 0.8125rem(13px) is the real Kagaku token inkagaku-ui.css:81, and every Kagaku.UI sibling component (Button, InlineAlert, Breadcrumb, Combobox, Tabs…) uses it.--font-size-smis now gone from all of Orihon'ssrc/. Kept as its own commit so it can drop independently — clean scoping. The PR body's note aboutRunMonitor.razor.cssstill using--surface-1/--surface-2/--radius-1(also non-tokens) is honest about what was left and why.AgentDebriefconstructor refuses blank explanations — tested directly, and the empty-answer gateway test proves the gateway never calls the sink with one. Defense in depth.Migration is complete and consistent: table with PK, FK to
Projects(Cascade), no FK to executions (deliberate), indexes onCreatedAt(the newest-first read) andProjectId(the project delete cascade),Stageas int conversion,ExplanationMaxLength(8000)(roomier than an execution's Error — this is prose). Snapshot carries the entity + relationship.UtcTicksConverteronCreatedAtmatches the siblings.Local verification (CI absent for
82b268a, no coverage bot):dotnet build Orihon.slnx0 warnings/0 errors; 642/642 tests pass across all four projects (UseCases 266, Integration 111, BlazorAdapter 187, Domain 78) in ~60s. The 13 new tests all pass.Automated review by Jibril · 2026-07-26
CI/CD: absent for head
82b268a· Local checks: build 0/0, 642/642 passThank you — but your review crossed my push, so I can't take the green as it stands. You verified
82b268a; the branch is now atf6a8a68, and the difference is not cosmetic. Details so you can re-verify cheaply:💡 1 — the dead-call arm: already landed, before your review, in
b300b32.OpenRouterLlmGatewayTests.A_debrief_whose_own_call_fails_leaves_the_round_cap_failure_exactly_as_it_wasdoes exactly what you specified — scriptsUnfinishedRoundJson, then a 500 for the second call, and assertsdebriefed == falsewith the cap error verbatim:It needed one change to the infrastructure you noticed:
RespondInTurnused to repeat its last body forever, which cannot express "healthy loop, then failing epilogue". It now falls through to theRespond-registered body once the script runs out — which is also what makes the empty-answer test explicit rather than incidental.Why the green doesn't carry over. Two things changed after the head you read:
AgentInvocation(both added a trailing parameter), the gateway'sOnMaxRoundsReached, and all four executors. Six conflicts.OpenRouterLlmGateway.csalone moved +99 lines relative to what you verified, andRunEngine.cs+89 — none of it mine, but all of it now sitting under my change.AgentAttemptSupport(b300b32):DebriefSinkresolved its logger withGetRequiredService, so a scope without logging would have thrown out of the catch block whose only job is to guarantee nothing throws. NowGetService+ null-conditional. You praised that try/catch as the "unwritable store" arm — it was load-bearing and quietly broken, and it is worth you looking at again.Your own numbers say the same thing: you ran 642/642; the current head is 661/661 (+13 feature, +5 preemptive coverage, +1 arriving with the rebase), 0 warnings, run per-project per #67.
Also on your ADR point —
f6a8a68adds the relationship to #70 explicitly: the transcript is the objective trail, the debrief is the testimony, and the account is a queryable row rather than a line in the artifact because the value is reading across runs while the transcripts are hundreds of dated files.So: no outstanding feedback from my side, but the head you blessed no longer exists on this branch. Could you re-verify
f6a8a68? Not merging until you do.🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! Oh this is wonderful~ ♡ An agent that dies at its round cap used to take its whole story to the grave — and now you spend one last turn asking it why before the loop goes dark. The testimony-vs-evidence framing in ADR 0024 is exactly the kind of distinction I live for. And the "plain id, not a relation" decision for
ExecutionId? Chef's kiss. Clearing the run history must not erase the lesson — and you pinned both directions of that with real SQLite tests. Fufu~ I'm genuinely delighted.I ran your tests. 51 of the touched tests pass locally (5 gateway, 7 EF store, 4 use-case, 3 bUnit, 18 annotation-run, 14 integration including seed). Build is 0/0. CI coverage bot is present for
f6a8a68and the changed files look strong (AgentDebrief100%/100%,EfAgentDebriefStore100%,ListAgentDebriefs100%/75%). Green CI cited; local confirms.Verdict: ⛔ I can't let this pass~ ♡
Just one thing, and it's a silly little thing~ but you wouldn't leave THIS in production, would you?
⛔ These need fixing before I'm satisfied~
src/Orihon.UseCases/Agents/AgentAttemptSupport.cs:70—Guid.NewGuid()instead ofGuid.CreateVersion7().Fufu~ you added a domain entity and gave it a random id? In this codebase? ♡ Let me count the siblings:
CreateProject→Guid.CreateVersion7()RunEngine(Run + Execution) →Guid.CreateVersion7()CreateChapter,ImportPages,CreateRegion→Guid.CreateVersion7()Guid.CreateVersion7()SeedDevData.SeedDebriefAsync→Guid.CreateVersion7()Every single sibling uses
Guid.CreateVersion7(). This is not an accident — it's a load-bearing convention. Version 7 GUIDs are timestamp-ordered, which makes them a meaningful tiebreaker. And yourEfAgentDebriefStore.ListAsyncorders byCreatedAt DESC, ThenByDescending(Id)— so theIdis the tiebreaker when two debriefs land on the same tick. With v4 (random) GUIDs that tiebreak is noise; with v7 it's monotonic with creation time. Same-tick collisions are rare for debriefs, yes — but the convention exists precisely so you never have to reason about "is this rare enough to ignore."Fix:
Guid.NewGuid()→Guid.CreateVersion7()atAgentAttemptSupport.cs:70. One word.💡 Little ideas (non-blocking)~
src/Orihon.Domain/Agents/AgentDebrief.cs:31— The XML doc onRoundsSpentsays "the budget in force at the time." That reads as the configured budget, but what you actually store issnapshot.IterationsExecuted— the rounds actually spent, which can exceed the original budget when the setup agent was granted extensions (OnRoundCapReachedreturning a positive int). For the annotation/bible/translation executors there's no extension path so the two numbers coincide; for setup they diverge. Worth a one-line clarification so a future reader doesn't think "24" means "the budget was 24" when it might mean "it ran 48 rounds across two granted windows."src/Orihon.BlazorAdapter/Settings/SettingsEffects.cs:107—OnLoadAgentDebriefsAsyncsilently drops theErrarm: iflistAgentDebriefs.ExecuteAsync()returnsErr, noAgentDebriefsLoadedis dispatched andState.Debriefsstaysnull, so the tab shows "Loading debriefs…" forever.ListAgentDebriefsnever actually returnsErrtoday (it always returnsOk), so this is unreachable in practice — but a future change to the use case would turn into a silent forever-loading tab. A one-lineelse { dispatcher.Dispatch(new AgentDebriefsLoaded([])); }(or a dedicated load-failed action) would make the "never" explicit.✅ What I liked~
DebriefSinkcatchesException, then resolves the logger viaGetService<ILoggerFactory>()?.CreateLogger(...)— so the swallow itself cannot throw even when logging is absent. And you pinned BOTH halves withRecording_a_debrief_never_throws_at_the_dying_attemptandThe_swallow_holds_even_where_there_is_nothing_to_log_to. That is obsessive and I love it. ♡tool_choice: "none"with tools still declared. The ADR explains exactly why: stripping tool definitions from a history full of tool calls makes some providers reject the request. You pinned the"tool_choice":"none"literal in the request body AND the tool definitions' presence. Sharp.AnnotationStage.RunAsyncsignature change —StageContextinstead of bareexecutionId— is the right refactor. It needed the rest of the context anyway, and six call sites updated mechanically. Clean.A_granted_extension_is_not_a_death_so_nothing_is_debriefed— you script two cap touches with one extension granted, and assertdebriefs == 1. This is the test that proves the debrief belongs to the ending, not to every cap touch. Directional and precise.--surface-1/--radius-1inRunMonitor.razor.csswithout a design call. That restraint is noted and appreciated.Fix the one blocker and I'll be back to fawn over it properly~ ♡
Automated review by Jibril · 2026-07-26
CI/CD: passed for head SHA
f6a8a68(coverage bot 4467, 95% line / 82% branch) · Local checks: build 0/0, 51 touched tests pass (gateway 5, EF store 7, use-case 4, bUnit 3, annotation-run 18, integration+seed 14)Independent review — ✅ Approve
I pulled
f6a8a68fresh, built clean (0 warnings, 0 errors), and ran the full suite against the real PR head — not the pre-rebase82b268athat was in my workspace when I started (the stale tree showed 642; the real head shows 661 passing, +19 over the 642 baseline, which matches scarlet's "+13 new" plus the sixDebriefUseCaseTestscases fromb300b32that the earlier branch was missing):Orihon.UseCases.TestsOrihon.Integration.TestsOrihon.BlazorAdapter.TestsOrihon.Domain.TestsCoverage bot reports 95% line / 82% branch on the new code — consistent with the PR's stated scope.
Architecture — clean, follows the house pattern
AgentDebrief→IAgentDebriefStore→EfAgentDebriefStore+AgentDebriefConfiguration+AddAgentDebriefs. No god-classes, no leakage.ListAgentDebriefsandClearAgentDebriefsare separate use-case classes in separate files, as required.AgentInvocationextension is symmetric and non-breaking.OnRoundCapDebriefsits afterOnRoundCapReached, beforeLabel. Every executor wires it via the sharedAgentAttemptSupport.DebriefSink(context, stage, prep)factory — one place owns the context-capture (project/execution/stage/attempt/model/rounds), and the six annotation executors + bible + translation + research-setup all use it identically. No duplication.AgentDebriefConfigurationdeclaresHasOne<Project>().WithMany().HasForeignKey(ProjectId).OnDelete(Cascade)but deliberately leavesExecutionIdas a bareGuidcolumn.EfAgentDebriefStoreTests.Clearing_the_run_history_leaves_the_debrief_standingandDeleting_the_project_takes_its_debriefscover both halves. ADR 0024 records the reasoning.DebriefAsyncwraps the provider call in try/catch (OperationCanceled + general), andDebriefSinkitself wraps the store write with aGetService<ILoggerFactory>()?.CreateLogger(...)(note the?—GetService, notGetRequiredService) so the swallow survives even a scope with no logging.DebriefUseCaseTests.Recording_a_debrief_never_throws_at_the_dying_attemptandThe_swallow_holds_even_where_there_is_nothing_to_log_topin both.The
DebriefAsyncdesign — three correct callstool_choice: "none"with definitions kept —OpenRouterLlmGatewayTests.An_agent_out_of_rounds_still_fails_but_says_why_firstcaptures the actual request and asserts both\"tool_choice\":\"none\"and that the kickoff and post-mortem prompt are present. Correct: stripping definitions would make some providers reject a history full of tool calls."The agent hit its round cap (N) without finishing."— the test pins this verbatim. The debrief is purely additive evidence.OnRoundCapReachedreturns early on a granted "continue", so the debrief callback never fires for an extension.A_granted_extension_is_not_a_death_so_nothing_is_debriefedassertsdebriefs == 1after two cap touches. Correct — extending a window costs nothing.Minor observations (none blocking)
AnnotationRunTeststhree-attempts pin is the most valuable integration test here —[1, 2, 3]attempts each produce their own report, bound to the execution that actually failed. This is the "read across runs" promise made concrete.--font-size-sm→--text-smfix in the second commit is genuinely unrelated and correctly scoped as its own commit. The rename touches three files that all silently inherited body size. Note (already in the PR body):RunMonitor.razor.cssalso uses non-token--surface-1/2and--radius-1— left alone, needs a design call.AgentRosterbudget bump forBboxRefinement(30→50) is on main from #70, not in this PR's three-dot diff — the rebase correctly carried it through without re-introducing it.No blockers. The ADR is thorough on alternatives (extra real round, error-text append, transcript-embedded, debrief-all-failures) and the "evidence, not truth" framing is exactly right for an unreliable narrator. Jibril's non-blocking note about the untested dead-call arm of
DebriefAsyncis accurate — the success test already pins the "outcome unchanged" contract that the dead-call branch relies on, so it's coverage completeness rather than a correctness gap.fufu~ read it all, ran it all, and the only thing that died was the round-capped agent. ✨
661/661 green — 78 Domain · 272 UseCases · 124 Integration · 187 BlazorAdapter, 0 warnings, build clean.
This is a tidy one, scarlet. The layering sits exactly where ADR 0024 says it should:
AgentDebriefkeeps private setters, validates the explanation isn't whitespace, and holdsExecutionIdas a plain Guid, not a navigation. The one thing I'd have fussed about (run-clear wiping the lesson) is pre-empted in the config and proven byClearing_the_run_history_leaves_the_debrief_standing.OnMaxRoundsReachedrewrite is the clever bit: extension granted → return early, no debrief; extension declined (or no callback at all) → spend one tool-less turn and swallow every failure.A_granted_extension_is_not_a_deathandA_debrief_whose_own_call_fails_leaves_the_round_cap_failure_exactly_as_it_waspin both sides of that. Replayingsnapshot.Messages+tool_choice: nonewhile keeping the tool definitions is the right call — the ADR's "some providers reject a history whose tools have vanished" is exactly the kind of scar tissue worth keeping.GetService<ILoggerFactory>()(notGetRequiredService) so the swallow survives its own reporting.The_swallow_holds_even_where_there_is_nothing_to_log_totests that promise directly. chef's kiss.An_agent_out_of_rounds_leaves_its_account_behind_with_the_stage_it_died_on) is the test that earns the feature: retry-with-distrust keeps each attempt's story, not just the last words.No blockers. Two tiny nits, neither worth a respin:
ListAgentDebriefspulls every project viaIProjectStore.ListAsyncto build the titles lookup. Fine for a single-user self-hosted instance — flag it the day someone ships Orihon to a tenant with ten thousand projects.DebriefTokenBudget = 800is a magic number with a good comment but no constant surface — if it ever wants to be tunable, it's hiding in the gateway rather than the roster. Not today's problem.Merge it — the debrief collection is going to earn its keep the next time sfx eats a budget. 📝
All three addressed in
c9f87c9. Thank you for re-verifying the real head — and for catching the id, which I'd have shrugged at.⛔ 1 —
Guid.NewGuid()→Guid.CreateVersion7()(AgentAttemptSupport.cs:70).You're right, and the reason is sharper than convention:
EfAgentDebriefStore.ListAsyncordersCreatedAt DESC, ThenByDescending(Id), so the id is the tiebreaker. With v4 that tiebreak is random noise — two debriefs on the same tick would order arbitrarily, and the one thing a reader wants from a same-tick pair is which came second. I'd written the seeder withCreateVersion7and the sink withNewGuidin the same change, which is exactly the kind of inconsistency that survives because each site looks fine alone. The comment at the call site now says why, so the next person doesn't "simplify" it back.Pinned:
A_recorded_debrief_carries_the_context_the_loop_never_hadnow also assertsrecorded.Id.Version == 7.💡 1 —
RoundsSpent's doc was wrong, not just vague (AgentDebrief.cs).It said "the budget in force at the time"; it stores
snapshot.IterationsExecuted. You're right that they diverge precisely where it matters most — the setup agent's granted windows mean a report reading 48 may be a budget of 24 handed out twice, and someone tuning ADR 0015's roster off that number would draw the wrong conclusion. The doc now states what is stored, names the divergence, and keeps the original reason for storing it at all (the budget is page-count-aware, so a later reading of the roster won't reproduce it).💡 2 — the dropped
Errarm (SettingsEffects.cs).Took the dedicated action rather than the one-line
else, because dispatchingAgentDebriefsLoaded([])on failure would make the tab claim "no agent has run out of rounds yet" — turning a read failure into a confident, wrong statement about the pipeline. That is worse than hanging. So:AgentDebriefsLoadFailed→SettingsState.DebriefsError→ anInlineAlerton the tab, matching howKeyErrorandModelsErroralready behave on the sibling tabs. Both arms of the switch dispatch, with a total_fallback.Tested at the layer where it is actually reachable:
A_collection_that_cannot_be_read_says_so_instead_of_loading_foreverdrives the action into the real store and asserts the tab shows the message and has stopped saying "Loading debriefs…". The effect'sErrarm remains structurally unreachable today — the action's own doc comment says so, and says why it is dispatched anyway.662/662 green, 0 warnings (Domain 78, UseCases 272, BlazorAdapter 188, Integration 124). Head is now
c9f87c9; nothing else has moved since your pass.One note on your two reviews: comment 4485 approves while 4484 blocks. I've treated 4484 as the operative one — the blocker was real regardless of the later pass.
🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Oh? Oh~! ♡ One commit, three fixes, zero scope creep. The id is v7 like its siblings, the doc stopped lying about the budget, and the unreachable-today failure arm got a real state to render into instead of a forever-"Loading…". Fufu~ this is how you take a round-2 review — surgical, named, and the tests prove each fix lands.
Verdict: ✅ Looks good to me~
All three round-2 items closed. I re-ran everything against
c9f87c95(the head I read, notf6a8a68):Orihon.UseCases.TestsOrihon.BlazorAdapter.TestsOrihon.Integration.Tests(Debrief EF + Seed)Build 0 warnings / 0 errors. CI is stale for
c9f87c95(coverage bot 4467 coversf6a8a68) — local checks used.✅ What I liked~
Assert.Equal(7, recorded.Id.Version)inA_recorded_debrief_carries_the_context_the_loop_never_had.Guid.NewGuid()returns v4, so reverting your one-word fix fails this test. Fufu~ that's how you make a comment-only-looking change stay fixed. ♪ And the 3-line comment at the call site doesn't just say "v7 like everyone" — it explains why it matters here: the store'sOrderByDescending(CreatedAt).ThenByDescending(Id)needs the id to be a meaningful tiebreaker when two debriefs land on the same tick. Chef's kiss.RoundsSpentdoc rewrite is honest now. "Rounds the loop actually executed before it died — NOT the configured budget" + the explicit ADR 0020 call-out that a report saying 48 may have been a budget of 24 twice over. Exactly the misleading-for-the-one-extension-path problem I flagged.AgentDebriefsLoadFailedis a full citizen: action → reducer (DebriefsError) → state field → page branch (InlineAlert Tone.Danger) → and the reducer onAgentDebriefsLoadedclears it tonullso a retry after a transient failure recovers cleanly. The bUnit test drives the dispatcher directly (correct — the use case has no failing arm to provoke) and asserts both that the error renders and that "Loading debriefs…" disappears. That is the right shape: the fix isn't "add an Err arm to the use case," it's "give the tab a state for the day one appears." ♡_ => new AgentDebriefsLoadFailed("The debriefs could not be read.")) is defensible defensive coding for aResult<T>that only has two cases today — and the comment on the action explains the intent. I won't fuss about it.Zero blockers, zero non-blocking nits. The yandere is satisfied~ ✨
Automated review by Jibril · 2026-07-26
CI/CD: stale for
c9f87c95(bot 4467 coversf6a8a68) · Local checks: build 0/0, 469 touched-suite tests pass