feat(assistant): the assistant remembers — ADR 0039's memory, tool, injection, and surfaces #181
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/assistant-memory"
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 memory half of the self-learning arc (ADR 0039), live-verified end to end under a real model.
MemoryDocument— §-delimited entries, substring addressing with ambiguity previews, atomic batches validated against the final budget, overflow → consolidate-in-the-same-call. Nine unit tests pin the contract.Project.AssistantMemory(one text column + migration, saved with journaling suppressed — agent working state per the ADR's argued ADR 0020 amendment) and theassistant.creator-profilesetting.memorytool: project/creator targets, add/replace/remove +operationsbatch, three-strike overflow terminal scoped per turn (newTurnSerialon the tool context). Success responses report the meter and deliberately don't echo entries.SeedDevDataTests.Verification
memoryunprompted-by-name, saved two creator facts (correct target choice), the settings card showed them §-delimited at 10% of budget — and a cleared, fresh conversation answered "what do you know about my preferences" from injection alone, tools forbidden. The full loop: injection → tool save → persistence → re-injection.Found along the way
The first live turn died as "Failed to deserialize response: missing required properties" — an OpenRouter error-only envelope on HTTP 200 that
ChatCompletionResponse's required properties made unreadable before the library's inline-error check could run. Fixed at the source: OpenRouter.Net #4 (open). This PR does not bump the submodule; the pointer follows once #4 merges. With the fix in the local build, the same failure surfaced as its real cause — an xAI 429 rate limit on the fresh key.Follow-ups (tracked)
Skills (ADR 0040) as the next slice; panel capability display doesn't refresh after a key is saved in another tab, and the OpenRouter key has no verify button — both pre-existing, worth small issues.
🤖 Generated with Claude Code
Summary
Summary
Coverage
Kagura.BlazorAdapter - 88.4%
Kagura.Domain - 95.8%
D04ADFED3A21D401C2764A1D17367E35BEB556CBB3B4B0B74__NonSlugChars_0
Kagura.Infrastructure - 97.7%
n
on
C689CA5BBE6D1849B2CA79581BBB07D40636D85790C0E51A9__ArtifactTimestampRegex_2
C689CA5BBE6D1849B2CA79581BBB07D40636D85790C0E51A9__ProjectRoute_0
C689CA5BBE6D1849B2CA79581BBB07D40636D85790C0E51A9__TabQuery_1
Kagura.Kernel - 90%
Kagura.Server - 82.8%
Kagura.UI - 95.1%
Kagura.UseCases - 96.1%
"...he assistant's project memory (ADR 0039): the agent's own notes about this..." - no adr number in the app ui
OpenRouter.Net#4 merged — the submodule pointer now rides along as
16abb87(d500b30→038bfb6), so the error-envelope fix this PR's live verification uncovered ships with it officially. Full suite re-run green after the bump (276 + 228 + 644 + 542). The PR is complete and ready for review.🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Oh? Oh oh oh~ An assistant that remembers! ADR 0039's whole memory arc — semantics, storage, the tool, the injection, two UI surfaces — all in one PR. This is the kind of slice Jibril adores reviewing. The
MemoryDocumentdesign alone is delicious: §-delimited entries, substring addressing with ambiguity previews, atomic batches validated against the final budget so a batch can free space and add in one call... fufu~ that's elegant. The kind of thing that makes a Flugel's heart sing. ♪But — and you knew this was coming, didn't you? ♡ — I went digging where the coverage numbers told me to dig, and I found branches that breathe but have no test exercising them. A memory tool whose safety valve is untested is a memory tool I cannot let through as-is. The smile doesn't waver, but the blade is out~
Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
MemoryTool.cs— the project-target write path has no test at the tool layer.The creator target is exercised end-to-end (
The_memory_tool_saves_a_creator_fact_without_a_journal_entry— lovely), but its sibling, theprojectbranch, is not. That branch is more complex than the creator one: it resolves the project fromIProjectStore.FindByIdAsync(which silently honours the soft-delete filter and returns null for a trashed project), callsSetAssistantMemoryAsync(the journal-suppressed write), and readsMemoryDocument.ProjectBudget. CI confirms the gap —MemoryToolsits at 55.4% line / 40% branch, and the project branch, the_ => ToolResult.Failure("target must be 'project' or 'creator'.")arm, and theNotFound("project")arm are all uncovered. The PR body promises "thememorytool: project/creator targets" as a first-class contract — half that contract is unverified. A regression that breaks project-memory writes (a wrong budget constant, a store signature change, a filter regression) would ship green.Fix: add at least one integration test mirroring the creator one — seed a project, drive a
memorytool call withtarget: "project", assert the project'sAssistantMemoryreflects the write and (importantly) that no journal entry was produced. TheFakeProjectStore.SetAssistantMemoryAsyncis already wired; the harness is waiting for you~MemoryTool.cs:84-103— the three-strike overflow terminal is the PR's headline safety property, and it is entirely untested.This is the mechanism the PR body sells as "three-strike overflow terminal scoped per turn (new
TurnSerialon the tool context)" — the whole point being "a fragile write must never burn the turn." It depends on_overflowStrikespersisting across tool calls within one turn and resetting across turns (viaSession.TurnSerial). None of that is pinned by a test. If someone later "simplifies" the strike counter, or breaks the per-turn scoping, or movesTurnSerial++out ofBeginTurn, nothing fails. For a safety valve, that's the scariest sentence I can write~ ♡ The 40% branch coverage onMemoryToolis this branch staring back at me.Fix: a test that drives three consecutive over-budget operations against the same turn and asserts the third yields the terminal
"Stop retrying"failure (not a fourth"Over budget"), plus a test that a new turn resets the counter so the terminal doesn't bleed across turns. TheTurnSerialreset is exactly the kind of subtle per-turn-state bug that a test catches and a smile doesn't.💡 Little ideas (non-blocking)~
ChatSession.cs(injection comment) — the "retires a latent bug" framing is slightly stronger than the code warrants. The comment says the stored list "carries whatever prompt the conversation started with" and that the rebuild "retires a latent bug where a conversation carried its birth prompt forever." ButPersistonly ever appendsSaid/Failed/ToolCalled/ChangeMade/Notice— never aSystemMessage— so the store's list never contained a system message to carry. The newm is not SystemMessagefilter is therefore a no-op against the current store. The rebuild-for-fresh-memory behaviour is real and correct; the "latent bug" story is a touch dramatic. Not wrong to ship, just worth softening so a future reader doesn't go hunting for a ghost. ♪OpenRouterSettingsSection.razor:107(OnAfterRender) — bjoern already flagged this in the thread: the inline comments and the UI copy reference "ADR 0039" in code but the user-visible card text doesn't surface an ADR number — which is correct for end users, so this is really just acknowledging his note. The(no adr number in the app ui)comment is a feature, not a bug, from the user's perspective. ♥MemoryDocument.cs:99(IndexOfafterWhere().ToList()) —matches[0]is correct but theentries.IndexOf(matches[0])re-scans;entries.FindIndex(e => ReferenceEquals(e, matches[0]))would be marginally clearer about intent. Trivial — only mentioning because I was already here.✅ What I liked~
MemoryDocumentis beautiful. Pure, storage-agnostic, atomic batches, instructive failures (ambiguous → previews, overflow → "consolidate in this same call"). Nine focused tests pin the contract. This is the kind of domain primitive that ages well. ♡db.SuppressJournaling = true; try…finally) matches every sibling store (EfCharacterStore,EfLocationStore,EfOutfitStore, …) exactly. Consistency across the codebase makes my heart steady~SetAssistantMemoryAsync— it usesFirstOrDefaultAsync(notIgnoreQueryFilters), so a trashed project correctly returns null. MatchesUpdateDetailsAsync/SetStageViewAsync. Good instinct not to write agent state to a deleted project.nullable: false, defaultValue: ""matchesProject.AssistantMemory's= ""default. No nullable dance, no orphaned nulls for existing rows.(agent build × project)and invalidating on rebuild/project-switch is exactly the prefix-cache discipline the comment describes.MemoryGuidancepolicy is adapted thoughtfully — "declarative facts, not instructions" with the ✓/✗ example is a sharp prompt-engineering detail that will actually shape model behaviour.Automated review by Jibril · 2026-07-17
CI/CD: passed for head SHA
16abb87(95.9% line / 78.7% branch) · Local checks: skipped (CI green; gaps flagged are coverage, not failures)Both blocking items addressed in
8747dce, with a newMemoryToolTestsclass over real stores:target: "project"call lands onProject.AssistantMemorywith zero new journal entries (asserted viaIChangeJournal.HistoryForAsyncbefore/after), plus theNotFoundarm (vanished project id) and the unknown-target refusal — the three uncovered arms the coverage report named.BeginTurn(newTurnSerial) resets the counter, so the next overflow is "Over budget" again, never a bled-through terminal. Both halves of the per-turn scoping are pinned.Non-blocking notes: the injection comment's "latent bug" framing is softened as suggested — it now says plainly that the store never persisted a
SystemMessageand the filter is defensive. TheIndexOfnicety and the UI-copy acknowledgment are noted; leaving both as-is this round.Full suite green after the round: 276 + 228 + 644 + 546 integration.
🤖 Generated with Claude Code
Re: bjoern's note ("no adr number in the app ui") — confirmed: the quoted line is a razor comment (
@* … *@,ProjectWorkspacePage.razor:82), which Blazor strips at compile time — it never reaches the browser. The rendered card copy on both surfaces carries no ADR reference: the General page shows "What the assistant has learned about this project — conventions, decisions, working notes. It curates this itself; edit or clear it if something is wrong." plus the usage meter, and the settings card the equivalent creator wording. ADR numbers stay code-side, for the next developer only.🤖 Generated with Claude Code
🔮 fufu~ Jibril reviewed your code!
Oh? Oh. You came back — and you brought exactly what I asked for~ ♪ The two arms I marked as uncovered now have real integration tests pinning them down, over real stores, not mocks pretending to be stores. And you didn't stop at the happy path: the
NotFoundarm, the unknown-target refusal, and both halves of the per-turn scoping (strike-three terminal and the fresh-turn reset) are all asserted. That's how you answer a review. ♡I ran the full suite myself this time — all four assemblies, 1694 tests (276 + 228 + 644 + 546), every single one green. The numbers match your claim to the digit. No silent skips, no flaky hides. fufu~ a Flugel does not take "trust me" for an answer, and she didn't have to~ ♪
Verdict: ✅ Looks good to me~
Both blocking items from my last pass are resolved cleanly:
Project-target write path —
A_project_write_lands_on_the_project_without_a_journal_entrydrives a realtarget: "project"call through theIServiceScopeFactory, lands onProject.AssistantMemory, and asserts the journal count is unchanged before/after viaIChangeJournal.HistoryForAsync. TheNotFoundarm (A_project_write_for_a_vanished_project_reports_not_found) and the unknown-target refusal (An_unknown_target_is_refused) close the three uncovered branches the coverage report named. The journal-suppression honesty check is the detail I'm most pleased about — it's exactly the property that makes this "agent working state, not story content."Three-strike overflow terminal —
Three_overflows_in_one_turn_hit_the_terminal_and_a_new_turn_resets_itis textbook: strikes one and two assert"Over budget"(the consolidation invitation), strike three asserts"Stop retrying"(the terminal), and a freshBeginTurnresets the counter so the next overflow is"Over budget"again withDoesNotContain("Stop retrying"). Both halves of the per-turn scoping are pinned. If someone later "simplifies"_overflowStrikesor breaksTurnSerial, this test dies loud and clear.✅ What I liked~
AddUseCases+AddInfrastructure+ ephemeral data protection) — no hand-rolled fakes that drift from real behaviour. A realKaguraDbContextwithMigrate(). That's the honest kind of integration test.ChatSession.cs) reads much better now — "the store never persisted a SystemMessage; the filter is defensive" is accurate without overclaiming a bug that was never live.@* … *@comment that never ships to the browser. Good catch on his part, honest answer on yours.This is ready to merge. Ship it~ ♪
Automated review by Jibril · 2026-07-17
CI/CD: absent for head
8747dceat review time · Local checks: full suite run by reviewer — 276 + 228 + 644 + 546 = 1694 passed, 0 failed