diff --git a/pkg/codingcontext/context.go b/pkg/codingcontext/context.go index 3e24192..859e624 100644 --- a/pkg/codingcontext/context.go +++ b/pkg/codingcontext/context.go @@ -210,7 +210,7 @@ func (cc *Context) Run(ctx context.Context, taskName string) (*Result, error) { } // Discover skills (load metadata only for progressive disclosure) - if err := cc.discoverSkills(); err != nil { + if err := cc.discoverSkills(ctx); err != nil { return nil, fmt.Errorf("failed to discover skills: %w", err) } @@ -1026,7 +1026,7 @@ func (cc *Context) runBootstrapScript(ctx context.Context, path string, frontmat // discoverSkills searches for skill directories and loads only their metadata (name and description) // for progressive disclosure. Skills are folders containing a SKILL.md file. -func (cc *Context) discoverSkills() error { +func (cc *Context) discoverSkills(ctx context.Context) error { // Skip skill discovery if bootstrap is disabled if !cc.doBootstrap { return nil @@ -1059,8 +1059,10 @@ func (cc *Context) discoverSkills() error { } } + cache := skillCacheFromContext(ctx) + for _, dir := range skillPaths { - if err := cc.discoverSkillsInDir(dir.path, dir.lenient); err != nil { + if err := cc.discoverSkillsInDir(dir.path, dir.lenient, cache); err != nil { return err } } @@ -1068,8 +1070,9 @@ func (cc *Context) discoverSkills() error { return nil } -// discoverSkillsInDir discovers skills within a single directory. -func (cc *Context) discoverSkillsInDir(dir string, lenient bool) error { +// discoverSkillsInDir discovers skills within a single directory. cache may +// be nil (no caching, parse every call) — see skillCacheFromContext. +func (cc *Context) discoverSkillsInDir(dir string, lenient bool, cache *SkillCache) error { if _, err := os.Stat(dir); os.IsNotExist(err) { return nil } else if err != nil { @@ -1100,7 +1103,7 @@ func (cc *Context) discoverSkillsInDir(dir string, lenient bool) error { skillFile := filepath.Join(dir, entry.Name(), "SKILL.md") - if err := cc.loadSkillEntry(skillFile, lenient); err != nil { + if err := cc.loadSkillEntry(skillFile, lenient, cache); err != nil { return err } } @@ -1109,7 +1112,8 @@ func (cc *Context) discoverSkillsInDir(dir string, lenient bool) error { } // loadSkillEntry loads and validates a single skill from its SKILL.md file. -func (cc *Context) loadSkillEntry(skillFile string, lenient bool) error { +// cache may be nil — see skillCacheFromContext. +func (cc *Context) loadSkillEntry(skillFile string, lenient bool, cache *SkillCache) error { if _, err := os.Stat(skillFile); os.IsNotExist(err) { return nil } else if err != nil { @@ -1122,16 +1126,41 @@ func (cc *Context) loadSkillEntry(skillFile string, lenient bool) error { return fmt.Errorf("failed to stat skill file %s: %w", skillFile, err) } - var frontmatter markdown.SkillFrontMatter + var ( + frontmatter markdown.SkillFrontMatter + parseErr error + ) + + if cached, ok := cache.get(skillFile); ok { + frontmatter, parseErr = cached.frontmatter, cached.err + } else { + _, parseErr = markdown.ParseMarkdownFileWithLogger(skillFile, &frontmatter, cc.logger) + + // Cache only deterministic outcomes. A parse error means the file was + // read and its content rejected, so every later call would reject it + // identically; an I/O error (permissions, fd exhaustion, a flaky + // network filesystem, a file that vanished mid-walk) says nothing + // about the next attempt, and caching one would let a single transient + // blip poison the rest of the run. + if parseErr == nil { + cache.set(skillFile, skillParseResult{frontmatter: frontmatter}) + } else if isDeterministicParseError(parseErr) { + // Discard frontmatter on error: parsing populates it field by + // field, so a failed parse can leave it partly filled. Only err is + // consulted while it is non-nil, but caching a half-built struct + // invites a later reader to trust it. + cache.set(skillFile, skillParseResult{err: parseErr}) + } + } - if _, err := markdown.ParseMarkdownFileWithLogger(skillFile, &frontmatter, cc.logger); err != nil { + if parseErr != nil { if lenient { - cc.logger.Warn("skipping skill file: failed to parse YAML frontmatter", "path", skillFile, "error", err) + cc.logger.Warn("skipping skill file: failed to parse YAML frontmatter", "path", skillFile, "error", parseErr) return nil } - return fmt.Errorf("failed to parse skill file %s: %w", skillFile, err) + return fmt.Errorf("failed to parse skill file %s: %w", skillFile, parseErr) } if cc.lintCollector != nil { diff --git a/pkg/codingcontext/context_edge_cases_test.go b/pkg/codingcontext/context_edge_cases_test.go index 5a73d81..dd7a3ba 100644 --- a/pkg/codingcontext/context_edge_cases_test.go +++ b/pkg/codingcontext/context_edge_cases_test.go @@ -227,6 +227,143 @@ func TestSkillSelectorFiltering(t *testing.T) { } } +// TestSkillCacheSharedAcrossContexts verifies that a SkillCache attached via +// NewContextWithSkillCache is actually consulted: a second *Context, sharing +// the same ctx (so the same cache) as the first, gets back a skill parsed by +// the first call even after the underlying file changes on disk. This is the +// intended usage: codegen-runner's lint-all-tasks loop constructs a new +// *Context per task, and shares one SkillCache across all of them via ctx to +// avoid re-parsing an unchanged skill tree once per task. +func TestSkillCacheSharedAcrossContexts(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + createTask(t, dir, "task", "", "Task content") + + skillPath := filepath.Join(dir, ".agents", "skills", "cached-skill", "SKILL.md") + createSkill(t, dir, ".agents/skills/cached-skill", + "---\nname: cached-skill\ndescription: Original description.\n---\n") + + cache := NewSkillCache() + ctx := NewContextWithSkillCache(context.Background(), cache) + + // First Context/Run populates the cache. + c1 := New(WithSearchPaths(dir)) + + result1, err := c1.Run(ctx, "task") + if err != nil { + t.Fatalf("first Run() error: %v", err) + } + + if len(result1.Skills.Skills) != 1 || result1.Skills.Skills[0].Description != "Original description." { + t.Fatalf("first Run() skills = %v, want one skill with original description", result1.Skills.Skills) + } + + // Change the file on disk. A second Context sharing the same ctx (and so + // the same cache) should still see the cached (original) result, proving + // the cache — not a fresh parse — served the second call. + if err := os.WriteFile(skillPath, + []byte("---\nname: cached-skill\ndescription: Changed after caching.\n---\n"), 0o600); err != nil { + t.Fatalf("failed to rewrite skill file: %v", err) + } + + c2 := New(WithSearchPaths(dir)) + + result2, err := c2.Run(ctx, "task") + if err != nil { + t.Fatalf("second Run() error: %v", err) + } + + if len(result2.Skills.Skills) != 1 || result2.Skills.Skills[0].Description != "Original description." { + t.Fatalf("second Run() skills = %v, want cached original description (cache was not consulted)", + result2.Skills.Skills) + } +} + +// TestSkillCacheCachesParseErrors verifies that a parse failure (not a +// missing-file case, which is handled separately) is itself cached: a second +// Context sharing the same cache must get back the same error rather than +// silently succeeding because only the happy path was memoized. +func TestSkillCacheCachesParseErrors(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + createTask(t, dir, "task", "", "Task content") + + skillPath := filepath.Join(dir, ".agents", "skills", "bad-skill", "SKILL.md") + createSkill(t, dir, ".agents/skills/bad-skill", "---\nname: [unclosed\n---\nBody.\n") + + cache := NewSkillCache() + ctx := NewContextWithSkillCache(context.Background(), cache) + + c1 := New(WithSearchPaths(dir)) + if _, err := c1.Run(ctx, "task"); err == nil { + t.Fatal("first Run() expected an error for malformed skill frontmatter, got nil") + } + + // Fixing the file on disk should NOT change the outcome for a second + // Context sharing the same cache: the cached error must still be served. + if err := os.WriteFile(skillPath, + []byte("---\nname: bad-skill\ndescription: Now valid.\n---\n"), 0o600); err != nil { + t.Fatalf("failed to rewrite skill file: %v", err) + } + + c2 := New(WithSearchPaths(dir)) + if _, err := c2.Run(ctx, "task"); err == nil { + t.Fatal("second Run() expected the cached parse error to be reused, got nil") + } +} + +// TestSkillCacheDoesNotCacheTransientIOErrors verifies the counterpart to +// TestSkillCacheCachesParseErrors: an error that describes one I/O attempt +// rather than the file's content must NOT be memoized. Caching one would let +// a single unreadable moment early in a run persist for every later caller, +// which is strictly worse than not caching at all. +func TestSkillCacheDoesNotCacheTransientIOErrors(t *testing.T) { + t.Parallel() + + if os.Geteuid() == 0 { + t.Skip("running as root: permission bits do not deny reads") + } + + dir := t.TempDir() + createTask(t, dir, "task", "", "Task content") + + skillPath := filepath.Join(dir, ".agents", "skills", "locked-skill", "SKILL.md") + createSkill(t, dir, ".agents/skills/locked-skill", + "---\nname: locked-skill\ndescription: Readable once permissions allow.\n---\n") + + // Make the file unreadable so the first pass fails on I/O, not on content. + if err := os.Chmod(skillPath, 0o000); err != nil { + t.Fatalf("failed to chmod skill file: %v", err) + } + + cache := NewSkillCache() + ctx := NewContextWithSkillCache(context.Background(), cache) + + c1 := New(WithSearchPaths(dir)) + if _, err := c1.Run(ctx, "task"); err == nil { + t.Fatal("first Run() expected an error for an unreadable skill file, got nil") + } + + // Restore access. A second Context sharing the cache must retry the read + // and succeed, rather than being served the earlier permission error. + if err := os.Chmod(skillPath, 0o600); err != nil { + t.Fatalf("failed to restore skill file permissions: %v", err) + } + + c2 := New(WithSearchPaths(dir)) + + result, err := c2.Run(ctx, "task") + if err != nil { + t.Fatalf("second Run() expected the transient error to be retried, got: %v", err) + } + + if !strings.Contains(result.Prompt, "Readable once permissions allow.") { + t.Error("second Run() did not pick up the now-readable skill") + } +} + // TestMergeSelectorsIntegerYAMLValue verifies that task frontmatter selectors whose // YAML values parse as integers (not strings) still match rule frontmatter correctly. // The mergeSelectors function uses fmt.Sprint, and MatchesIncludes does the same; diff --git a/pkg/codingcontext/markdown/frontmatter.go b/pkg/codingcontext/markdown/frontmatter.go index a723582..6dfe3bb 100644 --- a/pkg/codingcontext/markdown/frontmatter.go +++ b/pkg/codingcontext/markdown/frontmatter.go @@ -22,6 +22,15 @@ type BaseFrontMatter struct { // With yaml:",inline", goccy/go-yaml populates this map with all unknown keys, // while known fields on the embedding struct (e.g. TaskNames, License) are set // directly on those fields. This ensures outer-struct fields are not shadowed. + // + // Treat this map as read-only after parsing. Parsed frontmatter is memoized + // and shared across Context instances (see SkillCache), and copying the + // struct copies only the map header — so a write here would be visible to + // every other holder of the same cached parse. Nothing writes to it today, + // and that sharing is only safe for as long as that stays true. Callers in + // the lint-all-tasks path run sequentially, so such a write would surface as + // cross-task contamination rather than a race the detector would flag. To + // vary a value per-consumer, copy the map instead of mutating in place. Content map[string]any `json:"-" yaml:",inline"` } @@ -157,6 +166,9 @@ type SkillFrontMatter struct { Compatibility string `json:"compatibility,omitempty" yaml:"compatibility,omitempty"` // Metadata contains arbitrary key-value pairs (optional) + // + // Read-only after parsing, for the same reason as BaseFrontMatter.Content: + // this map is shared by every consumer of a cached skill parse. Metadata map[string]string `json:"metadata,omitempty" yaml:"metadata,omitempty"` // AllowedTools is a space-delimited list of pre-approved tools (optional, experimental) diff --git a/pkg/codingcontext/skillcache.go b/pkg/codingcontext/skillcache.go new file mode 100644 index 0000000..9030f2e --- /dev/null +++ b/pkg/codingcontext/skillcache.go @@ -0,0 +1,124 @@ +package codingcontext + +import ( + "context" + "errors" + "sync" + + "github.com/kitproj/coding-context-cli/pkg/codingcontext/markdown" +) + +// skillParseResult is the cached outcome of parsing a single SKILL.md file: +// either the parsed frontmatter, or the error that rejected it. Exactly one +// of the two is populated, matched by err == nil — on error the frontmatter +// is left zero rather than stored half-built, since parsing fills it field by +// field and can fail partway. +// +// Only deterministic outcomes reach the cache; see isDeterministicParseError. +// +// This is captured immediately after the parse call in loadSkillEntry — +// before validateAndAddSkill's lenient-mode name inference (which derives a +// name from skillFile's directory when frontmatter.Name is empty) — so what +// gets cached is the raw, mode-independent parse result. lenient does not +// affect ParseMarkdownFileWithLogger itself (it takes no such argument), so +// the same skillFile always parses to the same result regardless of which +// call's lenient value happens to populate the cache first. +type skillParseResult struct { + frontmatter markdown.SkillFrontMatter + err error +} + +// SkillCache memoizes SKILL.md parse results across repeated skill discovery +// runs, keyed by file path. Safe for concurrent use. +// +// discoverSkills (via New/Run/Lint) reruns a full parse of every skill file +// on every call — and codegen-runner's lint-all-tasks loop constructs a new +// *Context (and so triggers a fresh discoverSkills) once per task, re-parsing +// the same, unchanged skill tree from scratch each time. A SkillCache shared +// across those Context instances, via context.Context (see +// NewContextWithSkillCache), turns that into "parse once across the whole +// run" without changing any single Context's own behavior. +// +// Frontmatter values are safe to share this way: markdown.SkillFrontMatter's +// only reference fields (Content, Metadata maps) are read-only in every +// production code path once loadSkillEntry returns them — the maps are never +// written after that point, so sharing the map underneath a value-copied +// struct across goroutines/tasks is safe without a deep copy. +type SkillCache struct { + mu sync.RWMutex + results map[string]skillParseResult +} + +// NewSkillCache returns an empty, ready-to-use SkillCache. +func NewSkillCache() *SkillCache { + return &SkillCache{results: make(map[string]skillParseResult)} +} + +// get returns the cached parse result for skillFile, if any. +func (c *SkillCache) get(skillFile string) (skillParseResult, bool) { + if c == nil { + return skillParseResult{}, false + } + + c.mu.RLock() + defer c.mu.RUnlock() + + result, ok := c.results[skillFile] + + return result, ok +} + +// set stores the parse result for skillFile, replacing any existing entry. +func (c *SkillCache) set(skillFile string, result skillParseResult) { + if c == nil { + return + } + + c.mu.Lock() + defer c.mu.Unlock() + + c.results[skillFile] = result +} + +// isDeterministicParseError reports whether err is a verdict on a file's +// content rather than a report about the I/O that tried to reach it. +// +// A *markdown.ParseError means the bytes were read and rejected, so re-reading +// the same file yields the same verdict and the result is safe to memoize. +// Everything else — permission denied, too many open files, a network +// filesystem hiccup, a file that vanished mid-walk — describes one attempt, +// not the file, and re-reading may well succeed. Caching those would let a +// single transient failure early in a run persist for every later caller. +func isDeterministicParseError(err error) bool { + var parseErr *markdown.ParseError + + return errors.As(err, &parseErr) +} + +// skillCacheContextKey is an unexported type so this package's context key +// cannot collide with keys from other packages using the same value. +type skillCacheContextKey struct{} + +// NewContextWithSkillCache returns a child context carrying cache for +// discoverSkills to use. Callers that construct multiple *Context instances +// against the same skill tree (e.g. once per task in a lint-all loop) should +// create one SkillCache with NewSkillCache, attach it once via this +// function, and pass the resulting context.Context into every Run/Lint call +// to share parsed skill files across all of them. +// +// Passing a context without an attached SkillCache (e.g. context.Background() +// unmodified) is fully supported: discoverSkills falls back to parsing every +// call, exactly as before this cache existed. +func NewContextWithSkillCache(ctx context.Context, cache *SkillCache) context.Context { + return context.WithValue(ctx, skillCacheContextKey{}, cache) +} + +// skillCacheFromContext extracts the SkillCache attached via +// NewContextWithSkillCache, or nil if none was attached. All SkillCache +// methods are nil-receiver-safe (get returns !ok, set is a no-op), so callers +// never need to nil-check the result themselves. +func skillCacheFromContext(ctx context.Context) *SkillCache { + cache, _ := ctx.Value(skillCacheContextKey{}).(*SkillCache) + + return cache +} diff --git a/pkg/codingcontext/taskparser/grammar.go b/pkg/codingcontext/taskparser/grammar.go index fec37a3..018e4d4 100644 --- a/pkg/codingcontext/taskparser/grammar.go +++ b/pkg/codingcontext/taskparser/grammar.go @@ -1,6 +1,8 @@ package taskparser import ( + "sync" + "github.com/alecthomas/participle/v2" "github.com/alecthomas/participle/v2/lexer" ) @@ -50,7 +52,22 @@ type TextLine struct { NewlineOpt string `parser:"@Newline?"` } -func parser() *participle.Parser[Input] { +// taskParser builds the task grammar/lexer once, on first use, and returns +// the same *Parser[Input] on every subsequent call. This is an effectively +// immutable package-level value — like a compiled regexp.MustCompile result +// — not a mutable cache: sync.OnceValue's returned func always yields the +// identical value after its first call, and taskParser itself is never +// reassigned. participle.Parser[T] holds only the compiled grammar and +// lexer definition (fixed, derived solely from the Input struct tags above); +// all per-parse mutable state lives in a parseContext created fresh inside +// ParseString/ParseFromLexer on every call, so sharing one *Parser[Input] +// across calls (including concurrently, confirmed via -race) is the +// standard, documented participle usage pattern. Rebuilding it on every +// call (the previous behavior) repeated the grammar-compilation work for no +// benefit, since the grammar never varies — allocation-heavy per the +// participle internals (Branch/Defer/lexer.Upgrade/newTagLexer), confirmed +// via benchmark to be ~40% of both time and allocations per parse call. +var taskParser = sync.OnceValue(func() *participle.Parser[Input] { taskLexer := lexer.MustSimple([]lexer.SimpleRule{ {Name: "Slash", Pattern: `/`}, // Any "/" {Name: "Assign", Pattern: `=`}, // "=" @@ -66,6 +83,10 @@ func parser() *participle.Parser[Input] { participle.Lexer(taskLexer), participle.UseLookahead(taskLookahead), ) +}) + +func parser() *participle.Parser[Input] { + return taskParser() } // ========== PARAMS GRAMMAR ========== @@ -107,7 +128,10 @@ type Value struct { Raw string `parser:"(@QuotedDouble | @QuotedSingle | @Token)"` } -func paramsParser() *participle.Parser[ParamsInput] { +// paramsParserOnce builds the params grammar/lexer once and reuses it across +// every call to paramsParser(). See taskParser above for why an effectively +// immutable sync.OnceValue package var is safe here. +var paramsParserOnce = sync.OnceValue(func() *participle.Parser[ParamsInput] { paramsLexer := lexer.MustSimple([]lexer.SimpleRule{ {Name: "Whitespace", Pattern: `[\s\p{Z}]+`}, // Match ASCII and Unicode whitespace {Name: "Comma", Pattern: `,`}, @@ -127,4 +151,8 @@ func paramsParser() *participle.Parser[ParamsInput] { participle.Lexer(paramsLexer), participle.UseLookahead(paramsLookahead), ) +}) + +func paramsParser() *participle.Parser[ParamsInput] { + return paramsParserOnce() }