From cb36248ad6157cf249e1655317e235b39967b6fc Mon Sep 17 00:00:00 2001 From: Jim Hull Date: Thu, 24 Sep 2026 13:14:53 -0700 Subject: [PATCH 1/2] perf: build task/params grammar parsers once, reuse across calls parser() and paramsParser() rebuilt the participle grammar/lexer from scratch on every call via lexer.MustSimple + participle.MustBuild. Since the grammar is static (both functions take no arguments), this repeated compilation work on every parseGrammar/paramsParser.ParseString call for no benefit. Hoist construction into a sync.OnceValue-memoized package var, built once on first use and reused for every subsequent call, including concurrently (participle.Parser[T] is immutable after Build; all per-parse mutable state lives in a parseContext constructed fresh inside ParseString/ParseFromLexer). Verified safe under -race with a concurrent reuse benchmark. Measured on a lint run over a large context tree (~400 tasks against a plugin registry contributing several thousand skills), comparing pprof captures before/after: - newTagLexer cumulative allocations: 176.7M objects -> 0 (no longer invoked per call) - lexer.Upgrade cumulative allocations: 985.1M -> 640.7M objects (-35%) - total heap objects for the run: 10.60B -> 9.41B (-11.2%, ~1.19B fewer) - wall-clock: 625s -> 564.9s (-9.7%) --- pkg/codingcontext/taskparser/grammar.go | 32 +++++++++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-) diff --git a/pkg/codingcontext/taskparser/grammar.go b/pkg/codingcontext/taskparser/grammar.go index fec37a3b..018e4d43 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() } From 005305bbb06e3f62639df104755a0de534bab74e Mon Sep 17 00:00:00 2001 From: Jim Hull Date: Thu, 24 Sep 2026 13:15:36 -0700 Subject: [PATCH 2/2] perf: memoize skill parses across Context instances MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit discoverSkills re-parses every SKILL.md on each call, and callers that construct a Context per unit of work re-walk the whole skill tree each time. A lint-all-tasks loop does exactly that: one (or two, when the task imports a plugin) full discovery pass per task, over an unchanged tree. The same files were parsed once per task rather than once per run. Add an optional SkillCache, attached to the context via NewContextWithSkillCache and keyed by skill file path, so repeated discovery reuses earlier parse results. lenient does not affect parsing, so the path alone is a sufficient key; results are captured before validateAndAddSkill's lenient name inference, keeping the cached value mode-independent. A context without a cache attached behaves exactly as before: the cache methods are nil-receiver-safe, so discoverSkills parses every call. Only deterministic outcomes are memoized. A *markdown.ParseError means the file was read and its content rejected, so every later call would reject it identically and caching it saves a re-parse per task. An I/O error says nothing about the next attempt — permissions, fd exhaustion, a flaky network filesystem, a file that vanished mid-walk — so those are left uncached and retried; memoizing one would let a single transient blip early in a run fail every later task. On a cached error the frontmatter is stored zero rather than as parsed, since parsing fills it field by field and can fail partway, leaving a half-built struct that a later reader might trust. Cached frontmatter is shared by value, and copying the struct copies only the map headers of Content and Metadata. Nothing writes to either map in production today, and the sharing is safe only while that holds, so both fields are documented read-only. Consumers in the lint path run sequentially, so a future write there would surface as cross-task contamination rather than a race the detector would catch. Measured on a lint run over a large context tree (~400 tasks against a plugin registry contributing several thousand skills): ~559-579s before, 113s after. --- pkg/codingcontext/context.go | 51 +++++-- pkg/codingcontext/context_edge_cases_test.go | 137 +++++++++++++++++++ pkg/codingcontext/markdown/frontmatter.go | 12 ++ pkg/codingcontext/skillcache.go | 124 +++++++++++++++++ 4 files changed, 313 insertions(+), 11 deletions(-) create mode 100644 pkg/codingcontext/skillcache.go diff --git a/pkg/codingcontext/context.go b/pkg/codingcontext/context.go index 3e24192e..859e624c 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 5a73d819..dd7a3ba9 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 a723582b..6dfe3bbc 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 00000000..9030f2e1 --- /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 +}