diff --git a/README.md b/README.md index b040d0605..1ef60a283 100644 --- a/README.md +++ b/README.md @@ -642,6 +642,20 @@ PicoClaw stores data in your configured workspace (default: `~/.picoclaw/workspa └── USER.md # User preferences ``` +### Skill Sources + +By default, skills are loaded from: + +1. `~/.picoclaw/workspace/skills` (workspace) +2. `~/.picoclaw/skills` (global) +3. `/skills` (builtin) + +For advanced/test setups, you can override the builtin skills root with: + +```bash +export PICOCLAW_BUILTIN_SKILLS=/path/to/skills +``` + ### 🔒 Security Sandbox PicoClaw runs in a sandboxed environment by default. The agent can only access files and execute commands within the configured workspace. diff --git a/README.zh.md b/README.zh.md index 7c9351cb4..3ea27dea5 100644 --- a/README.zh.md +++ b/README.zh.md @@ -335,6 +335,20 @@ PicoClaw 将数据存储在您配置的工作区中（默认：`~/.picoclaw/work ``` +### 技能来源 (Skill Sources) + +默认情况下，技能会按以下顺序加载： + +1. `~/.picoclaw/workspace/skills`（工作区） +2. `~/.picoclaw/skills`（全局） +3. `/skills`（内置） + +在高级/测试场景下，可通过以下环境变量覆盖内置技能目录： + +```bash +export PICOCLAW_BUILTIN_SKILLS=/path/to/skills +``` + ### 心跳 / 周期性任务 (Heartbeat) PicoClaw 可以自动执行周期性任务。在工作区创建 `HEARTBEAT.md` 文件： diff --git a/pkg/agent/context.go b/pkg/agent/context.go index 17d552e6d..58622e091 100644 --- a/pkg/agent/context.go +++ b/pkg/agent/context.go @@ -33,6 +33,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 { @@ -150,6 +155,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{ @@ -169,6 +175,7 @@ func (cb *ContextBuilder) InvalidateCache() { cb.cachedSystemPrompt = "" cb.cachedAt = time.Time{} cb.existedAtCache = nil + cb.skillFilesAtCache = nil logger.DebugCF("agent", "System prompt cache invalidated", nil) } @@ -203,8 +210,9 @@ func (cb *ContextBuilder) skillRoots() []string { // 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 @@ -217,6 +225,7 @@ func (cb *ContextBuilder) buildCacheBaseline() cacheBaseline { 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 { @@ -227,14 +236,16 @@ func (cb *ContextBuilder) buildCacheBaseline() cacheBaseline { } } - // 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. + // 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 && info.ModTime().After(maxMtime) { - maxMtime = info.ModTime() + if info, err := os.Stat(path); err == nil { + skillFiles[path] = info.ModTime() + if info.ModTime().After(maxMtime) { + maxMtime = info.ModTime() + } } } return nil @@ -251,7 +262,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 @@ -276,15 +287,15 @@ func (cb *ContextBuilder) sourceFilesChangedLocked() bool { // --- Skill roots (workspace/global/builtin) --- // // 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. + // 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 } - if skillFilesModifiedSince(root, cb.cachedAt) { - return true - } + } + if skillFilesChangedSince(cb.skillRoots(), cb.skillFilesAtCache) { + return true } return false @@ -324,28 +335,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 { diff --git a/pkg/agent/context_cache_test.go b/pkg/agent/context_cache_test.go index 4044f34f5..8a8d60fd5 100644 --- a/pkg/agent/context_cache_test.go +++ b/pkg/agent/context_cache_test.go @@ -420,7 +420,9 @@ description: global-v2 t.Fatal(err) } future := time.Now().Add(2 * time.Second) - os.Chtimes(globalSkillPath, future, future) + if err := os.Chtimes(globalSkillPath, future, future); err != nil { + t.Fatalf("failed to update mtime for %s: %v", globalSkillPath, err) + } cb.systemPromptMutex.RLock() changed := cb.sourceFilesChangedLocked() @@ -478,7 +480,9 @@ description: builtin-v2 t.Fatal(err) } future := time.Now().Add(2 * time.Second) - os.Chtimes(builtinSkillPath, future, future) + if err := os.Chtimes(builtinSkillPath, future, future); err != nil { + t.Fatalf("failed to update mtime for %s: %v", builtinSkillPath, err) + } cb.systemPromptMutex.RLock() changed := cb.sourceFilesChangedLocked() @@ -496,6 +500,45 @@ description: builtin-v2 } } +// TestSkillFileDeletionInvalidatesCache verifies that deleting a nested skill +// file invalidates the cached system prompt. +func TestSkillFileDeletionInvalidatesCache(t *testing.T) { + tmpDir := setupWorkspace(t, map[string]string{ + "skills/delete-me/SKILL.md": `--- +name: delete-me +description: delete-me-v1 +--- +# Delete Me`, + }) + defer os.RemoveAll(tmpDir) + + cb := NewContextBuilder(tmpDir) + sp1 := cb.BuildSystemPromptWithCache() + if !strings.Contains(sp1, "delete-me-v1") { + t.Fatal("expected initial prompt to contain skill description") + } + + skillPath := filepath.Join(tmpDir, "skills", "delete-me", "SKILL.md") + if err := os.Remove(skillPath); err != nil { + t.Fatal(err) + } + + cb.systemPromptMutex.RLock() + changed := cb.sourceFilesChangedLocked() + cb.systemPromptMutex.RUnlock() + if !changed { + t.Fatal("sourceFilesChangedLocked() should detect deleted skill file") + } + + sp2 := cb.BuildSystemPromptWithCache() + if strings.Contains(sp2, "delete-me-v1") { + t.Error("rebuilt prompt should not contain deleted skill description") + } + if sp1 == sp2 { + t.Error("cache should be invalidated when skill file is deleted") + } +} + // 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 edb20d2c0..2d4281e79 100644 --- a/pkg/skills/loader.go +++ b/pkg/skills/loader.go @@ -72,10 +72,11 @@ func (sl *SkillsLoader) SkillRoots() []string { out := make([]string, 0, len(roots)) for _, root := range roots { - if strings.TrimSpace(root) == "" { + trimmed := strings.TrimSpace(root) + if trimmed == "" { continue } - clean := filepath.Clean(root) + clean := filepath.Clean(trimmed) if _, ok := seen[clean]; ok { continue } diff --git a/pkg/skills/loader_test.go b/pkg/skills/loader_test.go index 9428bea62..31619f9c2 100644 --- a/pkg/skills/loader_test.go +++ b/pkg/skills/loader_test.go @@ -326,3 +326,19 @@ func TestStripFrontmatter(t *testing.T) { }) } } + +func TestSkillRootsTrimsWhitespaceAndDedups(t *testing.T) { + tmp := t.TempDir() + workspace := filepath.Join(tmp, "workspace") + global := filepath.Join(tmp, "global") + builtin := filepath.Join(tmp, "builtin") + + sl := NewSkillsLoader(workspace, " "+global+" ", "\t"+builtin+"\n") + roots := sl.SkillRoots() + + assert.Equal(t, []string{ + filepath.Join(workspace, "skills"), + global, + builtin, + }, roots) +}