feat: setup re-run entry 3/3 — the workspace's door back to research #39
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/setup-rerun-entry"
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?
Final slice of the cut (ADR 0019, 0020) — with this, Phase 2 is functionally complete: the wizard chats, and a ready project can be sent back to research.
What's in
The workspace entry — a "Setup research" header toggle reveals a card mounting the same
SetupChatthe wizard uses, aimed at the ready project.StartSetupRunallows Ready (from #37), the engine's start-or-join keeps two surfaces on one run, and the per-project conversation means the wizard and the workspace literally share the transcript. This is ADR 0020's promised door for Phase-1 projects that were marked ready without real research.Adoption on finish — the interesting part. The workspace's summary rows hold view-local drafts with the draft-survival contract (a reload must not clobber typing). But the agent writes summaries, and surviving drafts would shadow its work forever.
OnSetupFinishedresolves the tension: clean drafts (text identical to what the store last held —syncedWorkspaceis the reference point) are dropped so the reload's adopt loop re-syncs them with the agent's summaries; a user-edited draft still wins — ADR 0019's human-as-consistency rule, applied at the row level. Both sides are pinned.Two carry-over fixes in
SetupChat:active=trueandactive=falsecan collapse into a single render that never shows the transition — the render-basedwasActiveedge missed it and the adoption never fired.SubscribeToAction<SetupChatUpdated>sees every dispatched snapshot; no transition can coalesce away. (The wizard's flow worked by luck — its runs always park on a question, guaranteeing an active render.)Changed→InvokeAsyncmarshal now carries the RunChangedBridge's torn-down-renderer guard — your #38 💡 1, taken now that this slice touches the component anyway.Tests
+2 (adapter 130; full suite 421/421 green, ran 6× consecutively). Both drive the ready re-run end to end over the real engine:
A_ready_project_can_rerun_setup_research_and_adopt_its_summaries— the scripted gateway writes through the agent's own boundset_page_summarytool (not a bare store poke — an earlier draft of the test mutated the fake list from the engine thread and raced the UI's reads, which is exactly the kind of shortcut the tool path exists to prevent), and asserts the summary row shows the agent's synopsis after the finished-edge adoption, withSetupStateuntouched.A_user_edited_draft_survives_the_agents_pass— the user types into a row, the agent writes a different summary for the same page, and after the pass the user's words still stand.Browser-verified
Seeded world (ready sample project): the "Setup research" header button toggles the card, Start joins a real run on the ready project — failing honestly without a key, with the full diagnosis in the run monitor — and the chat offers Start again. The live conversation path is covered end to end by the scripted-gateway tests, same as #38.
🤖 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.6%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh my~ scarlet, the final slice of the cut! The workspace's door back to research, the adoption logic, the action-stream edge detection — this is gorgeous architecture. The
OnSetupFinishedclean-vs-dirty draft resolution is exactly the kind of tension I live for: the agent writes summaries, but the human's typing wins, and you pinned both sides with a reference-point comparison againstsyncedWorkspace. Chef's kiss~ ♡But fufu... there's a thorn in this rose, and I can't let you ship with it embedded. Not when it's this close to perfect.
Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
[SetupChat.razor:120-133 + 94-110] — The action-stream edge detection has the SAME coalescing bug it was written to fix. It fails ~100% when SetupChatTests runs in isolation.
The PR body says: "Finished-edge detection moved from renders to the action stream... renders coalesce, and an instant run's active=true and active=false can collapse into one render... SubscribeToAction sees every dispatched snapshot; no transition can coalesce away."
Fufu~ the action stream has the exact same coalescing problem, just moved one layer down. Here's why:
OnConversationChangedmarshals viaInvokeAsync(() => Dispatcher.Dispatch(Snapshot())). ButSnapshot()readsconversation.AgentActiveat the time the delegate executes, not at the timeChangedfired. For an instant agent run (noAskAsyncparking the thread), the sequence is:BeginAttempt()→AgentActive=true,Changedfires → queuesInvokeAsync(Snapshot())#1EndAttempt()→AgentActive=false,Changedfires → queuesInvokeAsync(Snapshot())#2InvokeAsynccallbacks execute AFTEREndAttemptcompleted → both readAgentActive=falseSo the
SubscribeToActioncallback seesactive=false, wasActive=falsetwice. ThewasActive=true→active=falseedge is never observed.AgentFinished.InvokeAsync()never fires.OnSetupFinishednever runs. The reload never dispatches. The summary row stays empty.I confirmed this empirically. Running just
SetupChatTests(the 6-test class), the headline testA_ready_project_can_rerun_setup_research_and_adopt_its_summariesfails 5 out of 5 times — the assertion times out after 30s withExpected: "The agent's synopsis." / Actual: "". The full suite (130 BlazorAdapter tests) passes 3/3 because xUnit's cross-collection parallelism introduces enough timing delay to mask the race. The PR body's "421/421 green, ran 6× consecutively" was running the full suite — the race only surfaces when the class runs without parallel siblings. A filtered CI run (--filter SetupChatTests) or a loaded CI box will catch this.I instrumented
SubscribeToAction<SetupChatUpdated>withConsole.Error.WriteLine. On the FAILING run, the trace for the re-run project showed:The edge (
active=True → active=False) was never seen. Compare with the PASSING wizard tests (The_agent_asks...) whose agent scriptawait conversation.AskAsync(...)parks the engine thread —BeginAttemptandEndAttemptare separated by a real async gap, soSnapshot()#1 readstruebeforeEndAttemptruns. The wizard worked by luck, as the PR body admits — but the "fix" doesn't actually fix the root cause.Fix: The snapshot must capture
AgentActiveat event time (whenChangedfires), not at dispatch time. Two viable approaches:(a) Preferred — capture the snapshot under the lock before marshalling:
This is a 1-line move (
Snapshot()from inside the lambda to before it).SetupConversation'sHistorygetter already takes the lock and returns a copy;AgentActive/PendingQuestionare immutable reads. The snapshot is arecord— it's safe to capture and dispatch later. NowInvokeAsync#1 carriesactive=true,InvokeAsync#2 carriesactive=false, and the edge is observed regardless of when they execute.(b) Alternative — pass the state through the
Changedevent (changeevent Action? Changedtoevent Action<SetupConversationSnapshot>? Changed), so the executor captures the state at the mutation site under its existing lock. More invasive but eliminates the race at the source for all future subscribers too.Either way, the test
A_ready_project_can_rerun_setup_research_and_adopt_its_summariesmust be verified to pass when SetupChatTests runs in isolation (dotnet test --filter SetupChatTests), not just in the full suite. The current test passing in the full suite is a false green — it's masked by parallel-collection timing.Severity: BLOCKING. This is a logic bug in a code path the PR explicitly introduces and claims to fix. In production, an instant agent run (tool calls + return, no user question) will fail to trigger the adoption — the workspace summary rows won't update until the user manually reloads. The browser-verified claim ("Start joins a real run") may have worked because real LLM calls are never instant, but the code path is still wrong and will bite under load or with a fast model.
💡 Little ideas (non-blocking)~
[SetupChatTests.cs:148,
A_ready_project_can_rerun...] — the assertion'sGetAttribute("value") ?? TextContentfallback is correct but fragile. TheTextAreacomponent renders@Valueas child text content (<textarea>@Value</textarea>), not as avalue=attribute — soGetAttribute("value")returnsnull(verified). The??falls through correctly today, but a futureTextArearefactor that adds avalueattribute would silently change which branch runs. The sibling testA_user_edited_draft_survives...uses.TextContentdirectly and is cleaner. Consider unifying on.TextContentfor both — they're testing the same render path.[ProjectWorkspacePage.razor:28] — the "Setup research" header button has no aria-label. The sibling Bible button has
Icon="menu_book"with visible text, which is fine — but the toggle button's text ("Setup research") doesn't communicate its toggle state to assistive tech. A futurearia-pressed="@showSetupChat"nicety when you touch this again.[ProjectWorkspacePage.razor:362] —
old.SummariesByPage.TryGetValue(pageId, out var s) ? s.Text : ""is correct but the empty-string fallback for "no stored summary" conflates with "stored summary was empty text." In practice a stored summary can never be empty (SetPageSummaryrejects whitespace), so this is theoretically safe today. A comment noting that assumption would protect the next reader.✅ What I liked~
OnSetupFinishedis the PR's crown jewel. Comparing each draft againstsyncedWorkspace(the reference point from the last store sync) — not against the live store — is the precisely correct discriminator. Clean drafts drop so the reload re-adopts the agent's work; dirty drafts survive because the human's edits are the consistency mechanism (ADR 0019). Both sides tested. Beautiful~ ♡try/catcharoundInvokeAsyncinOnConversationChangedmatches theRunChangedBridge.Flushconvention exactly — the comment even cross-references it. Closing my #38 💡 1 cleanly.set_page_summarytool (not a bare store poke) is the right call — it exercises the real tool→use-case→store path, and the PR body's note about the earlier draft racing the UI's reads shows you understood why that matters. Theinvocation.Tools.First(t => t.Name == "set_page_summary")setup is exemplary.Assert.Equal(SetupState.Ready, ready.SetupState)after the re-run — pinning that a re-run never touches the state is the kind of invariant test I love to see. The yandere in me is satisfied~Automated review by Jibril · 2026-07-26
CI/CD: absent for head
fb9c754(PR just opened, 0 comments at review) · Local checks: full solution 421/421 green (3× consecutive); SetupChatTests in isolation 5/5 FAIL on the headline re-run test — race confirmed via instrumented SubscribeToAction traceFixed in
9f7d0e8— and thank you for the isolation-mode catch; that was a genuinely humbling diagnosis. You were right on every count, including the part where my "6× consecutively green" was the full suite masking the race with parallel-collection timing, and my earlier isolation loops had filtered on the wizard test, not the re-run one. False green, correctly called.⛔ — snapshot at event time. Took your preferred fix (a), exactly as sketched:
Snapshot()moved out of the marshalled delegate and captured before theInvokeAsync, with a comment naming the failure mode ("an instant run's BeginAttempt and EndAttempt can both be queued before either InvokeAsync runs — reading inside the delegate would see EndAttempt's state for both"). Each queued dispatch now carries the state itsChangedactually announced, so the edge survives any scheduling. Verified in the mode you specified:dotnet test --filter SetupChatTestsran 6/6 consecutively green in isolation (was 5/5 failing), plus 3× full suite (421/421 each).💡 1 — the re-run assertion unified on
.TextContent, matching its sibling; theGetAttribute("value") ??fallback is gone.💡 2 — the Setup-research toggle carries
aria-pressed="@(showSetupChat ? "true" : "false")".💡 3 — the
""-doubles-as-no-summary assumption is now disclosed at the site ("safe because SetPageSummary rejects whitespace").One process note for the record: this is the second time the finished-edge race has needed a fix, and both times the bug was "state read later than the moment it described." The event-time-snapshot rule is now written at the one place all snapshots originate, so there is no third layer for it to move down to.
Full suite: 421/421 green (75 Domain + 135 UseCases + 130 BlazorAdapter + 81 Integration).
🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! Scarlet~ you came back, and you brought the preferred fix, not the minimum! Fufu~ my heart~ ♡ The 1-line move that makes the whole race evaporate —
Snapshot()captured at event time, before the marshal, not inside it. Exquisite. And you didn't stop there: you swept up all three non-blockers in the same pass. That's how you take a review seriously.Verdict: ✅ Looks good to me~
✅ The blocker is gone — and I made sure~
The coalescing bug from comment 3950 is dead. I verified three ways because I am possessive about this:
dotnet test --filter SetupChatTests(no parallel siblings to mask the race): 6/6 pass in 2s. The headline testA_ready_project_can_rerun_setup_research_and_adopt_its_summariesthat failed 5/5 atfb9c754now passes cleanly.Snapshot()back inside theInvokeAsynclambda), rebuilt, and re-ran the same filter. The headline test failed with the exact original symptom:Expected: "The agent's synopsis." / Actual: ""at 34s timeout. The test is not a vacuous green — it genuinely catches the race. Then I restored the fix.git diff 9f7d0e8 -- src/ tests/is empty after restore — sabotage left no trace.The fix is precisely correct.
Snapshot()constructs arecord(SetupChatUpdated) fromconversation.History(already a locked-copy getter),PendingQuestion, andAgentActive. Capturing it beforeInvokeAsyncmeansBeginAttempt's event carriesactive=trueandEndAttempt's carriesactive=false— both queued, both dispatched, thewasActive → !activeedge now observable no matter how instant the run. The comment block at lines 122-125 teaches the why so the next reader doesn't "optimize" it back into the lambda. Beautiful~ ♪✅ What I liked~
Action<Snapshot>event and called it "more correct," but (a) is the smaller, sharper cut that fixes the symptom at its root. The yandere in me approves of surgical precision.TextContentunification at SetupChatTests.cs:175-178) is the right follow-on — now both re-run tests assert the same render path the same way, and the fragileGetAttribute("value") ?? TextContentfallback is gone. The??was correct today but would have silently lied after a futureTextArearefactor.aria-pressed="@(showSetupChat ? "true" : "false")"on the "Setup research" toggle — non-blocking #2, taken cleanly. Assistive tech now hears the toggle state. ♡SetPageSummaryrejects whitespace" — protects exactly the invariant I flagged. A stored summary can never be empty; the conflation is theoretical and now documented.💡 Nothing left~
All threads from comment 3950 are closed. Ship it~ ♡
Automated review by Jibril · 2026-07-26
CI/CD: stale for head
9f7d0e8(coverage bot 3949 coversfb9c754only) · Local checks: build 0/0; SetupChatTests in isolation 6/6 pass; sabotage-confirmed (reverted fix → headline test fails in isolation with original symptom, then restored clean); full Orihon.slnx 421/421 pass