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

[pikaxinge]

Summary

• align system-prompt cache invalidation scope with actual skills summary sources (workspace/global/builtin)
• add SkillRoots() in SkillsLoader as the single source of truth for tracked skill roots
• add regression tests for global and builtin skill file changes to ensure cached prompt rebuilds correctly

Root Cause

BuildSkillsSummary() reads from workspace/global/builtin skill roots, but cache invalidation only tracked workspace files and workspace/skills. This could keep serving stale cached system prompts when global or builtin skills changed.

Changes

• pkg/skills/loader.go
  • add SkillRoots() to expose unique effective skill roots in priority order
• pkg/agent/context.go
  • track all skill roots in cache baseline (existedAtCache + maxMtime)
  • check all skill roots for creation/deletion/mtime/content changes in sourceFilesChangedLocked()
• pkg/agent/context_cache_test.go
  • add TestGlobalSkillFileContentChange
  • add TestBuiltinSkillFileContentChange

Test Plan

• go test ./pkg/agent -run 'Test(GlobalSkillFileContentChange|BuiltinSkillFileContentChange)'
• go test ./pkg/agent ./pkg/skills
• go test ./...

[Copilot]

Pull request overview

This PR fixes a cache invalidation bug where the system prompt cache was not being invalidated when global or builtin skills were modified. Previously, only workspace skills were being tracked for cache invalidation, causing stale cached prompts to be served when skills from other locations changed.

Changes:

• Added SkillRoots() method to expose all unique skill root directories in priority order (workspace > global > builtin)
• Updated cache baseline tracking and invalidation logic to monitor all skill roots instead of just workspace skills
• Added comprehensive regression tests for global and builtin skill file changes

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
pkg/skills/loader.go	Added SkillRoots() method to return deduplicated skill roots in priority order, serving as single source of truth
pkg/agent/context.go	Updated skillRoots(), buildCacheBaseline(), and sourceFilesChangedLocked() to track and check all skill roots for changes
pkg/agent/context_cache_test.go	Added TestGlobalSkillFileContentChange and TestBuiltinSkillFileContentChange to verify cache invalidation works for all skill sources

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[nikolasdehor]

Clean fix for a real cache staleness bug. The issue is straightforward: BuildSkillsSummary() reads from workspace, global (~/.picoclaw/skills), and builtin (cwd/skills) skill roots, but cache invalidation only tracked the workspace skills directory. Editing a global or builtin skill would not invalidate the cached system prompt, serving stale content until a restart.

The fix:

1. SkillRoots() in loader.go exposes all skill root directories with deduplication
2. skillRoots() in context.go delegates to the loader (with fallback to workspace/skills if no loader)
3. buildCacheBaseline() and sourceFilesChangedLocked() now iterate all roots instead of just the workspace

Code quality observations:

• The deduplication in SkillRoots() via filepath.Clean + seen-set handles the case where workspace == builtin (when running from the workspace directory). Good.
• The empty-string guard (strings.TrimSpace(root) == "") prevents walking from the filesystem root.
• Tests cover both global and builtin skill modifications with mtime manipulation to force cache detection.

One potential issue (non-blocking): The builtin test (TestBuiltinSkillFileContentChange) changes os.Chdir() to set the builtin root, and uses t.Cleanup to restore. This is fine for the test, but os.Chdir affects the entire process, which could interfere with parallel tests. Consider using t.Setenv("PICOCLAW_BUILTIN_SKILLS", builtinRoot) if the loader supports it, or adding t.Parallel() exclusion.

LGTM.

[pikaxinge]

Updated based on review follow-up:

• Removed os.Chdir() usage from TestBuiltinSkillFileContentChange to avoid process-wide working-directory side effects during tests.
• Added PICOCLAW_BUILTIN_SKILLS override support in NewContextBuilder and switched the builtin-skill test to use env-based path injection.

Verification:

• go test ./pkg/agent -run 'Test(GlobalSkillFileContentChange|BuiltinSkillFileContentChange)' -count=1
• go test ./pkg/agent ./pkg/skills
• go test ./...

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[pikaxinge]

Hi @lxowalle and @yinwm, could you please take a look at this PR when you have time?

This PR fixes stale system-prompt cache invalidation for skills.
Previously, cache invalidation only tracked workspace skills, while BuildSkillsSummary() also reads global (~/.picoclaw/skills) and builtin (./skills) roots. So edits under global/builtin skills could leave cached prompt content stale.

What changed:

• Added SkillRoots() in loader as the single source of truth (workspace/global/builtin with dedup).
• Updated context cache baseline and change detection to iterate all skill roots.
• Added regression tests for global and builtin skill changes.
• Follow-up review fix: removed os.Chdir() in tests and switched builtin test setup to env-based path injection via PICOCLAW_BUILTIN_SKILLS.

Verification:

• go test ./pkg/agent -run 'Test(GlobalSkillFileContentChange|BuiltinSkillFileContentChange)' -count=1
• go test ./pkg/agent ./pkg/skills
• go test ./...

Thanks for your time and feedback.