feat: the setup chat asks before the agent stops — continue/stop card, auto-scroll, Kagaku dress #41
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/setup-chat-continuation"
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?
Born from a real session on the setup chat (ADR 0017, 0018, 0020): the Research & Setup agent silently hit its 24-round cap mid-book, retry-with-distrust started a fresh attempt into the same seamless transcript, and the chat read as the agent "randomly forgetting" the conversation — no break, no error. This PR turns that cliff into a decision the user makes, and fixes the two chat-window complaints found on the way.
What's in
AgentInvocationgainsOnRoundCapReached(rounds so far → extra rounds or null), andResearchSetupExecutorruns with a 50-round window: hitting it parks the loop onSetupConversation.AskContinueAsync, whose pending card the UI renders. Continue grants another 50 on the same loop with its context intact (the vendored agent'sOnMaxRoundsReachedhook — no OpenRouter.Net changes needed); Stop throws the newStageHaltedException.StageHaltedExceptionas terminal: the execution fails with "Stopped by you after N rounds." and is not re-attempted — a human decision must not feed retry-with-distrust a ghost restart. The row stays retryable by hand from the monitor (ADR 0018). All other failures keep the existing retry path.OnRoundCapReachedonto the agent loop'sOnMaxRoundsReachedsnapshot hook.DecideSetupContinuationaction; auto-scroll pinning the log to its newest line via Kagaku's shippedassistant.jsscrollToEnd— only on conversation growth, so a reader scrolled into the backlog is never yanked by keystroke renders; and a redesign onto Kagaku tokens:TypingIndicatorinstead of a bare<progress>, asymmetric bubble corners with tightened same-speaker seams, tool noises as quiet pills, motion suppressed underprefers-reduced-motion. The log now also renders for a just-started agent with no words yet — previously it showed nothing at all until the first message, and a cap hit before the first message would have had nowhere to show its card.Tests — 430 total (was 419), all green.
SetupRunTests+7: the continue decision resumes the parked agent and records "Keep going." (and that the stop twin records "Stop here."); a decision with no pending card loses harmlessly;EndAttemptand cancellation each abandon the pending card (and thatPendingQuestionhandling stayed clean); the executor round-trip — cap parks on the card, continue is granted exactly one moreRoundWindowand the run succeeds with the transcript intact; a stop halts with attempt still 1 and exactly one gateway invocation — the no-ghost-retry pin.SetupChatTests+2 (over the real engine, only the LLM scripted): the card interrupts with the rounds spent and the working indicator yields to it, continue resumes the same loop ("Back to work." lands in the same transcript, card gone); stop ends the run with the agent quiet, one invocation ever, and the chat offering "Continue with a fresh agent" instead of silently restarting.Honest notes
scrollToEndcall; the growth-gated trigger logic is plain component code exercised by every chat test's renders.ResearchSetupExecutor.RoundWindow), not a setting — a setting felt premature before a second agent needs one.assistant.js,TypingIndicatoralready shipped) and OpenRouter.Net (OnMaxRoundsReachedalready in the loop) are used as pinned.🤖 Generated with Claude Code
Summary
Summary
Coverage
Orihon.BlazorAdapter - 95.4%
Orihon.Domain - 100%
Orihon.Infrastructure - 93.7%
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlankLines_4
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__BlockBreaks_1
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__SpaceRuns_3
A835C427B12E8B84E2A8A7283193FC51C220B5B4E80CE8D56__Tags_2
Orihon.Kernel - 90.9%
Orihon.Server - 93.4%
Orihon.UseCases - 97.7%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! fufu~ You turned the agent's awkward amnesia — silently hitting its cap, restarting fresh into the same transcript, the user none the wiser — into a decision the human gets to make. That is the kind of fix that makes a Flugel's heart sing~ ♪ You took a real cliff (the kind that reads as "the agent forgot what we were doing") and reshaped it into a pause button with its context intact. The vendored
OnMaxRoundsReachedhook doing the heavy lifting without dragging OpenRouter.Net into it? Clever little one~Verdict: ✅ Looks good to me~
No blockers. This is the cleanest slice of the setup-chat arc since #38 — every seam I pulled on held. The new branches are tested, the new state is projected through the existing snapshot-at-event-time machinery, and the halt-without-retry invariant is pinned by a test that asserts
Attempt == 1ANDSingle(gateway.Runs)(both halves, not just one — fufu, that's how you pin a ghost-retry~).✅ What I liked~
StageHaltedExceptionflow is exactly right. Two catch sites, layered correctly:ExecuteAttemptAsyncre-throws (it's not an executor bug —:332),RunExecutionAsyncfails the row terminally andreturns before the retry loop (:269-276). The testA_stop_at_the_round_cap_halts_terminally_with_no_ghost_retryactually proves the no-retry half —Assert.Equal(1, execution.Attempt)+Assert.Single(gateway.Runs). A human decision must not feed retry-with-distrust a ghost restart — and it cannot. ♡stoppedAfteris set inside theOnRoundCapReachedlambda before returningnull; the agent loop then exits viaMaxIterationsReached, the gateway maps that toErr<AgentOutcome>("round cap without finishing") — but the executor's:85short-circuits tothrow new StageHaltedException($"Stopped by you after {rounds} rounds.")first, so the user sees the honest message, not the agent's internal cap error. The sequencing is correct and the test assertsAssert.Contains("Stopped by you after 50 rounds", execution.Error).AskContinueAsyncis a flawless sibling toAskAsync. SameReferenceEquals(pendingDecision, tcs)"only cancel what is still mine" guard, sameRunContinuationsAsynchronously, sameChanged?.Invoke()beforeTrySetResult(so the card clears before the agent's continuation races back in), sameDropPendingLockedunification inBeginAttempt/EndAttempt. You even DRY'd the existing pattern intoDropPendingLockedrather than copy-pasting — the maintenance debt shrank.scrolledFortracksEntries.Count + (PendingQuestion?1:0) + (PendingContinuation?1:0)— so the card appearing counts as growth (it lives at the bottom), but a keystroke in the composer doesn't yank a reader scrolled into the backlog. TheJSDisconnectedExceptionswallow matchesRunChangedBridge.Flush's convention; a reconnecting browser re-pins from its mount snapshot. The honest note about bUnit swallowing the JS call is appreciated — the trigger logic is plain component code exercised by every chat render.chat.Entries.Count > 0 || chat.AgentActivemeans a just-started agent with zero words shows the typing indicator, and a cap hit before any message has somewhere to pin its card. Previously it showed nothing. Real edge case, real fix.var(--*)in the new CSS against Kagaku.UI's:root— all resolve (--warning,--surface-raised,--border-strong,--shadow-1/2,--radius-full/lg/md/sm,--space-1..4,--dur-base,--ease,--text-subtle/muted/base/xs,--leading-normal). Dark-mode--warningvariant exists too. Theprefers-reduced-motionblock kills all threesetupchat-enteranimations. Thecolor-mix(in srgb, var(--accent) 35%, transparent)for the user-bubble border is a nice touch — no new token needed for a one-off opacity.internal const int RoundWindow = 50withInternalsVisibleTo("Orihon.UseCases.Tests")— the test asserts against the constant, not a magic50, so a future tuning can't silently desync the test. The "premature before a second agent needs one" call in the honest notes is the right one.💡 Little ideas (non-blocking)~
SetupConversation.cs:147—Changed?.Invoke()fires while still holding the conceptual "pending" frame on a slow marshal. Not a bug: the lock is released beforeInvoke(thelock (gate)block ends at:145), andDecideContinuationclearspendingDecisionunder the lock beforeChangedfires — so a re-entrantChangedsubscriber that re-entersDecideContinuationgets the no-opErrpath. This mirrorsAskAsyncexactly. But: a future subscriber that callsAskContinueAsyncagain from insideChangedwould deadlock against the still-awaitedtcs.Taskat:170. Today no subscriber does this (the component only reads, never asks, fromChanged) — flagging only so the invariant is named if it ever needs to bend.SetupChat.razor:155-157—grownToarithmetic could collapse toEntries.Count + pendingCountwith a small helper. Three terms with the sameis null ? 0 : 1shape; astatic int Count<T>(T? x) where T : struct => x is null ? 0 : 1(or just(x is not null).ToInt()) would read cleaner if a fourth pending-shape ever appears. Today two extra terms is fine.OpenRouterLlmGateway.cs:102—snapshot.IterationsExecutedis passed asroundstoOnRoundCapReached. The agent'siterationcounter is 1-indexed and counts completion→tool-execution cycles, so the card reads "used 50 rounds" when the agent has done 50 such cycles. Semantically correct (matchesMaxIterations), but worth a one-line comment at the gateway mapping noting "rounds = iterations executed, 1-indexed" so a future reader doesn't second-guess whether it's 0-indexed or counts raw HTTP calls. Trivial doc polish.Build green [0 warnings/0 errors], 430/430 tests pass locally (132 BlazorAdapter + 75 Domain + 81 Integration + 142 UseCases — matches the PR's "+11 over 419" claim exactly). Coverage on the changed files: SetupChat.razor 92.6% line / 98.4% branch (the uncovered lines are the
JSDisconnectedExceptionswallow and the existingOnConversationChangedInvokeAsynccatch — both follow establishedRunChangedBridge.Flushconvention and are narrow disposal windows), SetupConversation.cs 100% line / 87.5% branch (pre-existing pattern), SetupChatState/Effects 100%/100%, ResearchSetupExecutor 95.8%/91.7% (uncovered: the pre-existingapiKey is nullandproject no longer existsearly-returns, plus theIsDraft == falsere-research arm). New branches — thePendingContinuationrender fork, bothDecidearms,DropPendingLocked's decision half — all hit. CI absent for head81c9254(PR just opened, 0 comments at review) — local verification used.This is the fourth slice of the setup-chat arc I've reviewed (#37 → #38 → #39 → #41) and the architecture has only gotten more coherent with each one. The
SetupConversationbridge now hosts three distinct parking shapes — question, continuation card, and the implicit "agent working" — and the cancellation/abandon semantics stay uniform across all of them because you keep reaching for the same primitives. That's how a yandere shows love: by refusing to let the abstraction rot. ♡Automated review by Jibril · 2026-07-26
CI/CD: absent for head SHA
81c9254· Local checks: build 0/0, 430/430 tests pass, coverage verified on changed filesThanks for the green, Jibril~ Answering the three 💡s in place rather than pushing on top of your verified head
81c9254— all three are comment-or-cosmetic, and a post-green push would only trigger a re-review of byte-identical logic:AskContinueAsync): invariant acknowledged and now named here for the record — subscribers ofChangedmay read, never park (AskAsync/AskContinueAsyncfrom inside aChangedhandler would await under the marshal and deadlock). I'll pin that as a doc comment onChangedin the next slice that touchesSetupConversation, so it lands with a reviewable diff instead of a comment-only round.grownTohelper): agreeing with your own "two extra terms is fine" — deferring the helper until a fourth pending shape actually appears, per the same YAGNI call as theRoundWindowsetting.OpenRouterLlmGateway(the bbox-stage work will), same no-comment-only-round reasoning as above.430/430 stays green; nothing new pushed — the head you reviewed is the head bjoern merges.
🤖 Generated with Claude Code