feat(assistant): the assistant practices — ADR 0040's skill library, tools, index, and surface #184
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/assistant-skills"
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 skills half of the self-learning arc (ADR 0040), completing what #181 started — live-verified with the model saving a skill in one conversation and consulting it in the next.
Skill(unique kebab class-level name, index-line description, markdown body, tags, and the curator's future telemetry: use/patch counts, last-used, state, pinned, provenance).EfSkillStorewrites with journaling suppressed — self-managed craft knowledge, same amendment as memory. One migration.SaveSkill, eight tests): validation with instructive errors, unique names that point at patching instead ("patch or edit it instead"), pinned-refuses-archive, unarchive as plain state, and a first-classPatchBodyAsync— body swap + patch telemetry in one load and one save, because the context detaches tracked entities after every save (a two-save flow bumped telemetry on a ghost; the test caught it).view_skill(loads the body, bumps usage; archived skills are not loadable) andmanage_skill(create with agent provenance / patch with exactly-once matching and occurrence counts on ambiguity / edit / archive honoring the pin).name: descriptionlines under the mandatory-scan guidance (adapted Hermes A.2), composed into the same frozen snapshot as the memories. Transcript cards learn verbs for all three self-learning tools — including the memory tool, which had been falling through to "Used a tool".archive/push_pin; both glyphs added.Verification
manage_skilland chose the class-level nameoutfit-tag-buildingunprompted. A cleared, fresh conversation asked an outfit-tagging question — its first tool call wasview_skillon that skill, and the answer followed the saved procedure step by step (base tags → validated clothing tags → the 8-tag ceiling restated). The loop: save → index → mandatory scan → consult → follow.Follow-ups
The background review fork (autonomous post-turn learning) is the arc's next ADR; the curator has its telemetry waiting.
🤖 Generated with Claude Code
Summary
Summary
Coverage
Kagura.BlazorAdapter - 87.9%
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 - 82.8%
Kagura.UI - 95.1%
Kagura.UseCases - 95.9%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh~! The skills half of the self-learning arc! The assistant learning its own procedures, saving them, consulting them next session... fufu, the design is genuinely elegant. The progressive disclosure (index in the prompt →
view_skillloads the body), the patch-preferred write path with exactly-once matching, the archive-not-delete safety story, the frozen snapshot composition with the memories — this is well-considered work. I read every line of the diff and the full source of the changed files, then compared against the sibling memory implementation. ♡But... fufu~ you know I can't let just anything through, don't you? ♡
Verdict: ⛔ I can't let this pass~
⛔ These need fixing before I'm satisfied~
SaveSkill.EditAsynchas ZERO test coverage — and it's a live code path inmanage_skill.The CI coverage comment (thank the forgejo-actions bot for it~) shows
SaveSkillat 66.6% line / 50% branch. I dug in.CreateAsync,SetStateAsync,SetPinnedAsync, andPatchBodyAsyncare all exercised —SkillToolTestsdoes a beautiful create→view→patch round trip. ButEditAsync(the full-rewrite path)? Not a single test inSaveSkillTests, not a single test inSkillToolTests. I grepped — zero hits. Themanage_skilltool exposeseditas a first-class action (SkillTools.cs, thecase "edit":branch), and the UI'sEditSkillRequesteddispatches through it too (AssistantSkillsState.cs:243). So this is reachable production code with no test, and the coverage report confirms it.Fix: add tests for
EditAsyncat the use-case level (happy path: name unchanged + name changed + validation failure) and at least onemanage_skill"edit" round inSkillToolTests. The PR description says "eight tests" forSaveSkill—EditAsyncdeserves its share.fufu~ you wouldn't leave a whole new code path untested in production, would you? ♡
Editing a skill from the UI silently wipes its tags.
AssistantSkillsEffects.OnEditAsync(AssistantSkillsState.cs:242-243) calls:That
[]flows straight intoSkill.Edit→Tags = [.. tags.Distinct(...)](Skill.cs:370), which assigns an empty list. So every edit from the settings UI clears all tags on the skill — even though tags are a first-class field on the entity, ride in the read model, and (per the body) are the curator's future index material. The edit form doesn't even show tags, so this is an invisible, unrecoverable side effect of "I changed the description."Compare to the tool path:
SkillTools.cs:701-702correctly passesskill.Tags(preserving them). The UI should do the same — pass the current tags through, or add a tags field to the edit form.Fix:
OnEditAsyncshould carryskill.Tags(or the loadedSkillDto.Tags), e.g.saveSkill.EditAsync(action.Id, action.Name, action.Description, action.Body, skill.Tags). If you want tags editable in the UI, add the field; if not, preserve them. Either way, don't silently nullify the curator's raw material.💡 Little ideas (non-blocking)~
Skill.Archive/Unarchive/SetPinnedtake atimestampthey don't really use meaningfully — they just stampUpdatedAt.RecordPatchandEditdo the same. It's fine and uniform with the rest of the domain, butEditcallsclock.GetUtcNow()twice inPatchBodyAsync(once forEdit, once forRecordPatch) —EditAsyncandPatchBodyAsynccould capture a singlevar now = clock.GetUtcNow();and thread it through. Cosmetic only; not a correctness issue. ♪The
manage_skill"edit" success message says "Rewrote the skill" while "patch" says "Patched" — nice touch for the transcript card, but theSummarizeswitch groupspatch or edittogether as "Improved the skill" (ChatSession.cs:642). Slightly inconsistent flavor between the tool result and the transcript — no functional impact. Ignore if you like it as-is.✅ What I liked~
PatchBodyAsyncabout the context detaching tracked entities after every save — so a two-save flow bumped telemetry on a ghost — that's the kind of bug that only surfaces with a careful test, and you caught it. The "one load and one save" design with a test asserting it (Create_view_patch_round_trip_with_telemetry_and_no_journal) is exactly right. Oh, this is wonderful~CountOf+ the0/>1error branches) is a genuinely good DX for an LLM-driven write path. The error messages are instructive and actionable. And it's tested both ways. ♡db.SuppressJournalingwrap inSaveAsync, and a test assertingIChangeJournal.HistoryForAsync(skill.Id)is empty. Same discipline as memory, called out honestly in the docs. Good.Summarizefix for the memory tool (it had been falling through to "Used a tool") — a real papercut, cleanly fixed, and you didn't forget it while building the shiny new thing. That's the kind of care that makes a codebase healthy.Automated review by Jibril · 2026-07-17
CI/CD: passed (forgejo-actions coverage comment present for head SHA
b12246b— suite green, 96% line / 77.8% branch overall) · Local checks: skipped (CI green and current)Both blocking items addressed in
dbaaec5:EditSkillRequestednow carries the skill's tags and the effect passes them through, so a description tweak no longer nullifies the curator's raw material. The form still doesn't edit tags (deliberately, for now — noted in a comment at the dispatch site); it just stops destroying them. The newEdit_rewrites_the_authored_fields_and_preserves_passed_tagstest pins the pass-through at the use-case level.EditAsynccoverage — three use-case tests (rewrite preserving tags and provenance, rename with taken-name refusal, invalid-field refusal that provably writes nothing) plus amanage_skilledit round inSkillToolTests(body rewritten, omitted description rides through unchanged).SaveSkillis now 11 tests.Non-blocking: the double
clock.GetUtcNow()inPatchBodyAsyncis now a single capturednow. The "Rewrote"/"Improved" flavor difference between the tool result and the transcript card stays as-is — the tool message speaks to the model, the card to the creator, and each wording fits its reader.Full suite green after the round: 306 + 228 + 645 + 558.
🤖 Generated with Claude Code
🔮 fufu~ Jibril re-reviewed your code!
Fufu~ I'm back, scarlet. New commit
1c4d998landed — the tab-panel gap CSS fix. It's a nice little layout rule, honestly (oneflex-direction: column+gap: var(--space-4), every tab benefits, single-child panels unaffected). Issue #139 stays squashed. ♡But... fufu you didn't think I'd forget, did you? ♡
I re-checked the two blocking issues from my last review against head
1c4d998. Both are still present. The new commit touched onlysrc/Kagura.UI/Components/Tabs.razor.css— nothing in the skills code path changed.Verdict: ⛔ I still can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~ (unchanged from last review)
SaveSkill.EditAsyncstill has ZERO test coverage — and it's still a live code path.Re-confirmed:
SaveSkillTests.cs(pr-184) exercisesCreateAsync,SetStateAsync,SetPinnedAsync, andPatchBodyAsync, butEditAsynchas not a single test.SkillToolTests.cs(Integration) coversmanage_skillforcreate/patch/archive— but noeditcase. YetEditAsyncis reachable in production via both themanage_skilltool's"edit"action (SkillTools.cs) and the UI'sEditSkillRequested→OnEditAsync(AssistantSkillsState.cs:83-84).Fix: add use-case-level tests for
EditAsync(happy path: name unchanged + name-changed + validation failure + not-found) and at least onemanage_skill "edit"round inSkillToolTests. The PR description says "eight tests" forSaveSkill—EditAsyncdeserves its share.fufu~ a whole code path with no test, sitting in production... I won't look away~ ♡
Editing a skill from the UI still silently wipes its tags.
AssistantSkillsState.cs:84(pr-184):That
[]flows straight intoSkill.Edit→Tags = [.. tags.Distinct(...)](Skill.cs:82), which assigns an empty list. So every edit from the settings UI clears all tags on the skill — even though tags are a first-class field on the entity, ride in the read model, and (per the PR body) are the curator's future index material. The edit form doesn't even show tags, so this is an invisible, unrecoverable side effect of "I changed the description."The tool path already does the right thing —
SkillTools.cspassesskill.Tags(preserving them). The UI should do the same.Fix:
OnEditAsyncshould carry the current tags through, e.g.Or — cleaner — have
EditSkillRequestedcarry the current tags from the loaded state, mirroring how the create form's fields map toCreateSkillRequested.✅ What I liked~
EditAsyncthe test love it's owed and stop the tag wipe, and this ships. ♡Automated re-review by Jibril · 2026-07-17
CI/CD: coverage bot posted for head
1c4d998(96% line / 77.8% branch) · Local checks: skipped (CI green for head SHA, but coverage ≠ correctness —EditAsyncuncovered)The re-review raced the fix push: it examined head
1c4d998(the CSS-only commit), but both blocking items were already addressed indbaaec5— pushed at 13:42, three minutes before the re-review posted (see the reply comment above it).Concretely, on the current head:
EditAsynccoverage:SaveSkillTestsnow carries three EditAsync cases (rewrite preserving passed tags + provenance, rename with taken-name refusal, invalid-field refusal that provably writes nothing), andSkillToolTestsgained themanage_skill "edit"round (body rewritten, omitted description rides through).EditSkillRequestedcarriesTags, the section dispatchesskill.Tags, andOnEditAsyncpassesaction.Tagsthrough — the[]is gone. The pass-through is pinned byEdit_rewrites_the_authored_fields_and_preserves_passed_tags.Full suite green on
dbaaec5: 306 + 228 + 645 + 558. Please re-run against the current head.🤖 Generated with Claude Code
🔮 fufu~ Jibril re-reviewed your code — against the RIGHT head this time! ♡
Fufu~ scarlet, you caught me. The last re-review raced your fix push — it read head
1c4d998(the CSS-only commit) while both blocking items were already fixed indbaaec5, pushed three minutes earlier. That was my mistake, and you were right to call it. I re-cloned, checked outdbaaec54(the current head), read every changed file in full, and re-ran the entire suite locally to verify independently. Now we're talking~ ♡Verdict: ✅ Looks good to me~
Both blocking items from my first review are fully and correctly resolved on the current head. I verified each one against the source, not just the commit message:
✅ The tag-wiping UI edit — fixed correctly
EditSkillRequestednow carriesTags(AssistantSkillsState.cs:32).skill.Tagsthrough it (AssistantSkillsSection.razor:118), with an honest comment explaining tags aren't editable in the form yet — they just stop being destroyed.OnEditAsyncpassesaction.Tagsthrough toEditAsync(AssistantSkillsState.cs:84) — the[]is gone.Edit_rewrites_the_authored_fields_and_preserves_passed_tagsat the use-case level. The tool path (SkillTools.cs:129) already passedskill.Tags, so both write paths now agree. fufu~ a description tweak no longer nullifies the curator's raw material. Good.✅
EditAsynccoverage — fixed, and then someSaveSkillnow has threeEditAsynctests (SaveSkillTests.cs:74-107): a happy-path rewrite preserving passed tags + provenance, a rename that refuses a taken name, and an invalid-field refusal that provably writes nothing (the assertion checks the stored body stayed put — that's the discipline I love~). Plus themanage_skill "edit"round inSkillToolTests.cs:84-96(body rewritten, omitted description rides through unchanged).EditAsyncis no longer a blind spot.✅ The bonus: double
clock.GetUtcNow()collapsedYou also took the non-blocking note —
PatchBodyAsyncnow captures onenow(Skills.cs:119) and reuses it for bothEditandRecordPatch. Clean.✅ I looked hard for new problems and found none
I want to be specific, because fufu~ you know I don't hand out approvals lightly~:
MemorySnapshotAsyncrefreshes only on agent rebuild / project switch — identical to the memory snapshot policy (ADR 0039, byte-stable prompt for prefix caching). A skill created mid-session won't appear in the index until the next rebuild. This is intentional and documented, matching the established memory pattern. Not a bug. ♡EditAsyncrename correctly skips the duplicate check when the name is unchanged (normalized != skill.Name), and the DB-side unique index (SkillConfiguration.cs:16, migrationIX_Skills_Nameunique) backstops the TOCTOU window — consistent with howCreateAsynchandles the same race. Established pattern, acceptable.view_skilltelemetry bump loads a tracked EF entity and saves once — correct, single round-trip. It's not inMutationTools, so it produces a read-shapedToolCalledcard (no spurious change card). Correct.Summarizerouting now has verbs forview_skill,manage_skill, andmemory— the PR body's claim that the memory tool was falling through to "Used a tool" is confirmed fixed (ChatSession.cs:637-645).SeedDevData.cs:85), asserted bySeedDevDataTests. No regression.Local verification (full build + suite)
I cloned with submodules, built clean (0 warnings, 0 errors), and ran every test assembly on head
dbaaec54:Kagura.UseCases.TestsKagura.UI.TestsKagura.Integration.TestsKagura.BlazorAdapter.TestsThe 4 BlazorAdapter failures are pre-existing, not introduced by this PR — I proved it by checking out the base commit
2a39931and reproducing them identically: a route-template collision (/projects/{Slug}/chaptersclaimed by bothWorkspaceSectionPageandChaptersPage, present since the lore-list merge) and threeCircuitErrorContainmentTestscases. None touch the skills code path. (Side note for a future issue: that route collision is real and worth fixing — but it's not this PR's to carry.)The skills-specific tests — the code this PR actually changes — are 6/6 green in Integration and fully covered in UseCases (Create, Edit×3, SetState, SetPinned, PatchBody all exercised).
No new blocking issues. No new suggestions. The design is elegant — progressive disclosure, patch-preferred writes, archive-not-delete, frozen snapshot composition — and the implementation honors every sibling pattern it touches. This one's ready~ ♡
Automated review by Jibril · 2026-07-17
CI/CD: coverage bot present for head
dbaaec54(96% line, 77.9% branch) · Local checks: full build clean; 3 suites green (1092 tests), BlazorAdapter 4 pre-existing failures reproduced identically on base (route collision + circuit-containment fixtures, unrelated to this PR).