diff --git a/.claude/skills/github-pr-review.md b/.claude/skills/github-pr-review.md index 5d15be3..2da5e23 100644 --- a/.claude/skills/github-pr-review.md +++ b/.claude/skills/github-pr-review.md @@ -1,3 +1,8 @@ +--- +name: github-pr-review +description: Review checklist for Creed pull requests. +--- + # GitHub PR Review Skill Use this when reviewing a Creed pull request. diff --git a/.claude/skills/openspec-development.md b/.claude/skills/openspec-development.md index b4048ec..f7edfbe 100644 --- a/.claude/skills/openspec-development.md +++ b/.claude/skills/openspec-development.md @@ -1,3 +1,8 @@ +--- +name: openspec-development +description: Workflow for adding significant Creed behavior via OpenSpec changes. +--- + # OpenSpec Development Skill Use this when adding or changing significant Creed behavior. diff --git a/.creed/skills/github-pr-review.md b/.creed/skills/github-pr-review.md index 5d15be3..2da5e23 100644 --- a/.creed/skills/github-pr-review.md +++ b/.creed/skills/github-pr-review.md @@ -1,3 +1,8 @@ +--- +name: github-pr-review +description: Review checklist for Creed pull requests. +--- + # GitHub PR Review Skill Use this when reviewing a Creed pull request. diff --git a/.creed/skills/openspec-development.md b/.creed/skills/openspec-development.md index b4048ec..f7edfbe 100644 --- a/.creed/skills/openspec-development.md +++ b/.creed/skills/openspec-development.md @@ -1,3 +1,8 @@ +--- +name: openspec-development +description: Workflow for adding significant Creed behavior via OpenSpec changes. +--- + # OpenSpec Development Skill Use this when adding or changing significant Creed behavior. diff --git a/.cursor/rules/github-pr-review.md b/.cursor/rules/github-pr-review.md index 5d15be3..2da5e23 100644 --- a/.cursor/rules/github-pr-review.md +++ b/.cursor/rules/github-pr-review.md @@ -1,3 +1,8 @@ +--- +name: github-pr-review +description: Review checklist for Creed pull requests. +--- + # GitHub PR Review Skill Use this when reviewing a Creed pull request. diff --git a/.cursor/rules/openspec-development.md b/.cursor/rules/openspec-development.md index b4048ec..f7edfbe 100644 --- a/.cursor/rules/openspec-development.md +++ b/.cursor/rules/openspec-development.md @@ -1,3 +1,8 @@ +--- +name: openspec-development +description: Workflow for adding significant Creed behavior via OpenSpec changes. +--- + # OpenSpec Development Skill Use this when adding or changing significant Creed behavior. diff --git a/README.md b/README.md index 3c94b5a..ce71a4b 100644 --- a/README.md +++ b/README.md @@ -223,6 +223,33 @@ engine renders those descriptors instead of inferring behavior from filenames. `AGENTS.md`, `GEMINI.md`, `.windsurfrules`, or Aider's `CONVENTIONS.md`. - Skill directory outputs receive one file per skill, such as `.claude/skills/` `.cursor/rules/`, and `.gemini/`. +- **Directory-shaped skills.** A skill entry may point at a directory that + contains `SKILL.md` instead of a single flat file. The whole directory — + `references/`, `templates/`, `scripts/`, `assets/`, any regular files — is + emitted under `//`, so Hermes-style skills with support + files sync with zero data loss: + + ```yaml + skills: + - name: techgodhq + path: skills/techgodhq # directory containing SKILL.md + ``` + + emits `.claude/skills/techgodhq/SKILL.md` plus every support file, byte + for byte. Symlinks and non-regular files inside a skill directory are + rejected. A directory that contains only `SKILL.md` is an error — declare + the file path directly instead. +- **Skill frontmatter validation.** When a skill file carries YAML + frontmatter, creed validates the discovery contract: `name` must match the + manifest entry name and `description` must be present. `validate` reports + violations as errors naming the file and the problem; `sync` refuses to + render a skill that breaks the contract. Skills without frontmatter are + legal but produce a `missing_skill_frontmatter` warning, because + downstream tools (Claude Code, Hermes) discover skills through these + fields. +- `validate` also warns when declared skills have no enabled target with a + skill output (for example a skills-only source with just the `agents` + target enabled) — previously that combination was a silent no-op. - Target-specific config outputs are rendered by explicit per-target renderers. Aider receives `.aider.conf.yml` pointing Aider at `CONVENTIONS.md`, plus the separate `CONVENTIONS.md` context file. diff --git a/internal/adapters/localfs/source.go b/internal/adapters/localfs/source.go index f4c52d6..4f31350 100644 --- a/internal/adapters/localfs/source.go +++ b/internal/adapters/localfs/source.go @@ -6,6 +6,7 @@ import ( "context" "fmt" "io" + "io/fs" "os" "path/filepath" "runtime" @@ -172,6 +173,9 @@ func (s *Source) ReadManifestBytes(ctx context.Context) ([]byte, error) { } // ReadSkill reads a skill's full content by name. +// A skill entry may point at a single markdown file or at a directory +// containing SKILL.md; directory skills are read as a full support-file +// tree (see readSkillDirectory). func (s *Source) ReadSkill(ctx context.Context, name string) (*domain.Skill, error) { manifest, err := s.ReadManifest(ctx) if err != nil { @@ -185,6 +189,9 @@ func (s *Source) ReadSkill(ctx context.Context, name string) (*domain.Skill, err if err != nil { return nil, fmt.Errorf("invalid skill path %s: %w", entry.Path, err) } + if info, statErr := os.Lstat(skillPath); statErr == nil && info.IsDir() { + return s.readSkillDirectory(entry, skillPath) + } content, err := readContainedFile(s.rootDir, skillPath) if err != nil { return nil, fmt.Errorf("failed to read skill file %s: %w", skillPath, err) @@ -200,6 +207,73 @@ func (s *Source) ReadSkill(ctx context.Context, name string) (*domain.Skill, err return nil, fmt.Errorf("skill not found: %s", name) } +// readSkillDirectory reads a directory-shaped skill: SKILL.md becomes the +// skill content and every other regular file becomes a support file keyed by +// slash-separated path relative to the skill directory. The walk rejects +// symlinks and non-regular files so a skill directory cannot smuggle content +// outside the source tree or depend on link targets. +func (s *Source) readSkillDirectory(entry domain.SkillEntry, skillDir string) (*domain.Skill, error) { + skillRel := filepath.ToSlash(filepath.Clean(entry.Path)) + files := make(map[string][]byte) + err := filepath.WalkDir(skillDir, func(path string, d fs.DirEntry, walkErr error) error { + if walkErr != nil { + return walkErr + } + if path == skillDir { + return nil + } + if d.Type()&fs.ModeSymlink != 0 { + return fmt.Errorf("skill directory %s contains symlink %s", skillRel, relSlash(skillDir, path)) + } + rel := relSlash(skillDir, path) + if d.IsDir() { + // Reserved support directories emitted verbatim; anything else + // rides along as opaque content. + return nil + } + if !d.Type().IsRegular() { + return fmt.Errorf("skill directory %s contains non-regular file %s", skillRel, rel) + } + if rel == skillFileName { + return nil + } + content, err := readContainedFile(s.rootDir, path) + if err != nil { + return fmt.Errorf("failed to read skill file %s: %w", path, err) + } + files[rel] = content + return nil + }) + if err != nil { + return nil, err + } + if len(files) == 0 { + return nil, fmt.Errorf("skill directory %s contains only SKILL.md; declare the file path directly instead", skillRel) + } + skillMDPath := filepath.Join(skillDir, skillFileName) + content, err := readContainedFile(s.rootDir, skillMDPath) + if err != nil { + return nil, fmt.Errorf("failed to read skill file %s: %w", skillMDPath, err) + } + return &domain.Skill{ + Name: entry.Name, + Path: entry.Path, + Content: content, + Files: files, + }, nil +} + +// skillFileName is the required markdown entrypoint of a directory-shaped skill. +const skillFileName = "SKILL.md" + +func relSlash(base, path string) string { + rel, err := filepath.Rel(base, path) + if err != nil { + return filepath.ToSlash(path) + } + return filepath.ToSlash(rel) +} + // ListSkills returns lightweight info for all skills declared in the manifest. func (s *Source) ListSkills(ctx context.Context) ([]domain.SkillInfo, error) { manifest, err := s.ReadManifest(ctx) diff --git a/internal/adapters/localfs/source_test.go b/internal/adapters/localfs/source_test.go index ed4d02d..5eba9c7 100644 --- a/internal/adapters/localfs/source_test.go +++ b/internal/adapters/localfs/source_test.go @@ -250,3 +250,110 @@ func TestReadManifestRejectsAncestorSymlinkSourcePath(t *testing.T) { t.Fatal("ReadManifest traversed an ancestor symlink") } } + +// createDirectorySkillProject sets up a project whose skill entry points at a +// directory containing SKILL.md plus support files. +func createDirectorySkillProject(t *testing.T) string { + t.Helper() + root := t.TempDir() + creedDir := filepath.Join(root, ".creed") + if err := os.MkdirAll(filepath.Join(creedDir, "skills", "techgodhq", "references"), 0755); err != nil { + t.Fatal(err) + } + manifest := `version: 1 +source: + type: local + path: .creed + +targets: + - name: claude + enabled: true + output_dir: . + +skills: + - name: techgodhq + path: skills/techgodhq + +config: [] +` + if err := os.WriteFile(filepath.Join(creedDir, "manifest.yaml"), []byte(manifest), 0644); err != nil { + t.Fatal(err) + } + skillMD := "---\nname: techgodhq\ndescription: Org-wide agent procedures.\n---\n# TechGodHQ Skill\nUse the references.\n" + if err := os.WriteFile(filepath.Join(creedDir, "skills", "techgodhq", "SKILL.md"), []byte(skillMD), 0644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(creedDir, "skills", "techgodhq", "references", "git.md"), []byte("# Git policy\n"), 0644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(creedDir, "skills", "techgodhq", "references", "review.md"), []byte("# Review policy\n"), 0644); err != nil { + t.Fatal(err) + } + return root +} + +func TestReadSkillDirectory(t *testing.T) { + root := createDirectorySkillProject(t) + source := NewSource(root) + skill, err := source.ReadSkill(context.Background(), "techgodhq") + if err != nil { + t.Fatalf("ReadSkill directory skill: %v", err) + } + if !skill.IsDirectory() { + t.Fatal("expected directory-shaped skill") + } + want := "---\nname: techgodhq\ndescription: Org-wide agent procedures.\n---\n# TechGodHQ Skill\nUse the references.\n" + if string(skill.Content) != want { + t.Fatalf("SKILL.md content mismatch: %q", skill.Content) + } + if len(skill.Files) != 2 { + t.Fatalf("expected 2 support files, got %d: %v", len(skill.Files), skill.Files) + } + if string(skill.Files["references/git.md"]) != "# Git policy\n" { + t.Fatalf("support file git.md mismatch: %q", skill.Files["references/git.md"]) + } + if string(skill.Files["references/review.md"]) != "# Review policy\n" { + t.Fatalf("support file review.md mismatch: %q", skill.Files["references/review.md"]) + } +} + +func TestReadSkillDirectoryMissingSkillMD(t *testing.T) { + root := createDirectorySkillProject(t) + if err := os.Remove(filepath.Join(root, ".creed", "skills", "techgodhq", "SKILL.md")); err != nil { + t.Fatal(err) + } + source := NewSource(root) + if _, err := source.ReadSkill(context.Background(), "techgodhq"); err == nil { + t.Fatal("expected error when directory skill lacks SKILL.md") + } +} + +func TestReadSkillDirectoryRejectsSymlink(t *testing.T) { + root := createDirectorySkillProject(t) + outside := filepath.Join(root, "outside.md") + if err := os.WriteFile(outside, []byte("escaped content\n"), 0644); err != nil { + t.Fatal(err) + } + if err := os.Symlink(outside, filepath.Join(root, ".creed", "skills", "techgodhq", "references", "escape.md")); err != nil { + t.Fatal(err) + } + source := NewSource(root) + _, err := source.ReadSkill(context.Background(), "techgodhq") + if err == nil { + t.Fatal("expected symlink inside skill directory to be rejected") + } +} + +func TestReadSkillDirectoryOnlySkillMD(t *testing.T) { + root := createDirectorySkillProject(t) + if err := os.Remove(filepath.Join(root, ".creed", "skills", "techgodhq", "references", "git.md")); err != nil { + t.Fatal(err) + } + if err := os.Remove(filepath.Join(root, ".creed", "skills", "techgodhq", "references", "review.md")); err != nil { + t.Fatal(err) + } + source := NewSource(root) + if _, err := source.ReadSkill(context.Background(), "techgodhq"); err == nil { + t.Fatal("expected directory with only SKILL.md to be rejected in favor of a direct file entry") + } +} diff --git a/internal/domain/types.go b/internal/domain/types.go index 7ff9c40..f6efffd 100644 --- a/internal/domain/types.go +++ b/internal/domain/types.go @@ -5,14 +5,27 @@ package domain import "time" -// Skill represents an AI skill file stored in the creed source. +// Skill represents an AI skill stored in the creed source. +// A skill is either a single markdown file (Content set, Files nil) or a +// directory-shaped skill (Files set): a directory containing SKILL.md plus +// optional support files such as references/, templates/, scripts/, or assets/. type Skill struct { // Name is the canonical skill identifier (e.g., "code-review"). Name string // Path is the relative path to the skill file within the source. Path string - // Content is the raw file content of the skill. + // Content is the raw file content of the skill. For directory-shaped + // skills this holds the SKILL.md content. Content []byte + // Files holds the support files of a directory-shaped skill, keyed by + // path relative to the skill directory (e.g. "references/api.md"). + // Nil for flat single-file skills. + Files map[string][]byte +} + +// IsDirectory reports whether the skill is directory-shaped. +func (s Skill) IsDirectory() bool { + return s.Files != nil } // SkillInfo is a lightweight summary of a skill, without content payload. diff --git a/internal/service/impl.go b/internal/service/impl.go index 6f2c247..398b861 100644 --- a/internal/service/impl.go +++ b/internal/service/impl.go @@ -40,7 +40,12 @@ Document build, test, lint, and release commands here. }, { Path: "skills/review.md", - Content: `# Review Guidelines + Content: `--- +name: review +description: Guidelines for agents reviewing changes in this project. +--- + +# Review Guidelines Describe how agents should review changes in this project. `, diff --git a/internal/service/impl_test.go b/internal/service/impl_test.go index dfbe11f..3fe6766 100644 --- a/internal/service/impl_test.go +++ b/internal/service/impl_test.go @@ -794,3 +794,350 @@ func hasDoctorCheck(checks []DoctorCheck, kind, code string) bool { } return false } + +func writeSkillProject(t *testing.T, manifest string, files map[string]string) string { + t.Helper() + root := t.TempDir() + creedDir := filepath.Join(root, ".creed") + if err := os.MkdirAll(creedDir, 0755); err != nil { + t.Fatal(err) + } + for path, content := range files { + full := filepath.Join(root, filepath.FromSlash(path)) + if err := os.MkdirAll(filepath.Dir(full), 0755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(full, []byte(content), 0644); err != nil { + t.Fatal(err) + } + } + if err := os.WriteFile(filepath.Join(creedDir, "manifest.yaml"), []byte(manifest), 0644); err != nil { + t.Fatal(err) + } + return root +} + +func TestValidateAcceptsDirectorySkillWithFrontmatter(t *testing.T) { + root := writeSkillProject(t, `version: 1 +source: + type: local + path: .creed +targets: + - name: claude + enabled: true + output_dir: . +skills: + - name: techgodhq + path: skills/techgodhq +config: [] +`, map[string]string{ + ".creed/skills/techgodhq/SKILL.md": "---\nname: techgodhq\ndescription: Org procedures.\n---\n# Org\n", + ".creed/skills/techgodhq/references/git.md": "# Git\n", + ".creed/skills/techgodhq/templates/pr.md": "# PR\n", + }) + result, err := New(root).Validate(context.Background()) + if err != nil { + t.Fatalf("Validate() error = %v", err) + } + if !result.Valid { + t.Fatalf("Validate() errors = %#v", result.Errors) + } +} + +func TestValidateDirectorySkillDiagnostics(t *testing.T) { + base := `version: 1 +source: + type: local + path: .creed +targets: + - name: claude + enabled: true + output_dir: . +skills: + - name: %s + path: skills/%s +config: [] +` + cases := []struct { + name string + skillName string + dirName string + skillMD string + wantCode string + wantIn string // "errors" or "warnings" + extraFiles map[string]string + }{ + { + name: "missing SKILL.md", + skillName: "techgodhq", dirName: "techgodhq", + skillMD: "", wantCode: "missing_source_file", wantIn: "errors", + }, + { + name: "frontmatter name mismatch", + skillName: "techgodhq", dirName: "techgodhq", + skillMD: "---\nname: other\ndescription: x\n---\n# S\n", + wantCode: "skill_name_mismatch", wantIn: "errors", + }, + { + name: "missing description", + skillName: "techgodhq", dirName: "techgodhq", + skillMD: "---\nname: techgodhq\n---\n# S\n", + wantCode: "missing_skill_description", wantIn: "errors", + }, + { + name: "no frontmatter warns", + skillName: "techgodhq", dirName: "techgodhq", + skillMD: "# Plain skill\n", + wantCode: "missing_skill_frontmatter", wantIn: "warnings", + }, + { + name: "unterminated frontmatter", + skillName: "techgodhq", dirName: "techgodhq", + skillMD: "---\nname: techgodhq\n# never closed\n", + wantCode: "unterminated_skill_frontmatter", wantIn: "errors", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + files := map[string]string{} + if tc.skillMD != "" { + files[".creed/skills/"+tc.dirName+"/SKILL.md"] = tc.skillMD + } + for p, c := range tc.extraFiles { + files[p] = c + } + root := writeSkillProject(t, fmt.Sprintf(base, tc.skillName, tc.dirName), files) + result, err := New(root).Validate(context.Background()) + if err != nil { + t.Fatalf("Validate() error = %v", err) + } + var diags []ValidationDiagnostic + if tc.wantIn == "errors" { + diags = result.Errors + } else { + diags = result.Warnings + } + if !hasDiagnostic(diags, tc.wantCode) { + t.Fatalf("Validate() %s = %#v, missing %q", tc.wantIn, diags, tc.wantCode) + } + }) + } +} + +func TestValidateDirectorySkillRejectsSymlink(t *testing.T) { + root := t.TempDir() + creedDir := filepath.Join(root, ".creed") + skillDir := filepath.Join(creedDir, "skills", "techgodhq") + if err := os.MkdirAll(skillDir, 0755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(skillDir, "SKILL.md"), []byte("---\nname: techgodhq\ndescription: x\n---\n# S\n"), 0644); err != nil { + t.Fatal(err) + } + if err := os.Symlink("/etc/hostname", filepath.Join(skillDir, "escape.md")); err != nil { + t.Fatal(err) + } + manifest := `version: 1 +source: + type: local + path: .creed +targets: + - name: claude + enabled: true + output_dir: . +skills: + - name: techgodhq + path: skills/techgodhq +config: [] +` + if err := os.WriteFile(filepath.Join(creedDir, "manifest.yaml"), []byte(manifest), 0644); err != nil { + t.Fatal(err) + } + result, err := New(root).Validate(context.Background()) + if err != nil { + t.Fatalf("Validate() error = %v", err) + } + if result.Valid { + t.Fatalf("Validate() = valid, want symlink rejection; errors=%#v", result.Errors) + } + if !hasDiagnostic(result.Errors, "symlink_source_file") { + t.Fatalf("Validate() errors = %#v, missing symlink_source_file", result.Errors) + } +} + +func TestValidateWarnsWhenSkillsHaveNoOutputTarget(t *testing.T) { + root := writeSkillProject(t, `version: 1 +source: + type: local + path: .creed +targets: + - name: agents + enabled: true + output_dir: . +skills: + - name: plain + path: skills/plain.md +config: [] +`, map[string]string{ + ".creed/skills/plain.md": "---\nname: plain\ndescription: A skill.\n---\n# Plain\n", + }) + result, err := New(root).Validate(context.Background()) + if err != nil { + t.Fatalf("Validate() error = %v", err) + } + if !hasDiagnostic(result.Warnings, "skills_have_no_output") { + t.Fatalf("Validate() warnings = %#v, missing skills_have_no_output", result.Warnings) + } + if !result.Valid { + t.Fatalf("Validate() errors = %#v, want valid (warning only)", result.Errors) + } +} + +func TestSyncDirectorySkillEndToEndNoDataLoss(t *testing.T) { + root := writeSkillProject(t, `version: 1 +source: + type: local + path: .creed +targets: + - name: claude + enabled: true + output_dir: . +skills: + - name: techgodhq + path: skills/techgodhq +config: [] +`, map[string]string{ + ".creed/skills/techgodhq/SKILL.md": "---\nname: techgodhq\ndescription: Org procedures.\n---\n# Org Skill\nBody.\n", + ".creed/skills/techgodhq/references/git.md": "# Git policy\nSigned commits required.\n", + ".creed/skills/techgodhq/references/review.md": "# Review policy\nTwo reviewers.\n", + ".creed/skills/techgodhq/templates/pr-template.md": "# PR\nTemplate body.\n", + ".creed/skills/techgodhq/scripts/check.sh": "#!/bin/sh\nexit 0\n", + }) + svc := New(root) + ctx := context.Background() + + validateResult, err := svc.Validate(ctx) + if err != nil { + t.Fatalf("Validate() error = %v", err) + } + if !validateResult.Valid { + t.Fatalf("Validate() errors = %#v", validateResult.Errors) + } + + syncResult, err := svc.Sync(ctx, usecase.SyncOptions{Target: "claude"}) + if err != nil { + t.Fatalf("Sync() error = %v", err) + } + if syncResult.HasErrors() { + t.Fatalf("Sync() target errors: %#v", syncResult.Targets) + } + if syncResult.TotalFilesWritten() != 5 { + t.Fatalf("TotalFilesWritten() = %d, want 5; result=%#v", syncResult.TotalFilesWritten(), syncResult) + } + + emitted := map[string]string{ + ".claude/skills/techgodhq/SKILL.md": "---\nname: techgodhq\ndescription: Org procedures.\n---\n# Org Skill\nBody.\n", + ".claude/skills/techgodhq/references/git.md": "# Git policy\nSigned commits required.\n", + ".claude/skills/techgodhq/references/review.md": "# Review policy\nTwo reviewers.\n", + ".claude/skills/techgodhq/templates/pr-template.md": "# PR\nTemplate body.\n", + ".claude/skills/techgodhq/scripts/check.sh": "#!/bin/sh\nexit 0\n", + } + for path, want := range emitted { + got := mustRead(t, filepath.Join(root, filepath.FromSlash(path))) + if got != want { + t.Errorf("%s = %q, want %q", path, got, want) + } + } + + // Zero data loss: every source file under the skill directory appears + // under the emitted skill directory, and vice versa. + sourceFiles := 0 + err = filepath.Walk(filepath.Join(root, ".creed", "skills", "techgodhq"), func(path string, info os.FileInfo, err error) error { + if err == nil && !info.IsDir() { + sourceFiles++ + } + return err + }) + if err != nil { + t.Fatal(err) + } + emittedFiles := 0 + err = filepath.Walk(filepath.Join(root, ".claude", "skills", "techgodhq"), func(path string, info os.FileInfo, err error) error { + if err == nil && !info.IsDir() { + emittedFiles++ + } + return err + }) + if err != nil { + t.Fatal(err) + } + if sourceFiles != emittedFiles || sourceFiles != 5 { + t.Fatalf("file count mismatch: source=%d emitted=%d, want 5/5", sourceFiles, emittedFiles) + } + + // Second sync is a no-op. + second, err := svc.Sync(ctx, usecase.SyncOptions{Target: "claude"}) + if err != nil { + t.Fatalf("second Sync() error = %v", err) + } + if second.TotalFilesWritten() != 0 { + t.Fatalf("second sync wrote %d files, want 0 (idempotent)", second.TotalFilesWritten()) + } + if second.TotalFilesSkipped() != 5 { + t.Fatalf("second sync skipped %d files, want 5", second.TotalFilesSkipped()) + } +} + +func TestValidateFlatSkillFrontmatterDiagnostics(t *testing.T) { + base := `version: 1 +source: + type: local + path: .creed +targets: + - name: claude + enabled: true + output_dir: . +skills: + - name: demo + path: skills/%s +config: [] +` + cases := []struct { + name string + file string + content string + wantCode string + wantIn string + }{ + {"mismatch", "demo.md", "---\nname: other\ndescription: x\n---\n# S\n", "skill_name_mismatch", "errors"}, + {"missing description", "demo.md", "---\nname: demo\n---\n# S\n", "missing_skill_description", "errors"}, + {"no frontmatter", "demo.md", "# Plain\n", "missing_skill_frontmatter", "warnings"}, + {"valid", "demo.md", "---\nname: demo\ndescription: A skill.\n---\n# S\n", "", ""}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + root := writeSkillProject(t, fmt.Sprintf(base, tc.file), map[string]string{ + ".creed/skills/" + tc.file: tc.content, + }) + result, err := New(root).Validate(context.Background()) + if err != nil { + t.Fatalf("Validate() error = %v", err) + } + if tc.wantCode == "" { + if !result.Valid { + t.Fatalf("Validate() errors = %#v, want valid", result.Errors) + } + return + } + var diags []ValidationDiagnostic + if tc.wantIn == "errors" { + diags = result.Errors + } else { + diags = result.Warnings + } + if !hasDiagnostic(diags, tc.wantCode) { + t.Fatalf("Validate() %s = %#v, missing %q", tc.wantIn, diags, tc.wantCode) + } + }) + } +} diff --git a/internal/service/validate.go b/internal/service/validate.go index 401f326..8db6e52 100644 --- a/internal/service/validate.go +++ b/internal/service/validate.go @@ -5,6 +5,7 @@ import ( "context" "fmt" "io" + "io/fs" "os" "path/filepath" "strings" @@ -13,6 +14,7 @@ import ( "github.com/techgodhq/creed/internal/adapters/localfs" "github.com/techgodhq/creed/internal/domain" + "github.com/techgodhq/creed/internal/skillmeta" ) // ValidationDiagnostic identifies one manifest or source-health finding. @@ -123,6 +125,28 @@ func (s *Implementation) Validate(ctx context.Context) (ValidationResult, error) validateTargetConfigs(&result, manifest.Targets, "manifest.yaml") + if len(manifest.Skills) > 0 { + hasSkillDirOutput := false + for _, target := range manifest.Targets { + if !target.Enabled { + continue + } + known, lookupErr := domain.LookupTarget(target.Name) + if lookupErr != nil { + continue + } + for _, output := range known.Outputs("") { + if output.Kind == domain.OutputKindSkillDir { + hasSkillDirOutput = true + break + } + } + } + if !hasSkillDirOutput { + result.addWarning("skills_have_no_output", fmt.Sprintf("%d declared skill(s) but no enabled target has a skill output; skills will not be emitted anywhere", len(manifest.Skills)), "manifest.yaml") + } + } + localNames := map[string]struct{}{} if sourceType != "git" { seenNames := map[string]string{} @@ -404,6 +428,14 @@ func (s *Implementation) validateEntryAt(result *ValidationResult, sourceRoot, k result.addError("escaped_source_path", fmt.Sprintf("%s resolves outside source directory", label), cleanPath) return } + if info.IsDir() { + if kind != "skill" { + result.addError("non_regular_source_file", fmt.Sprintf("%s source must be a regular file", label), cleanPath) + return + } + s.validateSkillDirectory(result, label, name, path, cleanPath) + return + } if !info.Mode().IsRegular() { result.addError("non_regular_source_file", fmt.Sprintf("%s source must be a regular file", label), cleanPath) return @@ -420,6 +452,74 @@ func (s *Implementation) validateEntryAt(result *ValidationResult, sourceRoot, k if strings.TrimSpace(string(content)) == "" { result.addWarning("empty_source_content", fmt.Sprintf("%s source file is empty", label), cleanPath) } + if kind == "skill" { + validateSkillFrontmatter(result, label, name, content, cleanPath) + } +} + +// validateSkillDirectory validates a directory-shaped skill entry: SKILL.md +// must exist as a regular file, support files must be regular and +// non-symlinked, and SKILL.md frontmatter must carry a matching name and a +// description when frontmatter is present at all. +func (s *Implementation) validateSkillDirectory(result *ValidationResult, label, name, dirPath, cleanPath string) { + skillMD := filepath.Join(dirPath, "SKILL.md") + info, err := os.Lstat(skillMD) + if err != nil { + result.addError("missing_source_file", fmt.Sprintf("%s directory skill must contain SKILL.md", label), cleanPath) + return + } + if info.Mode()&os.ModeSymlink != 0 || !info.Mode().IsRegular() { + result.addError("non_regular_source_file", fmt.Sprintf("%s SKILL.md must be a regular file", label), cleanPath+"/SKILL.md") + return + } + walkErr := filepath.WalkDir(dirPath, func(path string, d fs.DirEntry, walkErr error) error { + if walkErr != nil { + return walkErr + } + if path == dirPath { + return nil + } + if d.Type()&fs.ModeSymlink != 0 { + result.addError("symlink_source_file", fmt.Sprintf("%s skill directory must not contain symlinks (%s)", label, relSlash(dirPath, path)), cleanPath) + return filepath.SkipDir + } + if d.IsDir() { + return nil + } + if !d.Type().IsRegular() { + result.addError("non_regular_source_file", fmt.Sprintf("%s skill directory must contain only regular files (%s)", label, relSlash(dirPath, path)), cleanPath) + } + return nil + }) + if walkErr != nil { + result.addError("unreadable_source_file", fmt.Sprintf("%s skill directory cannot be walked: %v", label, walkErr), cleanPath) + } + content, readErr := os.ReadFile(skillMD) + if readErr != nil { + result.addError("unreadable_source_file", fmt.Sprintf("%s SKILL.md cannot be read", label), cleanPath+"/SKILL.md") + return + } + validateSkillFrontmatter(result, label, name, content, cleanPath+"/SKILL.md") +} + +// validateSkillFrontmatter enforces the skill contract on SKILL.md content: +// when YAML frontmatter exists, its name must match the manifest entry name +// and a description must be present. Skills without frontmatter are legal but +// warned about, because downstream tools (Claude Code, Hermes) discover +// skills through these fields. +func validateSkillFrontmatter(result *ValidationResult, label, name string, content []byte, path string) { + fm, found, problems := skillmeta.Parse(content) + if !found { + result.addWarning("missing_skill_frontmatter", fmt.Sprintf("%s has no YAML frontmatter; name/description discovery may not work", label), path) + return + } + for _, problem := range problems { + result.addError(problem.Code, fmt.Sprintf("%s %s", label, problem.Message), path) + return + } + for _, problem := range skillmeta.Validate(name, fm) { + result.addError(problem.Code, fmt.Sprintf("%s %s", label, problem.Message), path) + } } func validateSkillIdentifier(name string) error { @@ -461,3 +561,13 @@ func (r *ValidationResult) addError(code, message, path string) { func (r *ValidationResult) addWarning(code, message, path string) { r.Warnings = append(r.Warnings, ValidationDiagnostic{Severity: "warning", Code: code, Message: message, Path: path}) } + +// relSlash returns path relative to base as a slash-separated path, falling +// back to the full path when the two are not related. +func relSlash(base, path string) string { + rel, err := filepath.Rel(base, path) + if err != nil { + return filepath.ToSlash(path) + } + return filepath.ToSlash(rel) +} diff --git a/internal/skillmeta/skillmeta.go b/internal/skillmeta/skillmeta.go new file mode 100644 index 0000000..e403f20 --- /dev/null +++ b/internal/skillmeta/skillmeta.go @@ -0,0 +1,95 @@ +// Package skillmeta implements creed's skill discovery contract: the YAML +// frontmatter a SKILL.md (or flat skill file) may carry, and the rules that +// frontmatter must satisfy. It is shared by validation (diagnostics) and +// sync rendering (generation-time enforcement) so the two cannot drift. +package skillmeta + +import ( + "fmt" + "strings" + + "gopkg.in/yaml.v3" +) + +// Frontmatter holds the skill discovery fields creed interprets. +// All other frontmatter keys pass through untouched. +type Frontmatter struct { + Name string `yaml:"name"` + Description string `yaml:"description"` +} + +// Problem describes one skill-contract violation. +type Problem struct { + // Code is a stable machine-readable identifier. + Code string + // Message names the violation for humans. + Message string +} + +const ( + // CodeUnterminated means a frontmatter block opened but never closed. + CodeUnterminated = "unterminated_skill_frontmatter" + // CodeInvalidYAML means the frontmatter block is not valid YAML. + CodeInvalidYAML = "invalid_skill_frontmatter" + // CodeMissingName means frontmatter exists but carries no name. + CodeMissingName = "missing_skill_name" + // CodeNameMismatch means the frontmatter name differs from the manifest name. + CodeNameMismatch = "skill_name_mismatch" + // CodeMissingDescription means the frontmatter carries no description. + CodeMissingDescription = "missing_skill_description" +) + +// Parse extracts frontmatter from skill content. found is false when the +// content carries no frontmatter block at all (legal, but callers should +// warn: downstream tools discover skills through these fields). A block +// that opens but never closes yields a CodeUnterminated problem. +func Parse(content []byte) (fm Frontmatter, found bool, problems []Problem) { + text := string(content) + if !strings.HasPrefix(text, "---\n") { + return Frontmatter{}, false, nil + } + rest := text[4:] + end := strings.Index(rest, "\n---") + if end < 0 { + return Frontmatter{}, true, []Problem{{ + Code: CodeUnterminated, + Message: "frontmatter block is opened but never closed", + }} + } + block := rest[:end] + if err := yaml.Unmarshal([]byte(block), &fm); err != nil { + return Frontmatter{}, true, []Problem{{ + Code: CodeInvalidYAML, + Message: fmt.Sprintf("frontmatter is not valid YAML: %v", err), + }} + } + return fm, true, nil +} + +// Validate checks parsed frontmatter against the manifest-declared skill +// name. It requires a present name that matches, and a non-empty +// description. Problems reference the manifest name so callers can prefix +// the file path when surfacing them. +func Validate(manifestName string, fm Frontmatter) []Problem { + var problems []Problem + if strings.TrimSpace(fm.Name) == "" { + problems = append(problems, Problem{ + Code: CodeMissingName, + Message: "frontmatter has no name", + }) + return problems + } + if fm.Name != manifestName { + problems = append(problems, Problem{ + Code: CodeNameMismatch, + Message: fmt.Sprintf("frontmatter name %q does not match manifest name %q", fm.Name, manifestName), + }) + } + if strings.TrimSpace(fm.Description) == "" { + problems = append(problems, Problem{ + Code: CodeMissingDescription, + Message: "frontmatter has no description", + }) + } + return problems +} diff --git a/internal/skillmeta/skillmeta_test.go b/internal/skillmeta/skillmeta_test.go new file mode 100644 index 0000000..0beb542 --- /dev/null +++ b/internal/skillmeta/skillmeta_test.go @@ -0,0 +1,67 @@ +package skillmeta + +import "testing" + +func TestParseNone(t *testing.T) { + _, found, problems := Parse([]byte("# Just markdown\n")) + if found || problems != nil { + t.Fatalf("found=%v problems=%v, want none", found, problems) + } +} + +func TestParseValid(t *testing.T) { + fm, found, problems := Parse([]byte("---\nname: demo\ndescription: A demo skill.\n---\n# Body\n")) + if !found || len(problems) != 0 { + t.Fatalf("found=%v problems=%v", found, problems) + } + if fm.Name != "demo" || fm.Description != "A demo skill." { + t.Fatalf("parsed %+v", fm) + } +} + +func TestParseUnterminated(t *testing.T) { + _, found, problems := Parse([]byte("---\nname: demo\n# no close\n")) + if !found || len(problems) != 1 || problems[0].Code != CodeUnterminated { + t.Fatalf("found=%v problems=%v", found, problems) + } +} + +func TestParseInvalidYAML(t *testing.T) { + _, found, problems := Parse([]byte("---\nname: [unclosed\n---\n# Body\n")) + if !found || len(problems) != 1 || problems[0].Code != CodeInvalidYAML { + t.Fatalf("found=%v problems=%v", found, problems) + } +} + +func TestValidateHappy(t *testing.T) { + problems := Validate("demo", Frontmatter{Name: "demo", Description: "ok"}) + if len(problems) != 0 { + t.Fatalf("problems=%v", problems) + } +} + +func TestValidateMismatchAndDescription(t *testing.T) { + problems := Validate("manifest-name", Frontmatter{Name: "other", Description: ""}) + if len(problems) != 2 { + t.Fatalf("problems=%v, want mismatch + missing description", problems) + } + if problems[0].Code != CodeNameMismatch || problems[1].Code != CodeMissingDescription { + t.Fatalf("problem codes: %v", problems) + } +} + +func TestValidateMissingName(t *testing.T) { + problems := Validate("x", Frontmatter{Description: "d"}) + if len(problems) != 1 || problems[0].Code != CodeMissingName { + t.Fatalf("problems=%v", problems) + } +} + +func TestCRLFContentIsNotFrontmatter(t *testing.T) { + // A file starting with "---\r\n" is not a frontmatter block for our + // purposes; it must not be parsed as one. + _, found, problems := Parse([]byte("---\r\nname: demo\r\n---\r\n")) + if found { + t.Fatalf("found=%v problems=%v, want no frontmatter detected", found, problems) + } +} diff --git a/internal/usecase/sync.go b/internal/usecase/sync.go index ad578e9..b297f29 100644 --- a/internal/usecase/sync.go +++ b/internal/usecase/sync.go @@ -15,6 +15,7 @@ import ( "github.com/techgodhq/creed/internal/domain" "github.com/techgodhq/creed/internal/ports" + "github.com/techgodhq/creed/internal/skillmeta" ) // previewEmitter is an optional emitter capability for dry-run diff previews. @@ -411,12 +412,52 @@ func renderContextOutput(output domain.TargetOutput, inputs renderInputs) ([]por return []ports.EmittedFile{{Path: output.Path, Content: content}}, nil } +// skillFileName is the required markdown entrypoint of a directory-shaped +// skill; it must match localfs.skillFileName. +const skillFileName = "SKILL.md" + +// checkSkillFrontmatter enforces the skill discovery contract at generation +// time: when a skill declares frontmatter, its name must match the manifest +// and it must carry a description. Missing frontmatter is not a render error +// (validate warns about it); broken or mismatched frontmatter is. +func checkSkillFrontmatter(skill domain.Skill) []skillmeta.Problem { + fm, found, problems := skillmeta.Parse(skill.Content) + if !found { + return nil + } + if len(problems) > 0 { + return problems + } + return skillmeta.Validate(skill.Name, fm) +} + func renderSkillDirOutput(output domain.TargetOutput, inputs renderInputs) ([]ports.EmittedFile, error) { files := make([]ports.EmittedFile, 0, len(inputs.skills)) for _, skill := range inputs.skills { if err := validateSkillName(skill.Name); err != nil { return nil, fmt.Errorf("skill %q: %w", skill.Name, err) } + if problems := checkSkillFrontmatter(skill); len(problems) > 0 { + return nil, fmt.Errorf("skill %q (%s): %s", skill.Name, skill.Path, problems[0].Message) + } + if skill.IsDirectory() { + files = append(files, ports.EmittedFile{ + Path: output.Path + skill.Name + "/" + skillFileName, + Content: skill.Content, + }) + support := make([]string, 0, len(skill.Files)) + for rel := range skill.Files { + support = append(support, rel) + } + sort.Strings(support) + for _, rel := range support { + files = append(files, ports.EmittedFile{ + Path: output.Path + skill.Name + "/" + rel, + Content: skill.Files[rel], + }) + } + continue + } files = append(files, ports.EmittedFile{ Path: output.Path + skill.Name + ".md", Content: skill.Content, diff --git a/internal/usecase/sync_test.go b/internal/usecase/sync_test.go index 021dde2..aa6f364 100644 --- a/internal/usecase/sync_test.go +++ b/internal/usecase/sync_test.go @@ -856,3 +856,123 @@ func TestSyncRejectsSkillNameTraversalBeforeEmit(t *testing.T) { t.Fatalf("malicious skill created an outside file: %v", err) } } + +func TestPrepareFiles_DirectorySkillEmitsFullTree(t *testing.T) { + target, _ := domain.LookupTarget("claude") // .claude/skills/ + skills := []domain.Skill{ + { + Name: "techgodhq", + Path: "skills/techgodhq", + Content: []byte("---\nname: techgodhq\ndescription: Org procedures.\n---\n# Org Skill\n"), + Files: map[string][]byte{ + "references/git.md": []byte("# Git\n"), + "references/review.md": []byte("# Review\n"), + "templates/pr.md": []byte("# PR template\n"), + }, + }, + {Name: "flat", Path: "skills/flat.md", Content: []byte("# Flat skill\n")}, + } + files, err := prepareFiles(target, skills, nil) + if err != nil { + t.Fatalf("prepare files: %v", err) + } + want := map[string]string{ + ".claude/skills/techgodhq/SKILL.md": "---\nname: techgodhq\ndescription: Org procedures.\n---\n# Org Skill\n", + ".claude/skills/techgodhq/references/git.md": "# Git\n", + ".claude/skills/techgodhq/references/review.md": "# Review\n", + ".claude/skills/techgodhq/templates/pr.md": "# PR template\n", + ".claude/skills/flat.md": "# Flat skill\n", + } + if len(files) != len(want) { + t.Fatalf("expected %d files, got %d: %v", len(want), len(files), files) + } + for _, f := range files { + expected, ok := want[f.Path] + if !ok { + t.Fatalf("unexpected emitted path %q", f.Path) + } + if string(f.Content) != expected { + t.Errorf("path %q content mismatch: got %q want %q", f.Path, f.Content, expected) + } + } +} + +func TestPrepareFiles_DirectorySkillSortedDeterministically(t *testing.T) { + target, _ := domain.LookupTarget("claude") + skills := []domain.Skill{ + { + Name: "zeta", + Path: "skills/zeta", + Content: []byte("# Zeta\n"), + Files: map[string][]byte{ + "b.md": []byte("b"), + "a.md": []byte("a"), + "c/d.md": []byte("d"), + }, + }, + } + files, err := prepareFiles(target, skills, nil) + if err != nil { + t.Fatalf("prepare files: %v", err) + } + var paths []string + for _, f := range files { + paths = append(paths, f.Path) + } + expected := []string{ + ".claude/skills/zeta/SKILL.md", + ".claude/skills/zeta/a.md", + ".claude/skills/zeta/b.md", + ".claude/skills/zeta/c/d.md", + } + if len(paths) != len(expected) { + t.Fatalf("expected %d paths, got %v", len(expected), paths) + } + for i := range expected { + if paths[i] != expected[i] { + t.Fatalf("path order mismatch at %d: got %q want %q (all: %v)", i, paths[i], expected[i], paths) + } + } +} + +func TestPrepareFiles_SkillFrontmatterMismatchFails(t *testing.T) { + target, _ := domain.LookupTarget("claude") + skills := []domain.Skill{ + { + Name: "right-name", + Path: "skills/wrong.md", + Content: []byte("---\nname: wrong-name\ndescription: Mismatched.\n---\n# Skill\n"), + }, + } + if _, err := prepareFiles(target, skills, nil); err == nil { + t.Fatal("expected frontmatter name mismatch to fail rendering") + } +} + +func TestPrepareFiles_SkillMissingDescriptionFails(t *testing.T) { + target, _ := domain.LookupTarget("claude") + skills := []domain.Skill{ + { + Name: "nodesc", + Path: "skills/nodesc.md", + Content: []byte("---\nname: nodesc\n---\n# Skill\n"), + }, + } + if _, err := prepareFiles(target, skills, nil); err == nil { + t.Fatal("expected missing description to fail rendering") + } +} + +func TestPrepareFiles_SkillWithoutFrontmatterRenders(t *testing.T) { + target, _ := domain.LookupTarget("claude") + skills := []domain.Skill{ + {Name: "legacy", Path: "skills/legacy.md", Content: []byte("# Legacy skill without frontmatter\n")}, + } + files, err := prepareFiles(target, skills, nil) + if err != nil { + t.Fatalf("prepare files: %v", err) + } + if len(files) != 1 || files[0].Path != ".claude/skills/legacy.md" { + t.Fatalf("unexpected files: %v", files) + } +}