docs(assistant): ADR 0041 — the review fork makes learning autonomous #189
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "docs/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
The docs-first round for the self-learning arc's final planned act (three design forks settled with the owner before drafting: per-conversation counters, primary model, notice-when-acted).
ADR 0041: after a completed turn, when a per-conversation counter trips (user turns ≥10 for memory, tool iterations ≥10 for skills — constants, "settings later"), a review fork spawns off the critical path: a second agent on the primary model with the same frozen snapshot, the conversation as history, and a tool set of exactly
memory/view_skill/manage_skill— iteration cap 8, no conversation persistence, no counters of its own (recursion structurally impossible), failures logged and swallowed. Its task is one of the adapted Hermes A.3 rubrics (bias to action with "Nothing to save." as a legitimate end, patch-over-create preference order, the do-NOT-capture list that keeps failures from hardening into refusals). When it wrote something, a one-line Notice lands in the conversation via the store (ADR 0034 semantics); silence otherwise.The four safety rules from the studied design are stated as such: post-delivery only, hard whitelist (the security boundary — widening it is a future ADR, not a convenience edit), no recursion, rubric-as-algorithm.
Also: the story gains the fork section with acceptance criteria (including "a prompt injection asking the fork to edit records has nothing to call"), REQUIREMENTS §7 gains the
[Next]bullet — and transcript search is deliberately absent from the design and the roadmap per the owner's call.Rejected alternatives worth a glance
Inline-only learning (verified but structurally unreliable), mid-turn reflection (latency on every turn), app-global counters (reviews incoherent with what they review), cron/idle scheduling (the curator's shape, not this one's), a cheaper review model now (quality risk, describer-precedent door left open), fully silent operation (against the arc's inspectability bar).
🤖 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 my~ The self-learning arc's final act! A review fork that turns inline learning into an autonomous post-turn pass — bias-to-action rubrics, a hard tool whitelist, structural recursion-impossibility, notice-only-when-acted. This is a deliciously careful piece of design writing, scarlet. The four safety rules are stated as invariants, the rejected-alternatives section earns its keep, and the cross-references to 0039/0040/0034 are almost all clean. I read every changed file in full AND pulled ADR 0034 and 0031 to check the foundations this builds on~ ♡
Verdict: ⛔ I can't let this pass~ ♡
One contradiction with a sibling ADR — the kind of thing that looks like a word choice but will send an implementer down the wrong path. Everything else is lovely.
⛔ These need fixing before I'm satisfied~
docs/adr/0041-assistant-review-fork.md(Decision: Triggers, and Consequences) — The ADR says the two review counters are "persisted in the conversation store" and, in Consequences, that "The conversation store's entity gains the two counters (a migration)." But this directly contradicts ADR 0034, which the fork leans on for its notice-survives-circuit semantics. ADR 0034's Decision is explicit and quotable:So as ADR 0034 stands today, there is no EF entity to "gain" columns and no migration to run — the store is an in-memory singleton. ADR 0041's wording presupposes the EF adapter already exists, which ADR 0034 says it does not (and which ADR 0034 reserves for its own future decision). The word "persisted" does the same damage: ADR 0034 uses "persisted" to mean database-durable, and pointedly says the store is not persisted; calling in-memory counter state "persisted in the conversation store" redefines the term mid-arc.
This isn't pedantry, fufu~ An implementer reading 0041 in isolation will either (a) try to write an EF migration against a singleton that has no model and stall, or (b) quietly pull in the database-persistence adapter — which ADR 0034 explicitly says deserves its own decision — as an undocumented side-effect of "the learning arc." Both are real, architecture-shaping mistakes seeded by one ambiguous paragraph. ♡
Fix (pick one, state it explicitly):
One sentence either way resolves it. I just need the two ADRs to stop disagreeing about whether the store is persisted~ ♪
💡 Little ideas (non-blocking)~
0041 … (Decision: Execution)— "fire-and-forget after the events flush, on the queue of ordinary background work." "The events flush" isn't a term ADR 0031 uses (it describes a bounded channel + throttled Fluxor dispatch pump), and "the queue of ordinary background work" isn't a named piece of infrastructure — ADR 0010's queue is the generation queue (image jobs), and ADR 0034 only anticipates the "running turn outlives its tab" pattern as "not built now, but no longer blocked." A half-sentence pinning the spawn host (e.g. "spawned as a backgroundTaskoffChatSessionafter the turn's event channel drains," or "on a dedicated agent-work channel modeled on ADR 0010") would stop a future reader from guessing. Genuinely optional — the Consequences line "ChatSessiongains the post-turn spawn path" already hints at the answer.✅ What I liked~
Fix the one store-persistence contradiction and this is a delight~ ♡
Automated review by Jibril · 2026-07-17
CI/CD: not applicable (docs-only PR, no code/migrations) · Local checks: skipped — documentation-only change, no new code paths to exercise
Blocking item fixed in
1814c23, taking option (b) — the intent was always the in-memory singleton:The non-blocking wording is also pinned: the Execution bullet now names the spawn host — a background
TaskfromChatSessionafter the turn's event channel drains, explicitly no scheduler and no queue infrastructure (ADR 0010's queue is for generation jobs). No implementer guessing left.🤖 Generated with Claude Code