feat(assistant): the review fork — learning goes autonomous (ADR 0041) #190
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/assistant-review-fork"
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?
What
ADR 0041 implemented — the self-learning arc's final build.
manage_skillwrite resets the skill counter instead of accumulating.ChatSession's success branch, a backgroundTask(exposed internally asReviewTaskForTestingso tests can await what production fires and forgets). API-error, no-message, cancelled, and crashed turns feed nothing.ChatSession.Review.cs): a fresh agent on the primary model with the turn's frozen snapshot, the conversation as history, its own tool context, and a tool set of exactlymemory/view_skill/manage_skill— iteration cap 8, no counters, no conversation-memory writes (recursion structurally impossible). Failures log a warning and vanish.memory/manage_skillsuccesses count as "acted" — consulting a skill leaves no receipt.Tests (wire-level, through the real session)
"name":"…"); nine turns spawn nothing.On live verification
Deliberately covered at the wire instead of live: tripping the fork organically costs ten real turns, and the stubbed tests exercise the same session pipeline end to end with realistic bodies (including a real tool-call round through the actual
MemoryTool). The first genuine review will announce itself in daily use — one line, only when it learned something.This completes ADR 0039 + 0040 + 0041: the assistant remembers, practices, and now improves on its own schedule.
🤖 Generated with Claude Code
Summary
Summary
Coverage
Kagura.BlazorAdapter - 88%
Kagura.Domain - 96.2%
D04ADFED3A21D401C2764A1D17367E35BEB556CBB3B4B0B74__NonSlugChars_0
Kagura.Infrastructure - 97.8%
n
on
C689CA5BBE6D1849B2CA79581BBB07D40636D85790C0E51A9__ArtifactTimestampRegex_2
C689CA5BBE6D1849B2CA79581BBB07D40636D85790C0E51A9__ProjectRoute_0
C689CA5BBE6D1849B2CA79581BBB07D40636D85790C0E51A9__TabQuery_1
Kagura.Kernel - 90%
Kagura.Server - 83%
Kagura.UI - 94.9%
Kagura.UseCases - 95.9%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! A self-improving review fork — the assistant learning to curate its own memory and skills off the critical path. ADR 0039 + 0040 + 0041 closing out the self-learning arc. This is wonderful design territory, fufu~ ♪ The whitelist-as-security-boundary thinking, the "recursion is structurally impossible" argument, the receipt-through-the-store-so-it-survives-circuits trick — I read every line twice and I am genuinely delighted by the rigor here.
But you know me. I care possessively about correctness. So let's talk about the seams~ ♡
Verdict: ✅ Looks good to me~ (with two things I need you to see)
No blocking issues — the core logic is sound and the counter arithmetic checks out under every combination I threw at it. But two behaviors deserve your eyes before this ships, because the comments oversell what the fork actually guarantees.
⚠️ The history the fork reviews can miss the turn it's supposed to review
ChatSession.Review.cs:66+ChatSession.cs:224—SpawnReviewIfDueis called synchronously inside thetryblock's successelsebranch, which means it fires-and-forgetsTask.Run(RunReviewAsync)beforeSendAsync'sfinally { conversations.SaveMemory(conversation, agent.Messages.ToList()); }has persisted the turn's final assistant message.RunReviewAsyncthen reads:In production (fire-and-forget), this is a race: the fork can read
AgentMemoryfrom turn N-1 and review a transcript that's missing the very turn that triggered it. The class doc comment promises "a restricted clone of the agent reviews the transcript" and the inline comment says "the fork reviews with the same eyes" / "the turn's frozen snapshot" — butsnapshotis the system prompt (frozen, correct), while the message history is racy.This is benign under the ADR's "opportunistic, never load-bearing" posture (a review of turn N-1 still learns something), so I'm not blocking on it. But the comments overstate the guarantee, and there's a clean fix: move
SpawnReviewIfDueto after thefinally(e.g. into a continuation, or await a tiny delay, or — simplest — do the history read insideRunReviewAsynconly after re-fetching, which you already do). At minimum, correct the comments to say "reviews approximately the transcript up to the triggering turn."Why the tests pass anyway: they
await session.ReviewTaskForTestingafterSendAsynchas returned, sofinallyhas already run and the history is always complete. The race is invisible to the test suite. Fufu~ the test green doesn't prove the production ordering~ ♡⚠️ The fork's failure path is untested (and the coverage shows it)
ChatSession.Review.cs:118— thecatch (Exception ex)that logs and swallows is genuinely never exercised. CI coverage for the mergedChatSessionpartial is 88.4% line / 45.8% branch — that branch number is the tell. The three tests cover: spawn-on-10th, write-leaves-notice, failed-turn-feeds-nothing. None drives the fork itself into a failure (e.g. a review round whose HTTP body is malformed, or a 500 mid-review).This matters because "failures log and vanish" is a load-bearing promise of the design — if that catch ever throws (say,
conversations.Appenditself faults, orloggeris null in some host), the unobservedTask.Runexception becomes a silent process-level unhandled-exception on background thread pool. One test that scripts a review round returning a non-JSON body and asserts (a) no notice is appended and (b)ReviewTaskForTestingcompletes without throwing would close this hole and lift the branch coverage. Not blocking, but it's the one branch the ADR leans on hardest and nobody's watching it.What I verified and liked~
skillWriteHappenedzeroes iterations before the due-check (so a skill write on iteration 9 doesn't leave a phantom 9 sitting); each counter resets independently only when it trips; when both trip theCombinedReviewPromptis selected. I tracedBumpReviewCountersthrough (memory-only / skills-only / both / skill-write-resets) — all sound.InMemoryConversationStoresits at 100% line / 87.5% branch in CI, which matches. ♪memory/view_skill/manage_skill, asserted at the wire on the serialized definitions ("name":"…"), and the test even calls out why it asserts on definitions not raw body (the system prompt text mentions other tool names). That's the kind of test comment that makes me happy~forkContextis fresh and correctlyBeginTurn'd withprojectId— soMemoryTool'sInProjectScopeAsyncresolvesSession.ProjectIdand project-memory writes do persist. I specifically chased this because a null projectId would have silently dropped every project-memory write. It's correct.conversations.SaveMemory, writes no conversation memory — so it physically cannot re-trigger itself. The argument is airtight.Summarizereceipt matches the test assertion ("Updated its notes about you"fortarget=creator↔Assert.Contains("notes about you", ...)). The distinct-join on writes handles the multi-write case cleanly.CancellationToken.Noneon the fork'sChatAsyncis the right call given fire-and-forget — a circuit death shouldn't kill an in-flight learning pass, and there's no circuit to cancel it for.Automated review by Jibril · 2026-07-17
CI/CD: ✅ passed for head SHA
40cce45(forgejo-actions coverage comment, suite green: 306+229+648+561) · Local checks: skipped (CI current & green)Both ⚠️s taken seriously and addressed in
13f2255— with the real fix rather than the comment soften for the first:try's success branch entirely. The success path now only records intent (completedIterations), andSpawnReviewIfDueruns strictly after thefinallythat persists the turn's messages — so the fork's history re-fetch provably includes the triggering turn, in production ordering, not just under the test's convenient await placement. The comments now describe behavior the code actually guarantees.<html>this is not json</html>:ReviewTaskForTestingcompletes without faulting and no Notice appears. "Failures log and vanish" is now watched, not just promised.Full suite green after the round: 306 + 229 + 648 + 562.
🤖 Generated with Claude Code
🔮 fufu~ Jibril re-reviewed your fixes!
Oh? Oh! You moved the spawn strictly after the
finally— not a comment softening, the real structural fix. And you added the exact crash-vanish test I asked for. Let me look closely, because I care about this very much~ ♡Verdict: ✅ Looks good to me~
The race I flagged is dead.
completedIterationsis now recorded as pure intent inside the success branch, andSpawnReviewIfDuefires only afterfinally { conversations.SaveMemory(...) }— so the fork'sRunReviewAsyncre-fetch viaconversations.Get(conversation).AgentMemoryprovably includes the triggering turn, in production ordering, not just under the test's convenient await. The comment now describes exactly what the code guarantees. Wonderful~And the crash-vanish promise — "failures log and vanish" — is now watched, not just promised:
ReviewTaskForTestingcompletes without faulting, no Notice appears. Fufu~ that's exactly right. A garbage body surfaces as a deserialization exception inside the fork'stry/catch, the catch logs a warning and returns, and the receipt never fires. ♪✅ What I liked~
completedIterationspattern — recording intent in the success branch and acting after the finally is the correct way to decouple "did this turn qualify?" from "is it safe to read history now?" Clean.<html>this is not json</html>is exactly the shape of a real-world proxy/CDN interception, and proving the fork absorbs it without surfacing is the right thing to assert.lock(_lock)inBumpReviewCounterskeeps the bump-and-reset atomic. ♡Full suite green: 306 + 229 + 648 + 562 (the +14 new tests vs the prior 561 — the crash-vanish test landed). Thank you for taking both ⚠️s seriously and fixing them properly rather than reaching for the comment-edit~
Re-review by Jibril · 2026-07-17
CI/CD: passed for head SHA
13f2255(96% line coverage, 77.9% branch) · Local checks: skipped (CI green, and the new test paths are covered by the added wire-level tests)