From ae3cd795a2ec89f3bbccfa6585304e2a99ac9d58 Mon Sep 17 00:00:00 2001 From: Costin Stroie Date: Wed, 6 May 2026 12:52:30 +0300 Subject: [PATCH] refactor: enhance skill catalog configuration and update related tests --- pkg/agent/context.go | 28 +++++---- pkg/agent/context_cache_test.go | 108 ++++++++++++++++++++++++-------- pkg/agent/instance.go | 3 +- pkg/config/config.go | 16 ++++- 4 files changed, 115 insertions(+), 40 deletions(-) diff --git a/pkg/agent/context.go b/pkg/agent/context.go index fc902bd5c..da5b49915 100644 --- a/pkg/agent/context.go +++ b/pkg/agent/context.go @@ -22,11 +22,12 @@ import ( ) type ContextBuilder struct { - workspace string - skillsLoader *skills.SkillsLoader - memory *MemoryStore - splitOnMarker bool - promptRegistry *PromptRegistry + workspace string + skillsLoader *skills.SkillsLoader + memory *MemoryStore + splitOnMarker bool + skillCatalogCfg config.SkillCatalogConfig + promptRegistry *PromptRegistry // Cache for system prompt to avoid rebuilding on every call. // This fixes issue #607: repeated reprocessing of the entire context. @@ -66,6 +67,11 @@ func (cb *ContextBuilder) WithSplitOnMarker(enabled bool) *ContextBuilder { return cb } +func (cb *ContextBuilder) WithSkillCatalogConfig(cfg config.SkillCatalogConfig) *ContextBuilder { + cb.skillCatalogCfg = cfg + return cb +} + func getGlobalConfigDir() string { return config.GetHome() } @@ -690,16 +696,14 @@ func (cb *ContextBuilder) BuildMessagesFromPrompt(req PromptBuildRequest) []prov }, &providers.CacheControl{Type: "ephemeral"}), } - // Inject the skill catalog only when the LLM needs to (re)discover available skills: - // - Turn 1: no history yet, LLM hasn't seen the catalog. - // - After compaction: history was summarized; early turns (including the original - // catalog injection) are gone, so the LLM must see it again. - // Skip it on tool-call continuations (mid-turn round-trips) and on ordinary - // subsequent turns where the catalog is already in the LLM's context window. + // Determine whether to inject the skill catalog. + // Both skip behaviours are opt-in via config (default: always include). isToolContinuation := len(req.History) > 0 && req.History[len(req.History)-1].Role == "tool" isFirstTurn := len(req.History) == 0 isAfterCompaction := req.Summary != "" - if !isToolContinuation && (isFirstTurn || isAfterCompaction) { + skipForTools := cb.skillCatalogCfg.SkipOnTools && isToolContinuation + skipForSubsequent := cb.skillCatalogCfg.SkipOnSubsequent && !isFirstTurn && !isAfterCompaction && !isToolContinuation + if !skipForTools && !skipForSubsequent { if skillsSummary := cb.skillsLoader.BuildSkillsSummary(); skillsSummary != "" { catalogPart := PromptPart{ ID: "capability.skill_catalog", diff --git a/pkg/agent/context_cache_test.go b/pkg/agent/context_cache_test.go index 86cb674e8..c561a13d5 100644 --- a/pkg/agent/context_cache_test.go +++ b/pkg/agent/context_cache_test.go @@ -8,6 +8,7 @@ import ( "testing" "time" + "github.com/sipeed/picoclaw/pkg/config" "github.com/sipeed/picoclaw/pkg/providers" ) @@ -614,15 +615,14 @@ description: delete-me-v1 } } -// TestSkillCatalogInjectionPolicy verifies that the catalog is included only -// when the LLM needs to (re)discover skills: turn 1 and after compaction. +// TestSkillCatalogInjectionPolicy verifies catalog inclusion under various +// config combinations. func TestSkillCatalogInjectionPolicy(t *testing.T) { tmpDir := setupWorkspace(t, map[string]string{ "skills/demo/SKILL.md": "---\nname: demo\ndescription: \"demo skill\"\n---\n# Demo", }) defer os.RemoveAll(tmpDir) - cb := NewContextBuilder(tmpDir) userMsg := providers.Message{Role: "user", Content: "hello"} assistantMsg := providers.Message{Role: "assistant", Content: "hi"} toolMsg := providers.Message{Role: "tool", Content: "result", ToolCallID: "tc1"} @@ -631,32 +631,90 @@ func TestSkillCatalogInjectionPolicy(t *testing.T) { return strings.Contains(systemPromptFromMessages(msgs), "demo skill") } - // Turn 1: no history — catalog must appear. - if !contains(cb.BuildMessagesFromPrompt(PromptBuildRequest{})) { - t.Error("turn 1 (no history): catalog should be included") + newCB := func(skipOnTools, skipOnSubsequent bool) *ContextBuilder { + return NewContextBuilder(tmpDir).WithSkillCatalogConfig(config.SkillCatalogConfig{ + SkipOnTools: skipOnTools, + SkipOnSubsequent: skipOnSubsequent, + }) } - // Tool continuation: last message is a tool result — catalog must be skipped. - if contains(cb.BuildMessagesFromPrompt(PromptBuildRequest{ - History: []providers.Message{userMsg, assistantMsg, toolMsg}, - })) { - t.Error("tool continuation: catalog should be skipped") - } + t.Run("default (both false): catalog always included", func(t *testing.T) { + cb := newCB(false, false) + for _, req := range []PromptBuildRequest{ + {}, + {History: []providers.Message{userMsg, assistantMsg}}, + {History: []providers.Message{userMsg, assistantMsg, toolMsg}}, + {History: []providers.Message{userMsg, assistantMsg}, Summary: "summary"}, + } { + if !contains(cb.BuildMessagesFromPrompt(req)) { + t.Error("catalog should always be included when both flags are false") + } + } + }) - // Turn > 1, no compaction: catalog must be skipped. - if contains(cb.BuildMessagesFromPrompt(PromptBuildRequest{ - History: []providers.Message{userMsg, assistantMsg}, - })) { - t.Error("turn > 1, no summary: catalog should be skipped") - } + t.Run("skip_on_tools: skips tool continuations only", func(t *testing.T) { + cb := newCB(true, false) + if !contains(cb.BuildMessagesFromPrompt(PromptBuildRequest{})) { + t.Error("turn 1: catalog should be included") + } + if !contains(cb.BuildMessagesFromPrompt(PromptBuildRequest{ + History: []providers.Message{userMsg, assistantMsg}, + })) { + t.Error("turn > 1 (no tool): catalog should be included") + } + if contains(cb.BuildMessagesFromPrompt(PromptBuildRequest{ + History: []providers.Message{userMsg, assistantMsg, toolMsg}, + })) { + t.Error("tool continuation: catalog should be skipped") + } + }) - // After compaction (summary present): catalog must be re-injected. - if !contains(cb.BuildMessagesFromPrompt(PromptBuildRequest{ - History: []providers.Message{userMsg, assistantMsg}, - Summary: "prior conversation summary", - })) { - t.Error("after compaction (summary present): catalog should be re-injected") - } + t.Run("skip_on_subsequent: skips turns > 1, re-injects after compaction", func(t *testing.T) { + cb := newCB(false, true) + if !contains(cb.BuildMessagesFromPrompt(PromptBuildRequest{})) { + t.Error("turn 1: catalog should be included") + } + if contains(cb.BuildMessagesFromPrompt(PromptBuildRequest{ + History: []providers.Message{userMsg, assistantMsg}, + })) { + t.Error("turn > 1, no summary: catalog should be skipped") + } + if !contains(cb.BuildMessagesFromPrompt(PromptBuildRequest{ + History: []providers.Message{userMsg, assistantMsg}, + Summary: "prior conversation summary", + })) { + t.Error("after compaction: catalog should be re-injected") + } + // tool continuation is NOT skipped when only skip_on_subsequent is set + if !contains(cb.BuildMessagesFromPrompt(PromptBuildRequest{ + History: []providers.Message{userMsg, assistantMsg, toolMsg}, + })) { + t.Error("tool continuation (skip_on_subsequent only): catalog should be included") + } + }) + + t.Run("both true: skips tool turns and subsequent turns, re-injects after compaction", func(t *testing.T) { + cb := newCB(true, true) + if !contains(cb.BuildMessagesFromPrompt(PromptBuildRequest{})) { + t.Error("turn 1: catalog should be included") + } + if contains(cb.BuildMessagesFromPrompt(PromptBuildRequest{ + History: []providers.Message{userMsg, assistantMsg, toolMsg}, + })) { + t.Error("tool continuation: catalog should be skipped") + } + if contains(cb.BuildMessagesFromPrompt(PromptBuildRequest{ + History: []providers.Message{userMsg, assistantMsg}, + })) { + t.Error("turn > 1, no summary: catalog should be skipped") + } + if !contains(cb.BuildMessagesFromPrompt(PromptBuildRequest{ + History: []providers.Message{userMsg, assistantMsg}, + Summary: "prior conversation summary", + })) { + t.Error("after compaction: catalog should be re-injected") + } + }) } // TestConcurrentBuildSystemPromptWithCache verifies that multiple goroutines diff --git a/pkg/agent/instance.go b/pkg/agent/instance.go index d0b25a0a8..1ca1443e5 100644 --- a/pkg/agent/instance.go +++ b/pkg/agent/instance.go @@ -127,7 +127,8 @@ func NewAgentInstance( mcpDiscoveryActive && cfg.Tools.MCP.Discovery.UseBM25, mcpDiscoveryActive && cfg.Tools.MCP.Discovery.UseRegex, ). - WithSplitOnMarker(cfg.Agents.Defaults.SplitOnMarker) + WithSplitOnMarker(cfg.Agents.Defaults.SplitOnMarker). + WithSkillCatalogConfig(cfg.Agents.Defaults.SkillCatalog) agentID := routing.DefaultAgentID agentName := "" diff --git a/pkg/config/config.go b/pkg/config/config.go index acceee4d5..8305a6914 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -254,6 +254,17 @@ type ToolFeedbackConfig struct { SeparateMessages bool `json:"separate_messages" env:"PICOCLAW_AGENTS_DEFAULTS_TOOL_FEEDBACK_SEPARATE_MESSAGES"` } +type SkillCatalogConfig struct { + // SkipOnTools omits the skill catalog from tool-call continuation requests + // (mid-turn LLM round-trips). The LLM already received the catalog on the + // initial turn request. Default false (catalog always included). + SkipOnTools bool `json:"skip_on_tools" env:"PICOCLAW_AGENTS_DEFAULTS_SKILL_CATALOG_SKIP_ON_TOOLS"` + // SkipOnSubsequent omits the skill catalog on turns after the first in a + // session. The catalog is still re-injected after context compaction. + // Default false (catalog always included). + SkipOnSubsequent bool `json:"skip_on_subsequent" env:"PICOCLAW_AGENTS_DEFAULTS_SKILL_CATALOG_SKIP_ON_SUBSEQUENT"` +} + type AgentDefaults struct { Workspace string `json:"workspace" env:"PICOCLAW_AGENTS_DEFAULTS_WORKSPACE"` RestrictToWorkspace bool `json:"restrict_to_workspace" env:"PICOCLAW_AGENTS_DEFAULTS_RESTRICT_TO_WORKSPACE"` @@ -274,8 +285,9 @@ type AgentDefaults struct { SteeringMode string `json:"steering_mode,omitempty" env:"PICOCLAW_AGENTS_DEFAULTS_STEERING_MODE"` // "one-at-a-time" (default) or "all" MaxParallelTurns int `json:"max_parallel_turns,omitempty" env:"PICOCLAW_AGENTS_DEFAULTS_MAX_PARALLEL_TURNS"` // Max concurrent turns (0 or 1 = sequential) SubTurn SubTurnConfig `json:"subturn" envPrefix:"PICOCLAW_AGENTS_DEFAULTS_SUBTURN_"` - ToolFeedback ToolFeedbackConfig `json:"tool_feedback,omitempty"` - SplitOnMarker bool `json:"split_on_marker" env:"PICOCLAW_AGENTS_DEFAULTS_SPLIT_ON_MARKER"` // split messages on <|[SPLIT]|> marker + ToolFeedback ToolFeedbackConfig `json:"tool_feedback,omitempty"` + SplitOnMarker bool `json:"split_on_marker" env:"PICOCLAW_AGENTS_DEFAULTS_SPLIT_ON_MARKER"` // split messages on <|[SPLIT]|> marker + SkillCatalog SkillCatalogConfig `json:"skill_catalog,omitempty"` ContextManager string `json:"context_manager,omitempty" env:"PICOCLAW_AGENTS_DEFAULTS_CONTEXT_MANAGER"` ContextManagerConfig json.RawMessage `json:"context_manager_config,omitempty" env:"PICOCLAW_AGENTS_DEFAULTS_CONTEXT_MANAGER_CONFIG"` MaxLLMRetries int `json:"max_llm_retries,omitempty" env:"PICOCLAW_AGENTS_DEFAULTS_MAX_LLM_RETRIES"`