diff --git a/pkg/agent/context.go b/pkg/agent/context.go index b7c6e1108..dcf59b02c 100644 --- a/pkg/agent/context.go +++ b/pkg/agent/context.go @@ -170,10 +170,9 @@ func (cb *ContextBuilder) InvalidateCache() { logger.DebugCF("agent", "System prompt cache invalidated", nil) } -// sourcePaths returns the workspace source file paths tracked for cache -// invalidation (bootstrap files + memory). The skills directory is handled -// separately in sourceFilesChangedLocked because it requires both directory- -// level and recursive file-level mtime checks. +// sourcePaths returns non-skill workspace source files tracked for cache +// invalidation (bootstrap files + memory). Skill roots are handled separately +// because they require both directory-level and recursive file-level checks. func (cb *ContextBuilder) sourcePaths() []string { return []string{ filepath.Join(cb.workspace, "AGENTS.md"), @@ -184,6 +183,20 @@ func (cb *ContextBuilder) sourcePaths() []string { } } +// skillRoots returns all skill root directories that can affect +// BuildSkillsSummary output (workspace/global/builtin). +func (cb *ContextBuilder) skillRoots() []string { + if cb.skillsLoader == nil { + return []string{filepath.Join(cb.workspace, "skills")} + } + + roots := cb.skillsLoader.SkillRoots() + if len(roots) == 0 { + return []string{filepath.Join(cb.workspace, "skills")} + } + return roots +} + // cacheBaseline holds the file existence snapshot and the latest observed // mtime across all tracked paths. Used as the cache reference point. type cacheBaseline struct { @@ -195,10 +208,10 @@ type cacheBaseline struct { // the latest mtime across all tracked files + skills directory contents. // Called under write lock when the cache is built. func (cb *ContextBuilder) buildCacheBaseline() cacheBaseline { - skillsDir := filepath.Join(cb.workspace, "skills") + skillRoots := cb.skillRoots() - // All paths whose existence we track: source files + skills dir. - allPaths := append(cb.sourcePaths(), skillsDir) + // All paths whose existence we track: source files + all skill roots. + allPaths := append(cb.sourcePaths(), skillRoots...) existed := make(map[string]bool, len(allPaths)) var maxMtime time.Time @@ -211,17 +224,19 @@ func (cb *ContextBuilder) buildCacheBaseline() cacheBaseline { } } - // Walk skills files to capture their mtimes too. + // Walk all skill roots recursively to capture skill file mtimes too. // Use os.Stat (not d.Info) to match the stat method used in // fileChangedSince / skillFilesModifiedSince for consistency. - _ = filepath.WalkDir(skillsDir, func(path string, d fs.DirEntry, walkErr error) error { - if walkErr == nil && !d.IsDir() { - if info, err := os.Stat(path); err == nil && info.ModTime().After(maxMtime) { - maxMtime = info.ModTime() + for _, root := range skillRoots { + _ = filepath.WalkDir(root, func(path string, d fs.DirEntry, walkErr error) error { + if walkErr == nil && !d.IsDir() { + if info, err := os.Stat(path); err == nil && info.ModTime().After(maxMtime) { + maxMtime = info.ModTime() + } } - } - return nil - }) + return nil + }) + } // If no tracked files exist yet (empty workspace), maxMtime is zero. // Use a very old non-zero time so that: @@ -255,22 +270,18 @@ func (cb *ContextBuilder) sourceFilesChangedLocked() bool { } } - // --- Skills directory (handled separately from sourcePaths) --- + // --- Skill roots (workspace/global/builtin) --- // - // 1. Creation/deletion: tracked via existedAtCache, same as bootstrap files. - skillsDir := filepath.Join(cb.workspace, "skills") - if cb.fileChangedSince(skillsDir) { - return true - } - - // 2. Structural changes (add/remove entries inside the dir) are reflected - // in the directory's own mtime, which fileChangedSince already checks. - // - // 3. Content-only edits to files inside skills/ do NOT update the parent - // directory mtime on most filesystems, so we recursively walk to check - // individual file mtimes at any nesting depth. - if skillFilesModifiedSince(skillsDir, cb.cachedAt) { - return true + // For each root: + // 1. Creation/deletion and directory mtime changes are tracked by fileChangedSince. + // 2. Content-only edits inside the tree are tracked by recursive file mtime checks. + for _, root := range cb.skillRoots() { + if cb.fileChangedSince(root) { + return true + } + if skillFilesModifiedSince(root, cb.cachedAt) { + return true + } } return false diff --git a/pkg/agent/context_cache_test.go b/pkg/agent/context_cache_test.go index ba70d4c0d..fc00fbc08 100644 --- a/pkg/agent/context_cache_test.go +++ b/pkg/agent/context_cache_test.go @@ -383,6 +383,128 @@ Updated content.` } } +// TestGlobalSkillFileContentChange verifies that modifying a global skill +// (~/.picoclaw/skills) invalidates the cached system prompt. +func TestGlobalSkillFileContentChange(t *testing.T) { + tmpHome := t.TempDir() + t.Setenv("HOME", tmpHome) + + tmpDir := setupWorkspace(t, nil) + defer os.RemoveAll(tmpDir) + + globalSkillPath := filepath.Join(tmpHome, ".picoclaw", "skills", "global-skill", "SKILL.md") + if err := os.MkdirAll(filepath.Dir(globalSkillPath), 0o755); err != nil { + t.Fatal(err) + } + v1 := `--- +name: global-skill +description: global-v1 +--- +# Global Skill v1` + if err := os.WriteFile(globalSkillPath, []byte(v1), 0o644); err != nil { + t.Fatal(err) + } + + cb := NewContextBuilder(tmpDir) + sp1 := cb.BuildSystemPromptWithCache() + if !strings.Contains(sp1, "global-v1") { + t.Fatal("expected initial prompt to contain global skill description") + } + + v2 := `--- +name: global-skill +description: global-v2 +--- +# Global Skill v2` + if err := os.WriteFile(globalSkillPath, []byte(v2), 0o644); err != nil { + t.Fatal(err) + } + future := time.Now().Add(2 * time.Second) + os.Chtimes(globalSkillPath, future, future) + + cb.systemPromptMutex.RLock() + changed := cb.sourceFilesChangedLocked() + cb.systemPromptMutex.RUnlock() + if !changed { + t.Fatal("sourceFilesChangedLocked() should detect global skill file content change") + } + + sp2 := cb.BuildSystemPromptWithCache() + if !strings.Contains(sp2, "global-v2") { + t.Error("rebuilt prompt should contain updated global skill description") + } + if sp1 == sp2 { + t.Error("cache should be invalidated when global skill file content changes") + } +} + +// TestBuiltinSkillFileContentChange verifies that modifying a builtin skill +// ({cwd}/skills) invalidates the cached system prompt. +func TestBuiltinSkillFileContentChange(t *testing.T) { + tmpHome := t.TempDir() + t.Setenv("HOME", tmpHome) + + tmpDir := setupWorkspace(t, nil) + defer os.RemoveAll(tmpDir) + + builtinRoot := t.TempDir() + oldWD, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + if err := os.Chdir(builtinRoot); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { + _ = os.Chdir(oldWD) + }) + + builtinSkillPath := filepath.Join(builtinRoot, "skills", "builtin-skill", "SKILL.md") + if err := os.MkdirAll(filepath.Dir(builtinSkillPath), 0o755); err != nil { + t.Fatal(err) + } + v1 := `--- +name: builtin-skill +description: builtin-v1 +--- +# Builtin Skill v1` + if err := os.WriteFile(builtinSkillPath, []byte(v1), 0o644); err != nil { + t.Fatal(err) + } + + cb := NewContextBuilder(tmpDir) + sp1 := cb.BuildSystemPromptWithCache() + if !strings.Contains(sp1, "builtin-v1") { + t.Fatal("expected initial prompt to contain builtin skill description") + } + + v2 := `--- +name: builtin-skill +description: builtin-v2 +--- +# Builtin Skill v2` + if err := os.WriteFile(builtinSkillPath, []byte(v2), 0o644); err != nil { + t.Fatal(err) + } + future := time.Now().Add(2 * time.Second) + os.Chtimes(builtinSkillPath, future, future) + + cb.systemPromptMutex.RLock() + changed := cb.sourceFilesChangedLocked() + cb.systemPromptMutex.RUnlock() + if !changed { + t.Fatal("sourceFilesChangedLocked() should detect builtin skill file content change") + } + + sp2 := cb.BuildSystemPromptWithCache() + if !strings.Contains(sp2, "builtin-v2") { + t.Error("rebuilt prompt should contain updated builtin skill description") + } + if sp1 == sp2 { + t.Error("cache should be invalidated when builtin skill file content changes") + } +} + // TestConcurrentBuildSystemPromptWithCache verifies that multiple goroutines // can safely call BuildSystemPromptWithCache concurrently without producing // empty results, panics, or data races. diff --git a/pkg/skills/loader.go b/pkg/skills/loader.go index 67d3e70e0..edb20d2c0 100644 --- a/pkg/skills/loader.go +++ b/pkg/skills/loader.go @@ -64,6 +64,28 @@ type SkillsLoader struct { builtinSkills string // builtin skills } +// SkillRoots returns all unique skill root directories used by this loader. +// The order follows resolution priority: workspace > global > builtin. +func (sl *SkillsLoader) SkillRoots() []string { + roots := []string{sl.workspaceSkills, sl.globalSkills, sl.builtinSkills} + seen := make(map[string]struct{}, len(roots)) + out := make([]string, 0, len(roots)) + + for _, root := range roots { + if strings.TrimSpace(root) == "" { + continue + } + clean := filepath.Clean(root) + if _, ok := seen[clean]; ok { + continue + } + seen[clean] = struct{}{} + out = append(out, clean) + } + + return out +} + func NewSkillsLoader(workspace string, globalSkills string, builtinSkills string) *SkillsLoader { return &SkillsLoader{ workspace: workspace,