From 52af9748a2f12b7413c91c66dfdd2a8cebcf1200 Mon Sep 17 00:00:00 2001 From: Costin Stroie Date: Wed, 6 May 2026 12:26:19 +0300 Subject: [PATCH] refactor: move skill catalog handling to dynamic context and update tests --- pkg/agent/context.go | 47 ++++++++++++++++++++------------- pkg/agent/context_cache_test.go | 43 +++++++++++++++++++----------- 2 files changed, 57 insertions(+), 33 deletions(-) diff --git a/pkg/agent/context.go b/pkg/agent/context.go index ecde7c33e..0b58193ff 100644 --- a/pkg/agent/context.go +++ b/pkg/agent/context.go @@ -202,24 +202,6 @@ func (cb *ContextBuilder) BuildSystemPromptParts() []PromptPart { }) } - // Skills - show summary, AI can read full content with read_file tool - skillsSummary := cb.skillsLoader.BuildSkillsSummary() - if skillsSummary != "" { - add(PromptPart{ - ID: "capability.skill_catalog", - Layer: PromptLayerCapability, - Slot: PromptSlotSkillCatalog, - Source: PromptSource{ID: PromptSourceSkillCatalog, Name: "skill:index"}, - Title: "skill catalog", - Content: fmt.Sprintf(`# Skills - -The following skills extend your capabilities. To use a skill, read its SKILL.md file using the read_file tool. - -%s`, skillsSummary), - Stable: true, - Cache: PromptCacheEphemeral, - }) - } // Memory context memoryContext := cb.memory.GetMemoryContext() @@ -314,6 +296,13 @@ func (cb *ContextBuilder) EstimateSystemTokens(summary string, activeSkills []st totalChars := utf8.RuneCountInString(staticPrompt) + dynamicContextChars + // Skill catalog is no longer in the static prompt; add it to the estimate + // (EstimateSystemTokens assumes a non-continuation turn). + if skillsSummary := cb.skillsLoader.BuildSkillsSummary(); skillsSummary != "" { + totalChars += utf8.RuneCountInString(skillsSummary) + 80 // header overhead + totalChars += 7 // separator + } + if skillsText := cb.buildActiveSkillsContext(activeSkills); skillsText != "" { totalChars += utf8.RuneCountInString(skillsText) totalChars += 7 // separator \n\n---\n\n @@ -701,6 +690,28 @@ func (cb *ContextBuilder) BuildMessagesFromPrompt(req PromptBuildRequest) []prov }, &providers.CacheControl{Type: "ephemeral"}), } + // Skip the skill catalog on tool-call continuations: the LLM already saw + // it in the initial turn request and doesn't need it re-sent for every + // intermediate tool round-trip. This saves significant tokens on providers + // without prompt caching (OpenAI-compat). + isToolContinuation := len(req.History) > 0 && req.History[len(req.History)-1].Role == "tool" + if !isToolContinuation { + if skillsSummary := cb.skillsLoader.BuildSkillsSummary(); skillsSummary != "" { + catalogPart := PromptPart{ + ID: "capability.skill_catalog", + Layer: PromptLayerCapability, + Slot: PromptSlotSkillCatalog, + Source: PromptSource{ID: PromptSourceSkillCatalog, Name: "skill:index"}, + Title: "skill catalog", + Content: fmt.Sprintf("# Skills\n\nThe following skills extend your capabilities. To use a skill, read its SKILL.md file using the read_file tool.\n\n%s", skillsSummary), + Stable: true, + Cache: PromptCacheEphemeral, + } + stringParts = append(stringParts, catalogPart.Content) + contentBlocks = append(contentBlocks, promptContentBlock(catalogPart, &providers.CacheControl{Type: "ephemeral"})) + } + } + promptParts := append([]PromptPart(nil), req.Overlays...) promptParts = append(promptParts, cb.buildActiveSkillsPromptParts(req.ActiveSkills)...) if contributedParts, err := cb.promptRegistryOrDefault().Collect(context.Background(), req); err != nil { diff --git a/pkg/agent/context_cache_test.go b/pkg/agent/context_cache_test.go index ef5e6c5de..ef8e45022 100644 --- a/pkg/agent/context_cache_test.go +++ b/pkg/agent/context_cache_test.go @@ -31,6 +31,16 @@ func setupWorkspace(t *testing.T, files map[string]string) string { return tmpDir } +// systemPromptFromMessages extracts the Content of the first system message. +func systemPromptFromMessages(msgs []providers.Message) string { + for _, m := range msgs { + if m.Role == "system" { + return m.Content + } + } + return "" +} + // TestSingleSystemMessage verifies that BuildMessages always produces exactly one // system message regardless of summary/history variations. // Fix: multiple system messages break Anthropic (top-level system param) and @@ -468,8 +478,9 @@ description: global-v1 } cb := NewContextBuilder(tmpDir) - sp1 := cb.BuildSystemPromptWithCache() - if !strings.Contains(sp1, "global-v1") { + // Skill catalog is injected per-request, not in the static cache; check via BuildMessagesFromPrompt. + sysMsg1 := systemPromptFromMessages(cb.BuildMessagesFromPrompt(PromptBuildRequest{})) + if !strings.Contains(sysMsg1, "global-v1") { t.Fatal("expected initial prompt to contain global skill description") } @@ -493,11 +504,11 @@ description: global-v2 t.Fatal("sourceFilesChangedLocked() should detect global skill file content change") } - sp2 := cb.BuildSystemPromptWithCache() - if !strings.Contains(sp2, "global-v2") { + sysMsg2 := systemPromptFromMessages(cb.BuildMessagesFromPrompt(PromptBuildRequest{})) + if !strings.Contains(sysMsg2, "global-v2") { t.Error("rebuilt prompt should contain updated global skill description") } - if sp1 == sp2 { + if sysMsg1 == sysMsg2 { t.Error("cache should be invalidated when global skill file content changes") } } @@ -528,8 +539,9 @@ description: builtin-v1 } cb := NewContextBuilder(tmpDir) - sp1 := cb.BuildSystemPromptWithCache() - if !strings.Contains(sp1, "builtin-v1") { + // Skill catalog is injected per-request, not in the static cache; check via BuildMessagesFromPrompt. + sysMsg1 := systemPromptFromMessages(cb.BuildMessagesFromPrompt(PromptBuildRequest{})) + if !strings.Contains(sysMsg1, "builtin-v1") { t.Fatal("expected initial prompt to contain builtin skill description") } @@ -553,11 +565,11 @@ description: builtin-v2 t.Fatal("sourceFilesChangedLocked() should detect builtin skill file content change") } - sp2 := cb.BuildSystemPromptWithCache() - if !strings.Contains(sp2, "builtin-v2") { + sysMsg2 := systemPromptFromMessages(cb.BuildMessagesFromPrompt(PromptBuildRequest{})) + if !strings.Contains(sysMsg2, "builtin-v2") { t.Error("rebuilt prompt should contain updated builtin skill description") } - if sp1 == sp2 { + if sysMsg1 == sysMsg2 { t.Error("cache should be invalidated when builtin skill file content changes") } } @@ -575,8 +587,9 @@ description: delete-me-v1 defer os.RemoveAll(tmpDir) cb := NewContextBuilder(tmpDir) - sp1 := cb.BuildSystemPromptWithCache() - if !strings.Contains(sp1, "delete-me-v1") { + // Skill catalog is injected per-request, not in the static cache; check via BuildMessagesFromPrompt. + sysMsg1 := systemPromptFromMessages(cb.BuildMessagesFromPrompt(PromptBuildRequest{})) + if !strings.Contains(sysMsg1, "delete-me-v1") { t.Fatal("expected initial prompt to contain skill description") } @@ -592,11 +605,11 @@ description: delete-me-v1 t.Fatal("sourceFilesChangedLocked() should detect deleted skill file") } - sp2 := cb.BuildSystemPromptWithCache() - if strings.Contains(sp2, "delete-me-v1") { + sysMsg2 := systemPromptFromMessages(cb.BuildMessagesFromPrompt(PromptBuildRequest{})) + if strings.Contains(sysMsg2, "delete-me-v1") { t.Error("rebuilt prompt should not contain deleted skill description") } - if sp1 == sp2 { + if sysMsg1 == sysMsg2 { t.Error("cache should be invalidated when skill file is deleted") } }