diff --git a/cmd/project/compile.go b/cmd/project/compile.go index 9ecdac5..32267bc 100644 --- a/cmd/project/compile.go +++ b/cmd/project/compile.go @@ -16,13 +16,18 @@ import ( // else - never a JSON error/report object - on failure. func printCompileIssues(cmd *cobra.Command, issues []projects.Issue) { errStyle := lipgloss.NewStyle().Foreground(lipgloss.Color("196")) + warnStyle := lipgloss.NewStyle().Foreground(lipgloss.Color("214")) for _, issue := range issues { location := issue.File if issue.Path != "" { location += " " + issue.Path } - fmt.Fprintln(cmd.ErrOrStderr(), errStyle.Render("✗ ")+location+": "+issue.Message) + prefix := errStyle.Render("✗ ") + if issue.IsWarning() { + prefix = warnStyle.Render("⚠ ") + } + fmt.Fprintln(cmd.ErrOrStderr(), prefix+location+": "+issue.Message) } } @@ -37,10 +42,14 @@ func newCompileCmd() *cobra.Command { Long: `Validates and compiles the project directory. With --json, prints the canonical single-line config JSON to stdout; otherwise prints the pretty-printed config and its hash.`, RunE: func(cmd *cobra.Command, args []string) error { result, issues := projects.CompileWithSkillBundles(dir, skillBundlesDir) + errs, _ := projects.PartitionIssues(issues) if len(issues) > 0 { printCompileIssues(cmd, issues) - return fmt.Errorf("%d validation issue(s)", len(issues)) + } + + if len(errs) > 0 { + return fmt.Errorf("%d validation issue(s)", len(errs)) } if asJSON { diff --git a/cmd/project/validate.go b/cmd/project/validate.go index edd925b..b92a3dd 100644 --- a/cmd/project/validate.go +++ b/cmd/project/validate.go @@ -9,22 +9,30 @@ import ( "github.com/spf13/cobra" ) -// printIssues renders validation issues either as human-readable lines or JSON. +// printIssues renders validation issues either as human-readable lines or +// JSON. "valid" (JSON) and the pass/fail line (human) are computed from +// errors only - warnings are printed but never make a run invalid. func printIssues(cmd *cobra.Command, issues []projects.Issue, asJSON bool) { if asJSON { - out, _ := json.MarshalIndent(map[string]any{"valid": len(issues) == 0, "issues": issues}, "", " ") + errs, _ := projects.PartitionIssues(issues) + out, _ := json.MarshalIndent(map[string]any{"valid": len(errs) == 0, "issues": issues}, "", " ") fmt.Fprintln(cmd.OutOrStdout(), string(out)) return } errStyle := lipgloss.NewStyle().Foreground(lipgloss.Color("196")) + warnStyle := lipgloss.NewStyle().Foreground(lipgloss.Color("214")) for _, issue := range issues { location := issue.File if issue.Path != "" { location += " " + issue.Path } - cmd.Println(errStyle.Render("✗ ") + location + ": " + issue.Message) + prefix := errStyle.Render("✗ ") + if issue.IsWarning() { + prefix = warnStyle.Render("⚠ ") + } + cmd.Println(prefix + location + ": " + issue.Message) } } @@ -38,16 +46,24 @@ func newValidateCmd() *cobra.Command { Long: `Validates project.json and every agent definition against the published schemas. Exits 0 when valid, 1 when not (CI-friendly).`, RunE: func(cmd *cobra.Command, args []string) error { issues := projects.Validate(dir) + errs, warnings := projects.PartitionIssues(issues) if len(issues) > 0 { printIssues(cmd, issues, asJSON) - return fmt.Errorf("%d validation issue(s)", len(issues)) + } else if asJSON { + printIssues(cmd, nil, true) } - if asJSON { - printIssues(cmd, nil, true) - } else { - cmd.Println("✓ Project is valid") + if len(errs) > 0 { + return fmt.Errorf("%d validation issue(s)", len(errs)) + } + + if !asJSON { + if len(warnings) > 0 { + cmd.Printf("✓ Project is valid (%d warning(s))\n", len(warnings)) + } else { + cmd.Println("✓ Project is valid") + } } return nil }, diff --git a/projects/compile.go b/projects/compile.go index b084db1..998837b 100644 --- a/projects/compile.go +++ b/projects/compile.go @@ -76,15 +76,20 @@ func compile(dir, skillBundlesDir string) (*CompileResult, []Issue) { for _, s := range loaded.Skills { compiled, skillIssues := readSkillBundle(dir, s, skillBundlesDir) - if len(skillIssues) > 0 { - issues = append(issues, skillIssues...) + issues = append(issues, skillIssues...) + + // compiled is nil either because of a genuine content error (caught + // below by the errors check) or because the skill's git tree hash + // could not be resolved - a warning-only skip (readSkillBundle), not + // a compile failure. + if compiled == nil { continue } skills = append(skills, *compiled) } - if len(issues) > 0 { + if errs, _ := PartitionIssues(issues); len(errs) > 0 { return nil, issues } @@ -97,7 +102,7 @@ func compile(dir, skillBundlesDir string) (*CompileResult, []Issue) { configJSON, err := json.Marshal(config) if err != nil { - return nil, []Issue{{File: "project.json", Message: "internal error serializing compiled config: " + err.Error()}} + return nil, append(issues, Issue{File: "project.json", Message: "internal error serializing compiled config: " + err.Error()}) } sum := sha256.Sum256(configJSON) @@ -106,11 +111,22 @@ func compile(dir, skillBundlesDir string) (*CompileResult, []Issue) { Config: config, ConfigJSON: configJSON, Hash: hex.EncodeToString(sum[:]), - }, nil + }, issues // issues may be non-nil here too: warning-only issues (e.g. an uncommitted skill dir) do not block compile. } -// Validate runs the full compile pipeline and reports issues only. +// Validate loads and validates a project directory: schema checks, agent and +// skill discovery, and skill file-content checks (dotfiles, size caps, path +// length, symlinks, frontmatter). It never touches git: tree-hash resolution +// only happens on the compile path (readSkillBundle), so an uncommitted or +// never-committed skill directory - the artifact that ships is the git tree +// at a commit, resolved by the platform's compile job, not the working tree - +// validates cleanly as long as its content is valid. func Validate(dir string) []Issue { - _, issues := Compile(dir) + absDir, err := filepath.Abs(dir) + if err != nil { + return []Issue{{File: "project.json", Message: "cannot resolve project directory: " + err.Error()}} + } + + _, issues := Load(absDir) return issues } diff --git a/projects/skills.go b/projects/skills.go index fb78a88..22fb6ad 100644 --- a/projects/skills.go +++ b/projects/skills.go @@ -90,7 +90,18 @@ func discoverSkills(dir, srcDir string) ([]SkillDefinition, []Issue) { } seenSlugs[slug] = skillDir - skills = append(skills, SkillDefinition{Slug: slug, Dir: skillDir}) + skill := SkillDefinition{Slug: slug, Dir: skillDir} + + // Content-only validation runs as part of discovery (Load), so + // `validate` catches dotfiles, size caps, bad frontmatter, etc. It + // never touches git *state* - only listSkillFiles's file-listing + // path uses git, to exclude gitignored files - so an uncommitted or + // never-committed skill directory validates cleanly. Git state + // (tree-hash resolution) is a compile-only concern (readSkillBundle). + _, contentIssues := validateSkillContent(dir, skill, false) + issues = append(issues, contentIssues...) + + skills = append(skills, skill) } sort.Slice(skills, func(i, j int) bool { return skills[i].Slug < skills[j].Slug }) @@ -107,49 +118,128 @@ type skillFile struct { content []byte } -// readSkillBundle walks a skill directory, validates every file against the -// server's skill-validator rules, and returns the compiled skill: its slug -// and the git tree hash of its directory at HEAD. When skillBundlesDir is -// non-empty, it also writes /.zip with every file's -// raw bytes. Symlinks (files or dirs) are rejected, mirroring readPromptFile's -// symlink handling in prompt.go. -func readSkillBundle(projectDir string, skill SkillDefinition, skillBundlesDir string) (*CompiledSkill, []Issue) { +// listSkillFiles resolves the relative file paths under a skill directory. +// Inside a git repository, the list comes from `git ls-files -co +// --exclude-standard`: tracked files plus untracked-but-not-ignored ones, so +// a gitignored file (e.g. .DS_Store) never reaches validation or the bundle - +// the same file set the platform's compile job sees when it clones the repo +// at a commit. Outside a git repository, it falls back to a plain filesystem +// walk (listSkillFilesFS) so `validate` still works anywhere; only the +// compile-only tree-hash step (resolveSkillTreeHash) cares whether a git +// repository exists at all. +func listSkillFiles(projectDir string, skill SkillDefinition) (relPaths []string, insideRepo bool, err error) { root := filepath.Join(projectDir, skill.Dir) - needAllContent := skillBundlesDir != "" - var files []skillFile - var issues []Issue + repoRoot, gitErr := runGit(root, "rev-parse", "--show-toplevel") + if gitErr != nil { + paths, walkErr := listSkillFilesFS(root) + return paths, false, walkErr + } - walkErr := filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error { - if err != nil { - return err + relToRepo, err := skillDirRelToRepo(repoRoot, root) + if err != nil { + return nil, true, err + } + + cmd := exec.Command("git", "ls-files", "-co", "--exclude-standard", "-z", "--", relToRepo) + cmd.Dir = repoRoot + + out, err := cmd.Output() + if err != nil { + return nil, true, err + } + + relToRepoOS := filepath.FromSlash(relToRepo) + + var paths []string + for _, p := range strings.Split(string(out), "\x00") { + if p == "" { + continue + } + + rel, relErr := filepath.Rel(relToRepoOS, filepath.FromSlash(p)) + if relErr != nil { + continue + } + + paths = append(paths, filepath.ToSlash(rel)) + } + + return paths, true, nil +} + +// listSkillFilesFS walks root's filesystem tree and returns every entry's +// path relative to root, forward-slash separated. Used only as the +// not-a-git-repository fallback for listSkillFiles. Symlinks are not +// special-cased here - WalkDir does not traverse into them regardless, and +// validateSkillContent lstats every path itself to reject them. +func listSkillFilesFS(root string) ([]string, error) { + var relPaths []string + + err := filepath.WalkDir(root, func(path string, d fs.DirEntry, walkErr error) error { + if walkErr != nil { + return walkErr } if path == root { return nil } + if d.IsDir() { + return nil + } + + relPaths = append(relPaths, filepath.ToSlash(strings.TrimPrefix(path, root+string(filepath.Separator)))) + return nil + }) + + return relPaths, err +} + +// validateSkillContent resolves a skill's file list (listSkillFiles) and +// applies every per-file and bundle-level check ported from the server's +// skill-validator: dotfiles, size caps, path length, symlink rejection, the +// 200-file cap, and SKILL.md frontmatter. It never touches git *state* - +// tree-hash resolution is a separate, compile-only step (resolveSkillTreeHash) +// - so this is what both Load (content-only validation, used by `validate`) +// and readSkillBundle (compile) call. needAllContent additionally reads every +// file's raw bytes (not just SKILL.md's) for bundle zipping. +func validateSkillContent(projectDir string, skill SkillDefinition, needAllContent bool) ([]skillFile, []Issue) { + root := filepath.Join(projectDir, skill.Dir) + + relPaths, _, err := listSkillFiles(projectDir, skill) + if err != nil { + return nil, []Issue{{File: skill.Dir, Message: fmt.Sprintf("cannot list files for skill %q: %s", skill.Slug, err.Error())}} + } + + sort.Strings(relPaths) + + var files []skillFile + var issues []Issue - relPath := filepath.ToSlash(strings.TrimPrefix(path, root+string(filepath.Separator))) + for _, relPath := range relPaths { fileID := filepath.Join(skill.Dir, filepath.FromSlash(relPath)) + absPath := filepath.Join(root, filepath.FromSlash(relPath)) + + info, err := os.Lstat(absPath) + if err != nil { + // Tracked-but-deleted-from-disk (git ls-files -c reports index + // entries regardless of working-tree presence); skip it. + continue + } - if d.Type()&fs.ModeSymlink != 0 { + if info.Mode()&fs.ModeSymlink != 0 { issues = append(issues, Issue{ File: fileID, Message: fmt.Sprintf("skill %q: %q is a symlink; symlinks are not allowed", skill.Slug, relPath), }) - return nil + continue } - if d.IsDir() { - return nil + if info.IsDir() { + continue } issues = append(issues, validateSkillFilePath(skill.Slug, fileID, relPath)...) - info, err := d.Info() - if err != nil { - return err - } - if info.Size() > MaxSkillFileBytes { issues = append(issues, Issue{ File: fileID, @@ -170,20 +260,15 @@ func readSkillBundle(projectDir string, skill SkillDefinition, skillBundlesDir s // other file's bytes are read only if a bundle zip needs them, and are // never treated as UTF-8. if relPath == "SKILL.md" || needAllContent { - content, err := os.ReadFile(path) + content, err := os.ReadFile(absPath) if err != nil { issues = append(issues, Issue{File: fileID, Message: "cannot read file: " + err.Error()}) - return nil + continue } sf.content = content } files = append(files, sf) - - return nil - }) - if walkErr != nil { - return nil, []Issue{{File: skill.Dir, Message: fmt.Sprintf("cannot read skill %q: %s", skill.Slug, walkErr.Error())}} } if len(files) > MaxSkillFiles { @@ -218,22 +303,45 @@ func readSkillBundle(projectDir string, skill SkillDefinition, skillBundlesDir s issues = append(issues, validateSkillFrontmatter(skill.Slug, filepath.Join(skill.Dir, "SKILL.md"), skillMD.content)...) } - treeHash, hashIssues := resolveSkillTreeHash(projectDir, skill) + return files, issues +} + +// readSkillBundle validates a skill's content and, only if its git tree hash +// resolves, returns the compiled skill: its slug and the git tree hash of its +// directory at HEAD. When skillBundlesDir is non-empty, it also writes +// /.zip with every file's raw bytes. Git state is +// advisory only for local compile (the authoritative compile job always runs +// on a clean clone where everything is committed): a genuine content problem +// (bad frontmatter, oversize file, symlink, ...) is a hard error and returns +// nil, but an unresolved tree hash (not a git repository, or the skill +// directory never committed) is reported as a warning in the returned issues +// and the skill is simply omitted (nil, warning-only issues) rather than +// failing compile. +func readSkillBundle(projectDir string, skill SkillDefinition, skillBundlesDir string) (*CompiledSkill, []Issue) { + needAllContent := skillBundlesDir != "" + + files, issues := validateSkillContent(projectDir, skill, needAllContent) + + treeHash, resolved, hashIssues := resolveSkillTreeHash(projectDir, skill) issues = append(issues, hashIssues...) - if len(issues) > 0 { + if errs, _ := PartitionIssues(issues); len(errs) > 0 { return nil, issues } + if !resolved { + return nil, issues // git state unresolved: warning only, skill omitted + } + sort.Slice(files, func(i, j int) bool { return files[i].relPath < files[j].relPath }) if needAllContent { if err := writeSkillBundleZip(skillBundlesDir, treeHash, files); err != nil { - return nil, []Issue{{File: skill.Dir, Message: fmt.Sprintf("cannot write bundle zip for skill %q: %s", skill.Slug, err.Error())}} + return nil, append(issues, Issue{File: skill.Dir, Message: fmt.Sprintf("cannot write bundle zip for skill %q: %s", skill.Slug, err.Error())}) } } - return &CompiledSkill{Slug: skill.Slug, TreeHash: treeHash}, nil + return &CompiledSkill{Slug: skill.Slug, TreeHash: treeHash}, issues } // writeSkillBundleZip writes every file in files to @@ -283,58 +391,73 @@ func runGit(dir string, args ...string) (string, error) { return strings.TrimSpace(string(out)), nil } +// skillDirRelToRepo resolves a skill directory's path relative to a git +// repository root, evaluating symlinks first (macOS puts TMPDIR behind a +// /var -> /private/var symlink) so the two paths are comparable instead of +// producing a bogus relative path that never matches anything at HEAD. git +// itself already resolves symlinks in `rev-parse --show-toplevel`, so only +// skillDir needs the same treatment here. +func skillDirRelToRepo(repoRoot, skillDir string) (string, error) { + realSkillDir, err := filepath.EvalSymlinks(skillDir) + if err != nil { + return "", err + } + + rel, err := filepath.Rel(repoRoot, realSkillDir) + if err != nil { + return "", err + } + + return filepath.ToSlash(rel), nil +} + // resolveSkillTreeHash computes the compiled tree hash for a skill directory: // the git tree object hash of the directory at HEAD (git rev-parse -// HEAD:). If the project isn't inside a git repository, -// or the skill directory has never been committed, that's a validation issue -// telling the user to commit it first. If the skill directory has uncommitted -// changes, the hash still resolves (it reflects HEAD), but that's reported as -// an issue too - Issue has no severity levels, so this is a hard issue, not -// merely a warning; the authoritative compile always runs on a clean clone at -// a fixed commit, so in practice this only affects local runs. -func resolveSkillTreeHash(projectDir string, skill SkillDefinition) (string, []Issue) { +// HEAD:). Git state is advisory only, never a hard error: +// local compile is a convenience ahead of the authoritative compile job, +// which always runs on a clean clone where every directory is committed, so +// nothing uncommitted locally can ever reach a deploy. If the project isn't +// inside a git repository, or the skill directory has never been committed, +// resolved is false and the returned issue is a warning telling the caller to +// skip the skill (readSkillBundle omits it from the compiled config rather +// than failing compile). If the skill directory has uncommitted changes, the +// hash still resolves (it reflects HEAD), but that's reported as a warning +// too. +func resolveSkillTreeHash(projectDir string, skill SkillDefinition) (hash string, resolved bool, issues []Issue) { absSkillDir := filepath.Join(projectDir, skill.Dir) repoRoot, err := runGit(absSkillDir, "rev-parse", "--show-toplevel") if err != nil { - return "", []Issue{{ - File: skill.Dir, - Message: fmt.Sprintf("skill %q is not inside a git repository; commit the project, including the skill directory, before compiling", skill.Slug), + return "", false, []Issue{{ + Severity: SeverityWarning, + File: skill.Dir, + Message: fmt.Sprintf("skill %q skipped: not inside a git repository; commit the skill directory (%s) to compile it - the tree hash is derived from git", skill.Slug, skill.Dir), }} } - // git resolves symlinks in --show-toplevel; resolve absSkillDir the same - // way (macOS puts TMPDIR behind a /var -> /private/var symlink) so the two - // paths are comparable instead of producing a bogus "../../.." relative - // path that still resolves but never matches HEAD. - realSkillDir, err := filepath.EvalSymlinks(absSkillDir) - if err != nil { - return "", []Issue{{File: skill.Dir, Message: "cannot resolve skill directory path: " + err.Error()}} - } - - relToRepo, err := filepath.Rel(repoRoot, realSkillDir) + relToRepo, err := skillDirRelToRepo(repoRoot, absSkillDir) if err != nil { - return "", []Issue{{File: skill.Dir, Message: "cannot resolve skill directory relative to the git repository root: " + err.Error()}} + return "", false, []Issue{{Severity: SeverityWarning, File: skill.Dir, Message: "cannot resolve skill directory relative to the git repository root: " + err.Error()}} } - relToRepo = filepath.ToSlash(relToRepo) - hash, err := runGit(repoRoot, "rev-parse", "HEAD:"+relToRepo) - if err != nil || !skillTreeHashPattern.MatchString(hash) { - return "", []Issue{{ - File: skill.Dir, - Message: fmt.Sprintf("skill %q has never been committed to git; commit the skill directory (%s) before compiling", skill.Slug, relToRepo), + headHash, err := runGit(repoRoot, "rev-parse", "HEAD:"+relToRepo) + if err != nil || !skillTreeHashPattern.MatchString(headHash) { + return "", false, []Issue{{ + Severity: SeverityWarning, + File: skill.Dir, + Message: fmt.Sprintf("skill %q skipped: commit the skill directory (%s) to compile it - the tree hash is derived from git", skill.Slug, relToRepo), }} } - var issues []Issue if status, statusErr := runGit(repoRoot, "status", "--porcelain", "--", relToRepo); statusErr == nil && status != "" { issues = append(issues, Issue{ - File: skill.Dir, - Message: fmt.Sprintf("skill %q has uncommitted changes; the compiled tree hash reflects the last commit (HEAD), not the working tree", skill.Slug), + Severity: SeverityWarning, + File: skill.Dir, + Message: fmt.Sprintf("skill %q has uncommitted changes; the tree hash reflects the last commit (HEAD), not your working tree - commit before pushing", skill.Slug), }) } - return hash, issues + return headHash, true, issues } // validateSkillFilePath ports the path-level checks from the server's diff --git a/projects/skills_test.go b/projects/skills_test.go index d185ca7..c662b14 100644 --- a/projects/skills_test.go +++ b/projects/skills_test.go @@ -319,7 +319,13 @@ func TestCompileSkillFileCountCapRaisedTo200(t *testing.T) { } } -func TestCompileSkillNotGitRepoRejected(t *testing.T) { +// TestCompileSkillNotGitRepoSkipsWithWarning checks that compiling outside a +// git repository never fails: a skill whose tree hash can't be resolved is +// reported as a warning and simply omitted from the compiled config. Local +// compile is advisory - the authoritative compile job always runs on a clean +// clone where everything is committed - so nothing about git state may block +// it. +func TestCompileSkillNotGitRepoSkipsWithWarning(t *testing.T) { dir := t.TempDir() writeFixtureProject(t, dir) @@ -331,13 +337,28 @@ func TestCompileSkillNotGitRepoRejected(t *testing.T) { t.Fatal(err) } - _, issues := Compile(dir) - if !findIssue(issues, "git repository") { - t.Fatalf("expected a not-a-git-repository issue, got: %+v", issues) + res, issues := Compile(dir) + if res == nil { + t.Fatalf("expected compile to succeed despite the skipped skill, got nil result with issues: %+v", issues) + } + if len(res.Config.Skills) != 0 { + t.Fatalf("expected the unresolved skill to be omitted, got: %+v", res.Config.Skills) + } + if strings.Contains(string(res.ConfigJSON), `"skills"`) { + t.Fatalf("expected no skills key when the only skill is skipped, got: %s", res.ConfigJSON) + } + if !findIssue(issues, "not inside a git repository") { + t.Fatalf("expected a not-a-git-repository warning, got: %+v", issues) + } + if errs, _ := PartitionIssues(issues); len(errs) != 0 { + t.Fatalf("expected no error-severity issues, got: %+v", errs) } } -func TestCompileSkillNeverCommittedRejected(t *testing.T) { +// TestCompileSkillNeverCommittedSkipsWithWarning mirrors +// TestCompileSkillNotGitRepoSkipsWithWarning for a skill directory that is +// inside a git repository but was never committed. +func TestCompileSkillNeverCommittedSkipsWithWarning(t *testing.T) { dir := t.TempDir() writeFixtureProject(t, dir) initGitRepo(t, dir) // commits project.json but not the skill created below @@ -350,13 +371,25 @@ func TestCompileSkillNeverCommittedRejected(t *testing.T) { t.Fatal(err) } - _, issues := Compile(dir) + res, issues := Compile(dir) + if res == nil { + t.Fatalf("expected compile to succeed despite the skipped skill, got nil result with issues: %+v", issues) + } + if len(res.Config.Skills) != 0 { + t.Fatalf("expected the never-committed skill to be omitted, got: %+v", res.Config.Skills) + } if !findIssue(issues, "commit the skill directory") { - t.Fatalf("expected a never-committed issue, got: %+v", issues) + t.Fatalf("expected a never-committed warning, got: %+v", issues) + } + if errs, _ := PartitionIssues(issues); len(errs) != 0 { + t.Fatalf("expected no error-severity issues, got: %+v", errs) } } -func TestCompileSkillUncommittedChangesRejected(t *testing.T) { +// TestCompileSkillUncommittedChangesWarns checks that a committed-then-locally +// -modified skill directory still compiles (the tree hash reflects HEAD), but +// reports the discrepancy as a warning, not an error. +func TestCompileSkillUncommittedChangesWarns(t *testing.T) { dir := t.TempDir() writeFixtureProject(t, dir) @@ -373,13 +406,173 @@ func TestCompileSkillUncommittedChangesRejected(t *testing.T) { // Edit after committing: HEAD has the original content, the working tree // does not - the compiled hash must still resolve (it reflects HEAD), but - // this must be reported. + // this must be reported as a warning. if err := os.WriteFile(skillMD, []byte("---\nname: Dirty\ndescription: has local edits.\n---\nBody, edited.\n"), 0644); err != nil { t.Fatal(err) } - _, issues := Compile(dir) + res, issues := Compile(dir) + if res == nil { + t.Fatalf("expected compile to succeed with only a warning, got nil result with issues: %+v", issues) + } + if len(res.Config.Skills) != 1 || res.Config.Skills[0].TreeHash == "" { + t.Fatalf("expected the dirty skill to still compile with a tree hash, got: %+v", res.Config.Skills) + } if !findIssue(issues, "uncommitted changes") { - t.Fatalf("expected an uncommitted-changes issue, got: %+v", issues) + t.Fatalf("expected an uncommitted-changes warning, got: %+v", issues) + } + for _, issue := range issues { + if !issue.IsWarning() { + t.Fatalf("expected every issue to be a warning, got an error: %+v", issue) + } + } +} + +// TestValidateGitignoredDotDSStoreClean checks that a gitignored .DS_Store +// inside a skill directory is invisible to validate: it is excluded by the +// git-lens file listing (listSkillFiles), so it never reaches the dotfile +// check at all. The artifact that ships is the git tree the platform compile +// job clones at a commit, so a gitignored file is phantom content. +func TestValidateGitignoredDotDSStoreClean(t *testing.T) { + dir := t.TempDir() + writeFixtureProject(t, dir) + + skillDir := filepath.Join(dir, "src", "skills", "clean") + if err := os.MkdirAll(skillDir, 0755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(skillDir, "SKILL.md"), []byte("---\nname: Clean\ndescription: has a gitignored .DS_Store.\n---\nBody.\n"), 0644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, ".gitignore"), []byte(".DS_Store\n"), 0644); err != nil { + t.Fatal(err) + } + + initGitRepo(t, dir) // commits project.json, the skill, and .gitignore + + // Created after the commit, and ignored: never enters the index. + if err := os.WriteFile(filepath.Join(skillDir, ".DS_Store"), []byte("junk"), 0644); err != nil { + t.Fatal(err) + } + + if issues := Validate(dir); len(issues) != 0 { + t.Fatalf("expected no issues for a gitignored .DS_Store, got: %+v", issues) + } + + bundlesDir := t.TempDir() + res, issues := CompileWithSkillBundles(dir, bundlesDir) + if len(issues) != 0 { + t.Fatalf("expected no compile issues, got: %+v", issues) + } + + zipPath := filepath.Join(bundlesDir, res.Config.Skills[0].TreeHash+".zip") + zr, err := zip.OpenReader(zipPath) + if err != nil { + t.Fatalf("open bundle zip: %v", err) + } + defer zr.Close() + + for _, f := range zr.File { + if f.Name == ".DS_Store" { + t.Fatalf("expected .DS_Store to be excluded from the bundle zip, got entries: %v", zr.File) + } + } +} + +// TestValidateNonIgnoredDotDSStoreRejected checks that a .DS_Store that is +// NOT gitignored still trips the dotfile rule: the server rejects it at +// publish, so a tracked or untracked-but-not-ignored dotfile must still be a +// validation error. +func TestValidateNonIgnoredDotDSStoreRejected(t *testing.T) { + dir := t.TempDir() + writeFixtureProject(t, dir) + + skillDir := filepath.Join(dir, "src", "skills", "unclean") + if err := os.MkdirAll(skillDir, 0755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(skillDir, "SKILL.md"), []byte("---\nname: Unclean\ndescription: has a tracked .DS_Store.\n---\nBody.\n"), 0644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(skillDir, ".DS_Store"), []byte("junk"), 0644); err != nil { + t.Fatal(err) + } + + initGitRepo(t, dir) // commits the .DS_Store too - it is tracked, not ignored + + if !findIssue(Validate(dir), "dotfile") { + t.Fatalf("expected a dotfile issue for a non-ignored .DS_Store, got: %+v", Validate(dir)) + } +} + +// TestValidateNeverCommittedSkillHasNoGitIssues checks that validate never +// mentions git state: a skill directory that has never been committed +// validates its content (a bad frontmatter is still reported) without any +// "not a git repository" / "commit" noise. +func TestValidateNeverCommittedSkillHasNoGitIssues(t *testing.T) { + dir := t.TempDir() + writeFixtureProject(t, dir) + initGitRepo(t, dir) // commits project.json but not the skill created below + + skillDir := filepath.Join(dir, "src", "skills", "wip") + if err := os.MkdirAll(skillDir, 0755); err != nil { + t.Fatal(err) + } + // Missing "description" in frontmatter: a genuine content problem that + // must still surface. + if err := os.WriteFile(filepath.Join(skillDir, "SKILL.md"), []byte("---\nname: WIP\n---\nBody.\n"), 0644); err != nil { + t.Fatal(err) + } + + issues := Validate(dir) + if !findIssue(issues, "description") { + t.Fatalf("expected the bad-frontmatter issue to still be reported, got: %+v", issues) + } + if findIssue(issues, "git") || findIssue(issues, "commit") { + t.Fatalf("expected no git-related issues from validate, got: %+v", issues) + } +} + +// TestValidateOutsideGitRepoFallsBackToFilesystemWalk checks that validate +// works in a directory that is not a git repository at all: listSkillFiles +// falls back to a plain filesystem walk. +func TestValidateOutsideGitRepoFallsBackToFilesystemWalk(t *testing.T) { + dir := t.TempDir() + writeFixtureProject(t, dir) + + skillDir := filepath.Join(dir, "src", "skills", "no-repo") + if err := os.MkdirAll(skillDir, 0755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(skillDir, "SKILL.md"), []byte("---\nname: No Repo\ndescription: not a git repo at all.\n---\nBody.\n"), 0644); err != nil { + t.Fatal(err) + } + + if issues := Validate(dir); len(issues) != 0 { + t.Fatalf("expected no issues validating outside a git repository, got: %+v", issues) + } +} + +// TestPartitionIssuesOnlyWarningsMeansSuccess exercises the seam cmd/project +// uses to decide exit codes: a run with only warning-severity issues has zero +// errors, which the CLI treats as success (exit 0). +func TestPartitionIssuesOnlyWarningsMeansSuccess(t *testing.T) { + issues := []Issue{ + {File: "src/skills/dirty", Message: "has uncommitted changes", Severity: SeverityWarning}, + {File: "src/skills/dirty", Message: "also skipped", Severity: SeverityWarning}, + } + + errs, warnings := PartitionIssues(issues) + if len(errs) != 0 { + t.Fatalf("expected no errors, got: %+v", errs) + } + if len(warnings) != 2 { + t.Fatalf("expected 2 warnings, got: %+v", warnings) + } + + mixed := append(issues, Issue{File: "project.json", Message: "missing name"}) + errs, _ = PartitionIssues(mixed) + if len(errs) != 1 { + t.Fatalf("expected exactly 1 error in a mixed set, got: %+v", errs) } } diff --git a/projects/types.go b/projects/types.go index f1731ba..72b792d 100644 --- a/projects/types.go +++ b/projects/types.go @@ -1,10 +1,40 @@ package projects +// Severity classifies an Issue as blocking or advisory. The zero value is +// SeverityError, so the many existing Issue{} literals across the package +// need no changes to keep meaning "this fails validation/compile". +type Severity string + +const ( + SeverityError Severity = "" + SeverityWarning Severity = "warning" +) + // Issue is a single validation problem tied to a file and a location within it. type Issue struct { - File string `json:"file"` - Path string `json:"path,omitempty"` - Message string `json:"message"` + File string `json:"file"` + Path string `json:"path,omitempty"` + Message string `json:"message"` + Severity Severity `json:"severity,omitempty"` +} + +// IsWarning reports whether the issue is advisory rather than blocking. +func (i Issue) IsWarning() bool { + return i.Severity == SeverityWarning +} + +// PartitionIssues splits issues into blocking errors and advisory warnings, +// preserving order within each group. Callers that only care whether a run +// should fail can check len(errors) > 0. +func PartitionIssues(issues []Issue) (errors, warnings []Issue) { + for _, issue := range issues { + if issue.IsWarning() { + warnings = append(warnings, issue) + } else { + errors = append(errors, issue) + } + } + return errors, warnings } // ProjectDefinition is the parsed project.json.