perf(core): cache skills loading with fsnotify invalidation - #121
Merged
Conversation
LoadSkills was called on every prompt, walking up to 4 directory trees, reading every SKILL.md, and parsing YAML frontmatter. With 100+ skills this adds 10-50ms of filesystem I/O per prompt. Add a SkillsCache backed by samber/hot (W-TinyLFU, 1h TTL) with a background fsnotify watcher that debounces burst events (500ms) before purging. Cache misses are deduplicated via singleflight. Cache is CWD-scoped, lifecycle-managed through a SkillsService DI registration, and gracefully degrades to direct disk loads if fsnotify is unavailable or after Close. - Add internal/core/skills_cache.go and tests (7 cases) - Add internal/di/skills_service.go with Shutdown lifecycle - Wire SkillsCache into Runtime.loadSkills() used by context_build, context_auto_compaction, and slash commands
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughIntroduces ChangesSkills Cache, Test Infrastructure, DI Wiring, and Runtime Integration
Sequence DiagramsequenceDiagram
participant AssistantRuntime
participant loadSkills
participant SkillsCache
participant HotCache
participant core_LoadSkills as core.LoadSkills
AssistantRuntime->>loadSkills: loadSkills(cwd)
alt skillsCache is set
loadSkills->>SkillsCache: Get(cwd)
SkillsCache->>HotCache: fetch(cwd)
alt cache hit
HotCache-->>SkillsCache: cached LoadSkillsResult
else cache miss
HotCache->>core_LoadSkills: LoadSkills(cwd)
core_LoadSkills-->>HotCache: LoadSkillsResult
HotCache->>SkillsCache: register watches via watchSkillDirs
end
SkillsCache-->>loadSkills: LoadSkillsResult.Skills
else skillsCache is nil (test or fallback)
loadSkills->>core_LoadSkills: LoadSkills(cwd, nil, true)
core_LoadSkills-->>loadSkills: LoadSkillsResult.Skills
end
loadSkills-->>AssistantRuntime: []Skill
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #121 +/- ##
==========================================
+ Coverage 78.40% 78.41% +0.01%
==========================================
Files 282 285 +3
Lines 22390 22530 +140
==========================================
+ Hits 17555 17668 +113
- Misses 3586 3605 +19
- Partials 1249 1257 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/core/skills_cache.go`:
- Around line 182-191: The watch registration for default skill paths is skipped
when directories don't exist initially, causing cache invalidation to fail if
those directories are created later. In the loop that processes
defaultSkillPaths with filepath.Clean, remove the condition that skips watch
registration based on resourcePathExists, and instead register the watch for all
paths regardless of their current existence. Keep the check for already-watched
directories to avoid duplicates, but ensure that every path returned by
defaultSkillPaths gets added to the watched map and has c.addWatch called on it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 40e82c9f-e417-4bd4-998c-17356d866d09
📒 Files selected for processing (20)
internal/assistant/context_auto_compaction_internal_test.gointernal/assistant/context_auto_compaction_test.gointernal/assistant/context_budget_test.gointernal/assistant/context_build.gointernal/assistant/context_compaction_lifecycle_test.gointernal/assistant/context_compaction_test.gointernal/assistant/provider_hooks_internal_test.gointernal/assistant/runtime.gointernal/assistant/runtime_slash.gointernal/assistant/runtime_test.gointernal/assistant/tool_executor_internal_test.gointernal/core/skills_cache.gointernal/core/skills_cache_test.gointernal/di/assistant_service.gointernal/di/assistant_service_internal_test.gointernal/di/container.gointernal/di/register.gointernal/di/skills_service.gointernal/terminal/prompt_send_internal_test.gointernal/terminal/render_parity_internal_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/assistant/testing.go (1)
13-22: ⚡ Quick winExpose
SkillsCacheinRuntimeTestOptionsto keep test wiring aligned withRuntimeOptions.
NewRuntimeForTestcurrently hardcodesSkillsCacheto nil, which prevents tests from explicitly exercising cache-enabled runtime paths. Add it to the helper options and forward it.♻️ Proposed change
import ( "log/slog" "github.com/omarluq/librecode/internal/config" + "github.com/omarluq/librecode/internal/core" "github.com/omarluq/librecode/internal/database" "github.com/omarluq/librecode/internal/event" "github.com/omarluq/librecode/internal/model" ) @@ type RuntimeTestOptions struct { Config *config.Config Sessions *database.SessionRepository Extensions runtimeExtensions Cache *ResponseCache Events *event.Bus Models *model.Registry Client Completer Logger *slog.Logger + SkillsCache *core.SkillsCache } @@ return NewRuntime(&RuntimeOptions{ Config: opts.Config, Sessions: opts.Sessions, Extensions: opts.Extensions, Cache: opts.Cache, Events: opts.Events, Models: opts.Models, Client: opts.Client, Logger: opts.Logger, - SkillsCache: nil, + SkillsCache: opts.SkillsCache, }) }Also applies to: 43-53
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/assistant/testing.go` around lines 13 - 22, Add a SkillsCache field to the RuntimeTestOptions struct (at lines 13-22) to match the runtime configuration options. Then update the NewRuntimeForTest function (at lines 43-53) to accept the SkillsCache value from RuntimeTestOptions and forward it when creating the runtime instance, instead of hardcoding it to nil, so that tests can explicitly exercise cache-enabled runtime paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/assistant/testing.go`:
- Around line 13-22: Add a SkillsCache field to the RuntimeTestOptions struct
(at lines 13-22) to match the runtime configuration options. Then update the
NewRuntimeForTest function (at lines 43-53) to accept the SkillsCache value from
RuntimeTestOptions and forward it when creating the runtime instance, instead of
hardcoding it to nil, so that tests can explicitly exercise cache-enabled
runtime paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ff98c89b-7559-4159-98c4-e78cce30408d
📒 Files selected for processing (14)
internal/assistant/context_auto_compaction_internal_test.gointernal/assistant/context_auto_compaction_test.gointernal/assistant/context_budget_test.gointernal/assistant/context_compaction_lifecycle_test.gointernal/assistant/context_compaction_test.gointernal/assistant/provider_hook_test_helpers_internal_test.gointernal/assistant/provider_hooks_internal_test.gointernal/assistant/runtime_test.gointernal/assistant/testing.gointernal/assistant/tool_executor_internal_test.gointernal/core/skills_cache.gointernal/core/skills_cache_test.gointernal/terminal/prompt_send_internal_test.gointernal/terminal/render_parity_internal_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/core/skills_cache_test.go
…gh NewRuntimeForTest
|
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
LoadSkills was called on every prompt, walking up to 4 directory trees,
reading every SKILL.md, and parsing YAML frontmatter. With 100+ skills
this adds 10-50ms of filesystem I/O per prompt. Add a SkillsCache backed
by samber/hot (W-TinyLFU, 1h TTL) with a background fsnotify watcher
that debounces burst events (500ms) before purging. Cache misses are
deduplicated via singleflight. Cache is CWD-scoped, lifecycle-managed
through a SkillsService DI registration, and gracefully degrades to
direct disk loads if fsnotify is unavailable or after Close. - Add
internal/core/skills_cache.go and tests (7 cases) - Add
internal/di/skills_service.go with Shutdown lifecycle - Wire SkillsCache
into Runtime.loadSkills() used by context_build,
context_auto_compaction, and slash commands