diff --git a/README.md b/README.md index feacc9c..7537800 100644 --- a/README.md +++ b/README.md @@ -703,6 +703,7 @@ These checks validate conformance with the [Agent Skills specification](https:// - **Structure**: `SKILL.md` exists; only recognized directories (`scripts/`, `references/`, `assets/`); no deep nesting; no orphan files - **Frontmatter**: required fields (`name`, `description`) are present and valid; `name` is lowercase alphanumeric with hyphens (1-64 chars) and matches the directory name; optional fields (`license`, `compatibility`, `metadata`, `allowed-tools`) conform to expected types and lengths; unrecognized fields are flagged +- **Read limit**: no single file is read past 8 MiB, so a pathological file cannot exhaust memory. A larger file's token count covers only its first 8 MiB and is flagged as such; the unclosed-fence and orphan checks skip it with a warning, since they need the whole file to be right **Extraneous file detection** - Files like `README.md`, `CHANGELOG.md`, and `LICENSE` are flagged at the skill root -- these are for human readers, not agents, and may be loaded into the context window unnecessarily diff --git a/report/json.go b/report/json.go index 2ad5c9f..d797fb4 100644 --- a/report/json.go +++ b/report/json.go @@ -42,8 +42,9 @@ type jsonTokenCounts struct { } type jsonTokenCount struct { - File string `json:"file"` - Tokens int `json:"tokens"` + File string `json:"file"` + Tokens int `json:"tokens"` + Truncated bool `json:"truncated,omitempty"` } type jsonMultiReport struct { @@ -77,7 +78,7 @@ func buildJSONReport(r *types.Report, perFile bool) jsonReport { Files: make([]jsonTokenCount, len(r.TokenCounts)), } for i, c := range r.TokenCounts { - tc.Files[i] = jsonTokenCount{File: c.File, Tokens: c.Tokens} + tc.Files[i] = jsonTokenCount{File: c.File, Tokens: c.Tokens, Truncated: c.Truncated} tc.Total += c.Tokens } out.TokenCounts = tc @@ -88,7 +89,7 @@ func buildJSONReport(r *types.Report, perFile bool) jsonReport { Files: make([]jsonTokenCount, len(r.OtherTokenCounts)), } for i, c := range r.OtherTokenCounts { - otc.Files[i] = jsonTokenCount{File: c.File, Tokens: c.Tokens} + otc.Files[i] = jsonTokenCount{File: c.File, Tokens: c.Tokens, Truncated: c.Truncated} otc.Total += c.Tokens } out.OtherTokenCounts = otc diff --git a/structure/bounded_reads_test.go b/structure/bounded_reads_test.go new file mode 100644 index 0000000..ffee08b --- /dev/null +++ b/structure/bounded_reads_test.go @@ -0,0 +1,115 @@ +package structure + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/agent-ecosystem/skill-validator/types" + "github.com/agent-ecosystem/skill-validator/util" +) + +// writeSparseFile creates a file of the given size without writing its +// bytes, so an over-limit fixture costs no disk (issue #87). +func writeSparseFile(t *testing.T, dir, rel string, size int64) { + t.Helper() + path := filepath.Join(dir, rel) + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatal(err) + } + f, err := os.Create(path) + if err != nil { + t.Fatal(err) + } + if err := f.Truncate(size); err != nil { + t.Fatal(err) + } + if err := f.Close(); err != nil { + t.Fatal(err) + } +} + +func TestCheckTokens_TruncatedFile(t *testing.T) { + saved := maxTokenizedFileBytes + maxTokenizedFileBytes = 32 + t.Cleanup(func() { maxTokenizedFileBytes = saved }) + + dir := t.TempDir() + content := strings.Repeat("word ", 40) // 200 bytes + writeFile(t, dir, "references/big.md", content) + writeFile(t, dir, "assets/template.md", content) + writeFile(t, dir, "references/small.md", "tiny") + + results, counts, _ := CheckTokens(dir, "Body.", Options{}) + + requireResultContaining(t, results, types.Warning, + "references/big.md is larger than 32 bytes; its token count covers only the first 32 bytes") + requireResultContaining(t, results, types.Warning, + "assets/template.md is larger than 32 bytes") + requireNoResultContaining(t, results, types.Warning, "references/small.md is larger") + + enc, err := getEncoder() + if err != nil { + t.Fatal(err) + } + prefixTokens, _, _ := enc.Encode(content[:32]) + for _, c := range counts { + switch c.File { + case "references/big.md", "assets/template.md": + if !c.Truncated { + t.Errorf("%s: Truncated = false, want true", c.File) + } + if c.Tokens != len(prefixTokens) { + t.Errorf("%s: Tokens = %d, want %d (count of the 32-byte prefix)", c.File, c.Tokens, len(prefixTokens)) + } + case "references/small.md": + if c.Truncated { + t.Errorf("%s: Truncated = true, want false", c.File) + } + } + } +} + +func TestCheckMarkdown_OversizedReferenceSkipped(t *testing.T) { + dir := t.TempDir() + writeSparseFile(t, dir, "references/big.md", util.MaxSkillFileBytes+1) + writeFile(t, dir, "references/broken.md", "# Ref\n```\nunclosed") + + results := CheckMarkdown(dir, "Clean body.") + + requireResultContaining(t, results, types.Warning, + "references/big.md is larger than 8 MiB; skipped the unclosed code fence check") + requireNoResultContaining(t, results, types.Error, "references/big.md") + // The other files are still checked. + requireResultContaining(t, results, types.Error, "references/broken.md has an unclosed code fence") +} + +func TestCheckOrphanFiles_OversizedFileNotScanned(t *testing.T) { + dir := t.TempDir() + writeSparseFile(t, dir, "references/big.md", util.MaxSkillFileBytes+1) + writeFile(t, dir, "references/leaf.md", "leaf content") + + // SKILL.md reaches big.md; whatever big.md links to is unknowable. + results := CheckOrphanFiles(dir, "See references/big.md", Options{}) + + requireResultContaining(t, results, types.Warning, + "references/big.md is larger than 8 MiB and was not scanned for references") + requireResultContaining(t, results, types.Warning, + "potentially unreferenced file: references/leaf.md") + requireNoResultContaining(t, results, types.Warning, "potentially unreferenced file: references/big.md") + + // The unscanned warning precedes the orphan warnings it qualifies. + unscannedAt, orphanAt := -1, -1 + for i, r := range results { + switch { + case strings.Contains(r.Message, "was not scanned"): + unscannedAt = i + case strings.Contains(r.Message, "potentially unreferenced"): + orphanAt = i + } + } + if unscannedAt < 0 || orphanAt < 0 || unscannedAt > orphanAt { + t.Errorf("unscanned warning at %d, orphan warning at %d; want unscanned first", unscannedAt, orphanAt) + } +} diff --git a/structure/markdown.go b/structure/markdown.go index dc229b0..b3120a9 100644 --- a/structure/markdown.go +++ b/structure/markdown.go @@ -1,6 +1,7 @@ package structure import ( + "errors" "os" "path/filepath" "strings" @@ -39,11 +40,16 @@ func CheckMarkdown(dir, body string) []types.Result { if !strings.HasSuffix(strings.ToLower(entry.Name()), ".md") { continue } + relPath := "references/" + entry.Name() data, err := util.SafeReadFile(dir, filepath.Join(refsDir, entry.Name())) + if errors.Is(err, util.ErrFileTooLarge) { + results = append(results, ctx.WarnFilef(relPath, + "%s is larger than %s; skipped the unclosed code fence check", relPath, util.FormatByteSize(util.MaxSkillFileBytes))) + continue + } if err != nil { continue } - relPath := "references/" + entry.Name() if line, ok := FindUnclosedFence(string(data)); ok { results = append(results, ctx.ErrorAtLinef(relPath, line, "%s has an unclosed code fence starting at line %d — this may cause agents to misinterpret everything after it as code", relPath, line)) diff --git a/structure/orphans.go b/structure/orphans.go index b57dbda..fdbab8e 100644 --- a/structure/orphans.go +++ b/structure/orphans.go @@ -1,6 +1,7 @@ package structure import ( + "errors" "fmt" "os" "path/filepath" @@ -60,6 +61,7 @@ func CheckOrphanFiles(dir, body string, opts Options) []types.Result { missingExtension := make(map[string]bool) // relPath → true if matched only without file extension scannedRootFiles := make(map[string]bool) scannedInitFiles := make(map[string]bool) + unscanned := make(map[string]bool) // relPath → true if too large to scan for references // Seed the queue with the SKILL.md body. queue := []queueItem{{text: body, source: "SKILL.md"}} @@ -89,6 +91,8 @@ func CheckOrphanFiles(dir, body string, opts Options) []types.Result { data, err := util.SafeReadFile(dir, filepath.Join(dir, rf)) if err == nil { queue = append(queue, queueItem{text: string(data), source: rf}) + } else if errors.Is(err, util.ErrFileTooLarge) { + unscanned[rf] = true } } } @@ -100,14 +104,14 @@ func CheckOrphanFiles(dir, body string, opts Options) []types.Result { continue } if containsReference(item.text, sourceDir, relPath) { - markReached(relPath, item.source, dir, &queue, reached, reachedFrom) + markReached(relPath, item.source, dir, &queue, reached, reachedFrom, unscanned) } else if isPython && pythonImportReaches(item.text, item.source, relPath) { // Python import resolution takes priority over the extensionless // fallback so that normal import statements (e.g., "from helpers // import merge") don't trigger a "missing extension" warning. - markReached(relPath, item.source, dir, &queue, reached, reachedFrom) + markReached(relPath, item.source, dir, &queue, reached, reachedFrom, unscanned) } else if containsReferenceWithoutExtension(item.text, sourceDir, relPath) { - markReached(relPath, item.source, dir, &queue, reached, reachedFrom) + markReached(relPath, item.source, dir, &queue, reached, reachedFrom, unscanned) missingExtension[relPath] = true } } @@ -126,11 +130,20 @@ func CheckOrphanFiles(dir, body string, opts Options) []types.Result { data, err := util.SafeReadFile(dir, filepath.Join(dir, initPath)) if err == nil { queue = append(queue, queueItem{text: string(data), source: initPath}) + } else if errors.Is(err, util.ErrFileTooLarge) { + unscanned[initPath] = true } } } } + // Files too large to scan cannot vouch for anything they link to, so + // say so before any orphan warnings those links would have prevented. + for _, relPath := range util.SortedKeys(unscanned) { + results = append(results, ctx.WarnFile(relPath, + fmt.Sprintf("%s is larger than %s and was not scanned for references — files it links to may be reported as unreferenced", relPath, util.FormatByteSize(util.MaxSkillFileBytes)))) + } + // Build results per directory. for _, d := range orderedRecognizedDirs { dirFiles := filesInDir(inventory, d) @@ -303,7 +316,7 @@ func isPathWordByte(b byte) bool { // markReached marks a file as reached, reads it if it's a text file, and // enqueues its content for further BFS scanning. -func markReached(relPath, source, dir string, queue *[]queueItem, reached map[string]bool, reachedFrom map[string]string) { +func markReached(relPath, source, dir string, queue *[]queueItem, reached map[string]bool, reachedFrom map[string]string, unscanned map[string]bool) { reached[relPath] = true reachedFrom[relPath] = source @@ -311,6 +324,8 @@ func markReached(relPath, source, dir string, queue *[]queueItem, reached map[st data, err := util.SafeReadFile(dir, filepath.Join(dir, relPath)) if err == nil { *queue = append(*queue, queueItem{text: string(data), source: relPath}) + } else if errors.Is(err, util.ErrFileTooLarge) { + unscanned[relPath] = true } } } diff --git a/structure/tokens.go b/structure/tokens.go index debbfe7..6de5cec 100644 --- a/structure/tokens.go +++ b/structure/tokens.go @@ -12,17 +12,29 @@ import ( "github.com/agent-ecosystem/skill-validator/util" ) -const maxTokenizedFileBytes = 8 * 1024 * 1024 +// maxTokenizedFileBytes bounds how much of any one file is read for token +// counting. Larger files are counted on this prefix only and flagged as +// truncated. A variable so tests can lower it. +var maxTokenizedFileBytes = util.MaxSkillFileBytes + +// readFileWithCap reads at most maxTokenizedFileBytes of path, never loading +// the rest into memory (issue #87). truncated reports whether the file had +// more. +func readFileWithCap(root, path string) (data []byte, truncated bool, err error) { + return util.SafeReadFileN(root, path, maxTokenizedFileBytes) +} -func readFileWithCap(root, path string) ([]byte, error) { - data, err := util.SafeReadFile(root, path) - if err != nil { - return nil, err - } - if len(data) > maxTokenizedFileBytes { - data = data[:maxTokenizedFileBytes] +// truncationNotes flags every count that covers only a prefix of its file. +func truncationNotes(ctx types.ResultContext, counts []types.TokenCount) []types.Result { + var results []types.Result + for _, tc := range counts { + if tc.Truncated { + limit := util.FormatByteSize(maxTokenizedFileBytes) + results = append(results, ctx.WarnFilef(tc.File, + "%s is larger than %s; its token count covers only the first %s", tc.File, limit, limit)) + } } - return data, nil + return results } const ( @@ -99,7 +111,7 @@ func CheckTokens(dir, body string, opts Options) ([]types.Result, []types.TokenC continue } path := filepath.Join(refsDir, entry.Name()) - data, err := readFileWithCap(dir, path) + data, truncated, err := readFileWithCap(dir, path) if err != nil { relPath := "references/" + entry.Name() results = append(results, ctx.WarnFilef(relPath, "could not read %s: %v", relPath, err)) @@ -109,8 +121,9 @@ func CheckTokens(dir, body string, opts Options) ([]types.Result, []types.TokenC fileTokens := len(tokens) relPath := "references/" + entry.Name() counts = append(counts, types.TokenCount{ - File: relPath, - Tokens: fileTokens, + File: relPath, + Tokens: fileTokens, + Truncated: truncated, }) refTotal += fileTokens @@ -203,6 +216,9 @@ func CheckTokens(dir, body string, opts Options) ([]types.Result, []types.TokenC assetCounts := countAssetFiles(dir, enc) counts = append(counts, assetCounts...) + results = append(results, truncationNotes(ctx, counts)...) + results = append(results, truncationNotes(ctx, otherCounts)...) + return results, counts, otherCounts } @@ -270,13 +286,13 @@ func countAssetFiles(dir string, enc tokenizer.Codec) []types.TokenCount { if !textAssetExtensions[ext] { return nil } - data, err := readFileWithCap(dir, path) + data, truncated, err := readFileWithCap(dir, path) if err != nil { return nil } rel, _ := filepath.Rel(dir, path) tokens, _, _ := enc.Encode(string(data)) - counts = append(counts, types.TokenCount{File: filepath.ToSlash(rel), Tokens: len(tokens)}) + counts = append(counts, types.TokenCount{File: filepath.ToSlash(rel), Tokens: len(tokens), Truncated: truncated}) return nil }) @@ -353,12 +369,12 @@ func countOtherFiles(dir string, enc tokenizer.Codec, opts Options, exclusions * if binaryExtensions[strings.ToLower(filepath.Ext(name))] { continue } - data, err := readFileWithCap(dir, filepath.Join(dir, name)) + data, truncated, err := readFileWithCap(dir, filepath.Join(dir, name)) if err != nil { continue } tokens, _, _ := enc.Encode(string(data)) - counts = append(counts, types.TokenCount{File: name, Tokens: len(tokens)}) + counts = append(counts, types.TokenCount{File: name, Tokens: len(tokens), Truncated: truncated}) } } @@ -398,12 +414,12 @@ func countFilesInDir(rootDir, dirName string, enc tokenizer.Codec, exclusions *t if binaryExtensions[strings.ToLower(filepath.Ext(info.Name()))] { return nil } - data, err := readFileWithCap(rootDir, path) + data, truncated, err := readFileWithCap(rootDir, path) if err != nil { return nil } tokens, _, _ := enc.Encode(string(data)) - counts = append(counts, types.TokenCount{File: filepath.ToSlash(rel), Tokens: len(tokens)}) + counts = append(counts, types.TokenCount{File: filepath.ToSlash(rel), Tokens: len(tokens), Truncated: truncated}) return nil }) @@ -432,12 +448,12 @@ func countRootFiles(dir string, enc tokenizer.Codec) []types.TokenCount { if binaryExtensions[strings.ToLower(filepath.Ext(name))] { continue } - data, err := readFileWithCap(dir, filepath.Join(dir, name)) + data, truncated, err := readFileWithCap(dir, filepath.Join(dir, name)) if err != nil { continue } tokens, _, _ := enc.Encode(string(data)) - counts = append(counts, types.TokenCount{File: name, Tokens: len(tokens)}) + counts = append(counts, types.TokenCount{File: name, Tokens: len(tokens), Truncated: truncated}) } return counts } diff --git a/types/types.go b/types/types.go index fec8623..68679e4 100644 --- a/types/types.go +++ b/types/types.go @@ -42,10 +42,13 @@ type Result struct { Line int // 0 = no line info } -// TokenCount holds the token count for a single file. +// TokenCount holds the token count for a single file. Truncated marks a +// file larger than the read limit, whose count covers only the prefix that +// was read. type TokenCount struct { - File string - Tokens int + File string + Tokens int + Truncated bool } // ContentReport holds content quality metrics computed by the content analyzer. diff --git a/util/util.go b/util/util.go index 9ba3b89..7395f52 100644 --- a/util/util.go +++ b/util/util.go @@ -6,6 +6,7 @@ package util import ( "errors" "fmt" + "io" "math" "os" "path/filepath" @@ -15,27 +16,91 @@ import ( var ErrUnsafeFile = errors.New("refusing to read unsafe file") -// SafeReadFile reads a file from an untrusted skill package rooted at root. -// It refuses non-regular files (symlinks, devices, pipes) and paths that -// resolve outside root after following symlinks in any parent directory, so -// a symlinked file or a symlinked directory inside the package cannot leak -// content from elsewhere on the machine. +// ErrFileTooLarge reports a file in a skill package larger than the read +// limit. Callers that need the whole file (link scanning, fence matching) +// skip the file and say so; the token counter reads a bounded prefix +// through SafeReadFileN instead. +var ErrFileTooLarge = errors.New("file exceeds the read limit") + +// MaxSkillFileBytes is the most the validator reads from any single file in +// a skill package (issue #87). A pathological multi-gigabyte file is never +// loaded whole: SafeReadFile refuses it, and SafeReadFileN returns only +// this many bytes. +const MaxSkillFileBytes int64 = 8 << 20 + +// FormatByteSize renders a byte count for messages: whole MiB or KiB where +// exact ("8 MiB"), plain bytes otherwise ("100 bytes"). +func FormatByteSize(n int64) string { + switch { + case n >= 1<<20 && n%(1<<20) == 0: + return fmt.Sprintf("%d MiB", n>>20) + case n >= 1<<10 && n%(1<<10) == 0: + return fmt.Sprintf("%d KiB", n>>10) + default: + return fmt.Sprintf("%d bytes", n) + } +} + +// SafeReadFile reads a whole file from an untrusted skill package rooted at +// root. It refuses non-regular files (symlinks, devices, pipes) and paths +// that resolve outside root after following symlinks in any parent +// directory, so a symlinked file or a symlinked directory inside the +// package cannot leak content from elsewhere on the machine. It also +// refuses, with ErrFileTooLarge, any file larger than MaxSkillFileBytes, +// without reading past that limit. func SafeReadFile(root, path string) ([]byte, error) { - info, err := os.Lstat(path) + data, truncated, err := SafeReadFileN(root, path, MaxSkillFileBytes) if err != nil { return nil, err } + if truncated { + return nil, fmt.Errorf("%w: %s is larger than %s", ErrFileTooLarge, path, FormatByteSize(MaxSkillFileBytes)) + } + return data, nil +} + +// SafeReadFileN is SafeReadFile with an explicit byte limit. It reads at +// most limit bytes and reports whether the file had more; the file is never +// read past the limit, so memory is bounded regardless of file size. +func SafeReadFileN(root, path string, limit int64) (data []byte, truncated bool, err error) { + if err := checkSafePath(root, path); err != nil { + return nil, false, err + } + f, err := os.Open(path) + if err != nil { + return nil, false, err + } + defer func() { _ = f.Close() }() + // Read one byte past the limit so truncation is detectable without a + // second stat (the file can change between Lstat and Open). + data, err = io.ReadAll(io.LimitReader(f, limit+1)) + if err != nil { + return nil, false, err + } + if int64(len(data)) > limit { + return data[:limit], true, nil + } + return data, false, nil +} + +// checkSafePath applies SafeReadFile's regular-file and containment checks +// without reading anything. +func checkSafePath(root, path string) error { + info, err := os.Lstat(path) + if err != nil { + return err + } if !info.Mode().IsRegular() { - return nil, fmt.Errorf("%w: %s is not a regular file", ErrUnsafeFile, path) + return fmt.Errorf("%w: %s is not a regular file", ErrUnsafeFile, path) } inside, err := ResolvesWithin(root, path) if err != nil { - return nil, err + return err } if !inside { - return nil, fmt.Errorf("%w: %s resolves outside the skill directory", ErrUnsafeFile, path) + return fmt.Errorf("%w: %s resolves outside the skill directory", ErrUnsafeFile, path) } - return os.ReadFile(path) + return nil } // ResolvesWithin reports whether path, after resolving all symlinks in both diff --git a/util/util_test.go b/util/util_test.go index c168e79..840a8b6 100644 --- a/util/util_test.go +++ b/util/util_test.go @@ -197,3 +197,90 @@ func TestResolvesWithin(t *testing.T) { t.Errorf("ResolvesWithin(root, outside) = %v, %v; want false, nil", inside, err) } } + +func TestSafeReadFileN(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "ten.txt") + if err := os.WriteFile(path, []byte("0123456789"), 0o644); err != nil { + t.Fatal(err) + } + + for _, tt := range []struct { + limit int64 + want string + truncated bool + }{ + {4, "0123", true}, + {10, "0123456789", false}, + {20, "0123456789", false}, + } { + got, truncated, err := SafeReadFileN(dir, path, tt.limit) + if err != nil { + t.Fatalf("SafeReadFileN(limit=%d): %v", tt.limit, err) + } + if string(got) != tt.want || truncated != tt.truncated { + t.Errorf("SafeReadFileN(limit=%d) = %q, %v; want %q, %v", tt.limit, got, truncated, tt.want, tt.truncated) + } + } + + if runtime.GOOS != "windows" { + link := filepath.Join(dir, "link.txt") + if err := os.Symlink(path, link); err != nil { + t.Fatal(err) + } + if _, _, err := SafeReadFileN(dir, link, 4); !errors.Is(err, ErrUnsafeFile) { + t.Errorf("SafeReadFileN(symlink) error = %v, want ErrUnsafeFile", err) + } + } +} + +// TestSafeReadFile_TooLarge covers issue #87: a file past MaxSkillFileBytes +// is refused rather than loaded whole. The oversized file is sparse, so the +// test costs no disk. +func TestSafeReadFile_TooLarge(t *testing.T) { + dir := t.TempDir() + sparse := func(name string, size int64) string { + path := filepath.Join(dir, name) + f, err := os.Create(path) + if err != nil { + t.Fatal(err) + } + if err := f.Truncate(size); err != nil { + t.Fatal(err) + } + if err := f.Close(); err != nil { + t.Fatal(err) + } + return path + } + + over := sparse("over.bin", MaxSkillFileBytes+1) + if _, err := SafeReadFile(dir, over); !errors.Is(err, ErrFileTooLarge) { + t.Errorf("SafeReadFile(over limit) error = %v, want ErrFileTooLarge", err) + } + + exact := sparse("exact.bin", MaxSkillFileBytes) + got, err := SafeReadFile(dir, exact) + if err != nil { + t.Fatalf("SafeReadFile(at limit): %v", err) + } + if int64(len(got)) != MaxSkillFileBytes { + t.Errorf("SafeReadFile(at limit) len = %d, want %d", len(got), MaxSkillFileBytes) + } +} + +func TestFormatByteSize(t *testing.T) { + for _, tt := range []struct { + n int64 + want string + }{ + {8 << 20, "8 MiB"}, + {64 << 10, "64 KiB"}, + {100, "100 bytes"}, + {(8 << 20) + 1, "8388609 bytes"}, + } { + if got := FormatByteSize(tt.n); got != tt.want { + t.Errorf("FormatByteSize(%d) = %q, want %q", tt.n, got, tt.want) + } + } +}