Perf/cache skill and grammar parses

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,17 +1059,20 @@ 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
 		}
 	}
 
 	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 {

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;

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)