fix(agent): invalidate system prompt cache for global/builtin skills (#845)

* fix(agent): invalidate system prompt cache for global/builtin skills * test(agent): avoid os.Chdir in builtin skill cache test * fix(agent): harden skill cache invalidation checks
2026-06-12 18:08:54 +00:00 · 2026-03-03 18:25:00 +08:00
parent 1265655ef0
commit 3902061db1
6 changed files with 340 additions and 56 deletions
@@ -34,6 +34,11 @@ type ContextBuilder struct {
 	// created (didn't exist at cache time, now exist) or deleted (existed at
 	// cache time, now gone) — both of which should trigger a cache rebuild.
 	existedAtCache map[string]bool
+
+	// skillFilesAtCache snapshots the skill tree file set and mtimes at cache
+	// build time. This catches nested file creations/deletions/mtime changes
+	// that may not update the top-level skill root directory mtime.
+	skillFilesAtCache map[string]time.Time
 }

 func getGlobalConfigDir() string {
@@ -47,8 +52,11 @@ func getGlobalConfigDir() string {
 func NewContextBuilder(workspace string) *ContextBuilder {
 	// builtin skills: skills directory in current project
 	// Use the skills/ directory under the current working directory
-	wd, _ := os.Getwd()
-	builtinSkillsDir := filepath.Join(wd, "skills")
+	builtinSkillsDir := strings.TrimSpace(os.Getenv("PICOCLAW_BUILTIN_SKILLS"))
+	if builtinSkillsDir == "" {
+		wd, _ := os.Getwd()
+		builtinSkillsDir = filepath.Join(wd, "skills")
+	}
 	globalSkillsDir := filepath.Join(getGlobalConfigDir(), "skills")

 	return &ContextBuilder{
@@ -148,6 +156,7 @@ func (cb *ContextBuilder) BuildSystemPromptWithCache() string {
 	cb.cachedSystemPrompt = prompt
 	cb.cachedAt = baseline.maxMtime
 	cb.existedAtCache = baseline.existed
+	cb.skillFilesAtCache = baseline.skillFiles

 	logger.DebugCF("agent", "System prompt cached",
 		map[string]any{
@@ -167,14 +176,14 @@ func (cb *ContextBuilder) InvalidateCache() {
 	cb.cachedSystemPrompt = ""
 	cb.cachedAt = time.Time{}
 	cb.existedAtCache = nil
+	cb.skillFilesAtCache = nil

 	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"),
@@ -185,23 +194,39 @@ 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 {
-	existed  map[string]bool
-	maxMtime time.Time
+	existed    map[string]bool
+	skillFiles map[string]time.Time
+	maxMtime   time.Time
 }

 // buildCacheBaseline records which tracked paths currently exist and computes
 // 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))
+	skillFiles := make(map[string]time.Time)
 	var maxMtime time.Time

 	for _, p := range allPaths {
@@ -212,17 +237,21 @@ func (cb *ContextBuilder) buildCacheBaseline() cacheBaseline {
 		}
 	}

-	// Walk skills files to capture their 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()
+	// Walk all skill roots recursively to snapshot skill files and mtimes.
+	// Use os.Stat (not d.Info) for consistency with sourceFilesChanged checks.
+	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 {
+					skillFiles[path] = info.ModTime()
+					if 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:
@@ -234,7 +263,7 @@ func (cb *ContextBuilder) buildCacheBaseline() cacheBaseline {
 		maxMtime = time.Unix(1, 0)
 	}

-	return cacheBaseline{existed: existed, maxMtime: maxMtime}
+	return cacheBaseline{existed: existed, skillFiles: skillFiles, maxMtime: maxMtime}
 }

 // sourceFilesChangedLocked checks whether any workspace source file has been
@@ -254,21 +283,17 @@ func (cb *ContextBuilder) sourceFilesChangedLocked() bool {
 		return true
 	}

-	// --- 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
+	// For each root:
+	// 1. Creation/deletion and root directory mtime changes are tracked by fileChangedSince.
+	// 2. Nested file create/delete/mtime changes are tracked by the skill file snapshot.
+	for _, root := range cb.skillRoots() {
+		if cb.fileChangedSince(root) {
+			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) {
+	if skillFilesChangedSince(cb.skillRoots(), cb.skillFilesAtCache) {
 		return true
 	}

@@ -309,28 +334,64 @@ func (cb *ContextBuilder) fileChangedSince(path string) bool {
 // if the callback returned nil when its err parameter is non-nil.
 var errWalkStop = errors.New("walk stop")

-// skillFilesModifiedSince recursively walks the skills directory and checks
-// whether any file was modified after t. This catches content-only edits at
-// any nesting depth (e.g. skills/name/docs/extra.md) that don't update
-// parent directory mtimes.
-func skillFilesModifiedSince(skillsDir string, t time.Time) bool {
-	changed := false
-	err := filepath.WalkDir(skillsDir, func(path string, d fs.DirEntry, walkErr error) error {
-		if walkErr == nil && !d.IsDir() {
-			if info, statErr := os.Stat(path); statErr == nil && info.ModTime().After(t) {
-				changed = true
-				return errWalkStop // stop walking
-			}
-		}
-		return nil
-	})
-	// errWalkStop is expected (early exit on first changed file).
-	// os.IsNotExist means the skills dir doesn't exist yet — not an error.
-	// Any other error is unexpected and worth logging.
-	if err != nil && !errors.Is(err, errWalkStop) && !os.IsNotExist(err) {
-		logger.DebugCF("agent", "skills walk error", map[string]any{"error": err.Error()})
+// skillFilesChangedSince compares the current recursive skill file tree
+// against the cache-time snapshot. Any create/delete/mtime drift invalidates
+// the cache.
+func skillFilesChangedSince(skillRoots []string, filesAtCache map[string]time.Time) bool {
+	// Defensive: if the snapshot was never initialized, force rebuild.
+	if filesAtCache == nil {
+		return true
 	}
-	return changed
+
+	// Check cached files still exist and keep the same mtime.
+	for path, cachedMtime := range filesAtCache {
+		info, err := os.Stat(path)
+		if err != nil {
+			// A previously tracked file disappeared (or became inaccessible):
+			// either way, cached skill summary may now be stale.
+			return true
+		}
+		if !info.ModTime().Equal(cachedMtime) {
+			return true
+		}
+	}
+
+	// Check no new files appeared under any skill root.
+	changed := false
+	for _, root := range skillRoots {
+		if strings.TrimSpace(root) == "" {
+			continue
+		}
+
+		err := filepath.WalkDir(root, func(path string, d fs.DirEntry, walkErr error) error {
+			if walkErr != nil {
+				// Treat unexpected walk errors as changed to avoid stale cache.
+				if !os.IsNotExist(walkErr) {
+					changed = true
+					return errWalkStop
+				}
+				return nil
+			}
+			if d.IsDir() {
+				return nil
+			}
+			if _, ok := filesAtCache[path]; !ok {
+				changed = true
+				return errWalkStop
+			}
+			return nil
+		})
+
+		if changed {
+			return true
+		}
+		if err != nil && !errors.Is(err, errWalkStop) && !os.IsNotExist(err) {
+			logger.DebugCF("agent", "skills walk error", map[string]any{"error": err.Error()})
+			return true
+		}
+	}
+
+	return false
 }

 func (cb *ContextBuilder) LoadBootstrapFiles() string {