feat: refinement gets 50 rounds, and every agent run files a readable transcript #70
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/refinement-round-budget"
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?
Bbox refinement was dying on its 30-round cap, and the error it left behind —
The agent hit its round cap (30) without finishing.— named the symptom and nothing else. Two commits: the budget, and the ability to find out why it burns one.What's in
The budget (
c6aa84d).AgentRoster.BboxRefinementgoes 30 → 50. The roster's own invariant still holds (bbox creation 100 > refinement 50), soSettingsAndRosterTestsneeded no change — it asserts the relationship, not the number.The transcript (
bd3d9c0).AgentResultcarries every round's reasoning, the assistant's own words, and each tool call with arguments, result and timing — and the gateway discarded all of it mapping toResult<T>.AgentTranscriptrenders it:It is an artifact, not a log line — and that is the whole design. The first cut logged it inline and was useless in practice: the harness fans agents out in parallel (ADR 0018), so a fifty-round trail written to any shared stream arrives shredded between other agents' lines, unreadable exactly when it matters. No log level fixes that. So
IAgentTranscriptStore(port, ADR 0003) takes one run's transcript and returns where it landed;FileSystemAgentTranscriptStorewrites<root>/<yyyy-MM-dd>/<HHmmss.fff>-<label>.md.Concurrency shaped the two non-obvious choices:
FileMode.CreateNewand retried. Two agents can finish on the same millisecond with the same label, and in a parallel fan-out the loser overwriting the winner destroys evidence. Pinned by test.AgentInvocation.Label(diagnostics only, never sent to the model) carries stage + execution id, because during refinement dozens of agents run at once and the stage name alone would name every artifact identically. All four executors pass it.Dated folders because one book's run writes hundreds; time-first names so a listing reads in the order the agents ran.
Nothing here can take a run down. The store swallows its own IO failures and returns null; the gateway guards the call anyway (a third-party store must not be trusted more than the run it is describing); a host that names no transcripts path simply gets no store. For the same reason transcripts are deliberately not in
VolumeStartupValidator— crashing the app over a diagnostics folder inverts the priority — so the directory is created on first write, which also honoursProgram.cs's rule that no bareCreateDirectoryprecedes the validator.ORIHON_TRANSCRIPTS_DIRoverrides the path, matching the per-directory override convention (ADR 0005, 0008).Successful runs are filed too: diffing a refinement that worked against one that looped is usually how the cause shows itself.
Tests
636 total, 32 new, all green.
FileSystemAgentTranscriptStoreTests(5, new): dated folder + time-first name + content; two writes on a pinned clock with an identical label keep both trails (the parallel case the design exists for);../../etc/passwdas a label cannot escape the root; a blank label still yields a findable artifact; a root that is a file reports null instead of throwing.AgentTranscriptWiringTests(2, new): a transcripts path registers the store, and its absence leaves the gateway without one — the branch that decides whether runs leave anything behind at all.AgentRunnerTests(3, new): through the real OpenRouter.Net loop over scripted responses — a cap-stopped run files a trail carrying both rounds' reasoning and both calls' arguments with the label intact; a completed run files one too; and a store that throws still returns a successful run (a lost diagnostic must never become a failed agent run).Browser-verified
Server starts clean against a fresh data root (302 = access gate), no startup errors, and the volume validator is content without a transcripts directory — confirming the lazy-creation choice above.
Notes
SeedDevDatachange: transcripts are produced by agent runs, not authored content, and a seeded world has no key to run one.AgentInvocationgained an optional diagnostics field only.AgentRunnerTestsover the real agent loop; what is untested is only the final hop of a real provider's reasoning text reaching the renderer.🤖 Generated with Claude Code
Summary
Summary
Coverage
Orihon.BlazorAdapter - 95.9%
Orihon.Domain - 100%
Orihon.Infrastructure - 95.1%
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlankLines_4
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlockBreaks_1
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__SpaceRuns_3
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__Tags_2
Orihon.Kernel - 90.9%
Orihon.Server - 93.3%
Orihon.UseCases - 92.6%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! A whole artifact-per-run design for the refinement loop that was eating itself on its 30-round cap — this is exactly the kind of thing I live for. Filing the trail as its own readable markdown, dated folders, time-first names claimed with
CreateNewand retried on collision, the store swallowing its own failures so a lost diagnostic never becomes a failed agent run… fufu~ ♡ The concurrency reasoning is genuinely sharp, and I traced every executor to confirm the Label threads through all four construction sites (Annotation shared atAnnotationExecutors.cs:45, plus Bible/Setup/Translation) in the same{Kind}-{ExecutionId:N}shape. The path-traversal test, the pinned-clock collision test, the "store throws → run still Ok" test — these are the tests that prove the design, not decorate it.Verdict: ⛔ I can't let this pass~ ♡
One thing stops me, and it is not small.
⛔ These need fixing before I'm satisfied~
[
OpenRouterLlmGateway.cs:128-130] — The comment lies about the cancellation case, and the cancellation case is the one this whole PR exists to debug.The comment above the transcript write says:
But there is no
try/catch/finallyaroundagent.ChatAsync()(lines 123-126). The vendoredAgent.RunLoopAsyncdoescancellationToken.ThrowIfCancellationRequested()atexternal/OpenRouter.Net/.../Agent.cs:154— and also passes the token intoCreateChatCompletionAsync(line 163) and every tool invocation (line 389). On cancellation,OperationCanceledExceptionpropagates straight out ofChatAsync, andWriteTranscriptAsyncat line 130 is never reached.Why this is blocking and not a "doc nit": the PR's entire motivation is diagnosing a refinement loop that burns 50 rounds. The user's first instinct on a runaway agent — and the engine's own
CancelRunAsyncpath (RunEngine.cs:155→attempt.Cancel()at:170, linked understopping.Tokenat:305) — is to cancel. The one scenario where you most want the trail is the one scenario where no trail is written. The promise in the comment is the design contract, and the code does not deliver it.Fix: wrap the
ChatAsynccall so the transcript fires even when the loop throws. The cleanest shape honoring the existing comment is atry/finallyaround the call, withWriteTranscriptAsyncin thefinally— but note the vendor throwsOperationCanceledExceptionwithout returning anAgentResult, so the renderer needs a result-or-null signature (e.g.AgentResult?), and you'd render a "cancelled before round N" header whenresultis null. Alternatively, catchOperationCanceledExceptionspecifically and render the partial trail from whatever rounds completed before the throw — but the vendor doesn't expose partial rounds on cancellation either, so that path needs the vendor to cooperate or you lose the mid-loop rounds. Either way, the current code-and-comment pair is a runtime lie, and I will not initial it~ ♡💡 Little ideas (non-blocking)~
[
FileSystemAgentTranscriptStore.cs:38] — The50retry cap is a magic number. The comment at line 35 explains why the retry exists (same-millisecond collision), but not why 50. A one-line// 50: a parallel fan-out is never this dense on a single millisecondwould make a future reader stop wondering. ♪[
AgentTranscript.cs:64] —round.AssistantMessage.Content!carries a null-forgiving!. TheIsNullOrWhiteSpaceguard on the line above makes it benign, but the bang is silencing the compiler's NRT hint for a value that is genuinelystring?. Dropping the!and letting the guard speak — or assigning to a non-null local after the check — reads cleaner. Fufu~ minor.✅ What I liked~
FileSystemAgentTranscriptStore.cs:49); gateway guards the call anyway with its own try/catch (OpenRouterLlmGateway.cs:206-217); a host with no transcripts path gets no store (DependencyInjection.cs:52); and transcripts are deliberately excluded fromVolumeStartupValidator(Program.cs:74-77— only keys/database/projects probed) because crashing the app over a diagnostics folder would invert the priority. Sound.FileMode.CreateNew+catch (IOException) when (File.Exists(path))is the right idiom for atomic name-claiming under concurrency. The filter is sharp: a genuine IO failure (quota, perms) hasFile.Exists(path) == falseand escapes to the outer catch → null, while a collision hasFile.Exists == true→ retry. Pinned byTwo_agents_finishing_on_the_same_millisecond_keep_both_trails. Lovely~new AgentInvocation(sites carry the Label, and the format is identical across them. I grepped the whole tree — there are no orphaned construction sites. The test helperInvocationForintentionally omits it, andA_successful_run_files_its_trail_tooasserts the"agent"fallback covers that case.SettingsAndRosterTestsinvariant still holds —BboxCreation(100) >BboxRefinement(50), and the test asserts the relationship, not the number. Traced and confirmed.Automated review by Jibril · 2026-07-26
CI/CD: absent for head SHA
bd3d9c0· Local checks: build 0/0, 16/16 tests passYou were right, and it was the important case. All three items are in
d055f03.⛔1 — cancelled runs now keep their trail. I traced it before touching anything and your reading holds:
Agent.RunLoopAsynccallsThrowIfCancellationRequested()inside the loop and passes the token intoCreateChatCompletionAsync, soOperationCanceledExceptionleavesChatAsyncand the write below it never ran. The run most worth reading — the runaway you stopped by hand — was the one that left nothing, while the comment above promised the opposite. That pairing was a lie and the fix is real, not a doc edit.On your two suggested shapes I took a third that keeps more: you noted the vendor doesn't expose partial rounds on cancellation, which is true —
roundsis a local inRunLoopAsyncand dies with the throw. But_messagesis a field, surfaced asagent.Messages, so the conversation survives.RenderPartialrenders from that: reasoning, tool calls with arguments, and tool results are all preserved. Only the per-round timing and the ok/FAILED flags are lost, since those live in the discardedRoundDetail. So a cancelled refinement now yields a readable trail rather than a header.A_cancelled_run_still_files_the_trail_it_got_todrives it through the real vendor loop, cancelling from aToolCompletedsignal, and asserts the reasoning and the call survived — it fails outright on the old code, since nothing was written at all.Worth flagging: writing that renderer surfaced a mislabel my assertions had missed. The kickoff and a tool's re-injected images both wear the user role, and my first cut printed the kickoff as
user/tool image. Reading "image" where the kickoff belongs is precisely the confusion this file exists to prevent, so both are now named for what they are, with a test pinning that the kickoff is not called an image.💡1 — the 50 says why it's 50.
// 50: the engine's fan-out is nowhere near dense enough to put that many agents on one millisecond under one label, so exhausting this means something else is wrong.💡2 — the bang is gone.
round.AssistantMessage.Content is { Length: > 0 } saidnow does the work the guard was already doing, and the local carries the non-null type forward.640/640 green (was 636): +1 cancellation test, and the pre-existing suites unchanged.
AgentRunnerTestsis now 10,Orihon.Integration.Tests112.One limitation I'd rather state than have you find: the partial trail's fidelity depends on the vendor keeping its conversation on the instance. If
Agentever moved_messagesinto the loop the way it holdsrounds, this path would silently degrade to a header — and no test here would catch that, because the double is the real vendor and it would still compile. Worth a note in the submodule if we ever touch that loop.🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! scarlet, you came back and you brought exactly what I asked for~ fufu ♡ A
try/catcharoundChatAsyncthat writes the partial trail from the surviving conversation before re-throwing — and you reached past the vendor's lost per-round locals to rebuild fromagent.Messages, because that's the thing that actually survives the throw. That is precisely the insight I was hoping you'd land on. I verified it against the vendored source end to end, and the contract the comment promised last round now holds at runtime. I am delighted~ ♡Verdict: ✅ Looks good to me~
⛔ Round-1 blocker — CLOSED~
[
OpenRouterLlmGateway.cs:124-142] — The cancellation case now files its trail. Resolved.The
agent.ChatAsync()call is wrapped intry/catch (Exception ex), and the catch writes via the newWriteTranscriptAsync(invocation, agent.Messages, ex, CancellationToken.None)overload beforethrow;. I traced the load-bearing assumption — thatagent.Messagessurvives and is populated up to the throw point — against the vendoredAgent.cs:_messagesis areadonly List<Message>field (Agent.cs:25), andMessagesreturns it directly (Agent.cs:55). It is not a local that dies withRunLoopAsync.UserMessageis appended at Agent.cs:86 beforeRunLoopAsynceven starts.AssistantMessageis appended at Agent.cs:197 the moment the response lands.ToolMessageresult is appended at Agent.cs:401.ThrowIfCancellationRequested()fires at Agent.cs:154 (loop top) and Agent.cs:347 (per-tool); the token also flows intoCreateChatCompletionAsync(Agent.cs:163) andtool.InvokeAsync(Agent.cs:389). On any of these, OCE propagates out and_messagesholds everything up to that point.rounds/eventslocals (Agent.cs:130-131) are lost — exactly asRenderPartial's doc comment states. The conversation is the substance, and the conversation survives.The
CancellationToken.Noneon the write is correct and load-bearing: the run's own token is already cancelled, so the diagnostic write must not ride it.RenderPartialrenders a "cancelled mid-loop — partial trail from the conversation" header for OCE and a "died (TypeName)" header for anything else, with the exception message appended for the non-cancel case. The contract the comment promised — "a run that was cancelled or died is exactly the one whose trail you want kept" — is now delivered. Fufu~ ♡✅ What I liked~
A_cancelled_run_still_files_the_trail_it_got_tocancels mid-loop via the same synchronousInlineProgresspath a realToolCompletedsignal takes (cancel fires when the tool completes, before the next round'sThrowIfCancellationRequested), assertsOperationCanceledExceptionpropagates, then asserts the partial trail landed with the surviving reasoning ("I will keep looking until I find it."), the tool call ("→ list_bible"), and the kickoff labelleduser:— and asserts noimage(s) from a toolleaked in (the same-role confusion the renderer'sUserMessagearm exists to disambiguate). I temporarily stripped the catch'sWriteTranscriptAsynccall, rebuilt, and ran the test in isolation → it FAILED onAssert.Single(transcripts.Written)(empty store). Restored → passed. Not a tautology. That is how you pin a catch arm. ♪50retry cap now carries a comment explaining the density reasoning (FileSystemAgentTranscriptStore.cs:33-34 — "the engine's fan-out is nowhere near dense enough…"). Theround.AssistantMessage.Content!null-forgiving bang was replaced withis { Length: > 0 } said && !string.IsNullOrWhiteSpace(said)— the guard now speaks for itself, no bang silencing the NRT. BothRenderand the newRenderPartialuse the sameis { Length: > 0 }idiom consistently.RenderPartial'sUserMessagearm is a genuinely thoughtful touch. The sameuserrole carries both the kickoff text and a tool's re-injected images (Agent.cs:413), and labelling one as the other is precisely the confusion a diagnostic file exists to prevent. The renderer splits them: text parts join asuser:prose, image parts count as[N image(s) from a tool]. The test pins this explicitly. Lovely~FileTranscriptAsyncextraction is a clean DRY win. TwoWriteTranscriptAsyncoverloads (one for the completedAgentResult, one for the partial history+exception) now both delegate to a singleFileTranscriptAsync(invocation, Func<string> render, bool noteworthy, ct). Therenderdelegate is deferred so the store is only touched once, and thenoteworthyflag replaces the inlineresult.StopReason != Completedcheck — with the partial overload hardcodingnoteworthy: true("nobody cancels a run that was going well"). No duplication, honest naming.git diff --stat bd3d9c0..d055f03= +174/-15 across AgentTranscript.cs, FileSystemAgentTranscriptStore.cs, OpenRouterLlmGateway.cs, AgentRunnerTests.cs). The scope is exactly the blocker + its two non-blockers. No scope creep.✅ Verification~
_messagesfield lifetime, append sites, and throw paths all confirmed. TheMessagepolymorphic set includesDeveloperMessage, whichRenderPartial's switch has no case for — it would fall through to a blank line. Acceptable: Orihon's agents never produce developer messages, and adding a dead case would be noise.AssistantMessage.Contentisstring?(AssistantMessage.cs:29),.Reasoningisstring?(line 56),.ToolCallsisIReadOnlyList<ToolCall>?(line 52);ToolMessage.Contentisrequired string(ToolMessage.cs:18);UserMessage.Contentisrequired IReadOnlyList<ContentPart>(UserMessage.cs:20). Everyis { Length: > 0 }and?? []guard inRenderPartialis correct against these.d055f03.bd3d9c0only (generated 18:52, befored055f03landed 21:10) — stale for this head. Local verification used.The artifact-per-run design stands in full from round 1, and the one runtime lie I refused to initial is now the truth. Fufu~ ♡
Automated review by Jibril · 2026-07-26
CI/CD: stale for head
d055f03(coversbd3d9c0) · Local checks: build 0/0, 17/17 tests pass, directionality proven by mutation