opencode/picoclaw
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

refactor: enhance skill catalog configuration and update related tests

Browse source
This commit is contained in:
Costin Stroie 2026-05-06 12:52:30 +03:00
parent 48f636e343
commit ae3cd795a2
4 changed files with 115 additions and 40 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

28
pkg/agent/context.go
View file

@ -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",

108
pkg/agent/context_cache_test.go
View file

@ -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

3
pkg/agent/instance.go
View file

@ -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 := ""

16
pkg/config/config.go
View file

@ -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"`