fix(validate): count characters, accept every major version, and keep citations inside the workspace

internal/validate/codewiki.go

@@ -106,7 +106,17 @@ func checkCitation(set *finding.Set, root, file string, line int, m []string) {
 		return
 	}
 
-	resolved := filepath.Join(root, filepath.FromSlash(target))
+	// A citation names a path inside the checkout, and nothing else. Without this the
+	// target goes straight into filepath.Join, where a leading ../.. is cleaned into a
+	// real path outside root and then opened — a Markdown file in the workspace would
+	// be choosing what scc reads off the machine. Same rule as workspace.SafeName, at
+	// the one place a path arrives from a document rather than from argv.
+	resolved, ok := resolveInRoot(root, target)
+	if !ok {
+		set.Addf(file, line, "codewiki.citation-invalid",
+			"%s escapes the workspace; a citation names a path inside the checkout", target)
+		return
+	}
 	if !isFile(resolved) {
 		set.Addf(file, line, "codewiki.citation-unresolved", "%s does not exist", target)
 		return
@@ -124,6 +134,55 @@ func checkCitation(set *finding.Set, root, file string, line int, m []string) {
 	}
 }
 
+// resolveInRoot joins target onto root and reports whether the result stayed inside it.
+// An absolute target is rejected outright; a relative one is rejected once Clean has
+// walked it above root.
+//
+// Lexical containment is necessary and not sufficient. Both isFile and os.ReadFile
+// follow symlinks, so a link that sits inside the workspace and points out of it walks
+// past a Clean-based check untouched — the path never spells "..", the kernel does the
+// escaping. The second check therefore compares the paths with every link already
+// resolved.
+func resolveInRoot(root, target string) (string, bool) {
+	t := filepath.FromSlash(target)
+	if filepath.IsAbs(t) || strings.HasPrefix(target, "/") {
+		return "", false
+	}
+	resolved := filepath.Join(root, t)
+	if !within(root, resolved) {
+		return "", false
+	}
+
+	// The root is resolved too, not just the target: a workspace legitimately sits under
+	// a symlink (macOS puts /var behind /private/var), and comparing a fully resolved
+	// target against an unresolved root would call every citation in such a workspace an
+	// escape.
+	realRoot, err := filepath.EvalSymlinks(root)
+	if err != nil {
+		return "", false
+	}
+	realResolved, err := filepath.EvalSymlinks(resolved)
+	if err != nil {
+		// The target does not exist, so nothing was followed and nothing escaped. The
+		// lexical result stands and the caller reports it as unresolved, which is the
+		// more useful finding than calling a typo an escape.
+		return resolved, true
+	}
+	if !within(realRoot, realResolved) {
+		return "", false
+	}
+	return resolved, true
+}
+
+// within reports whether path is root or sits beneath it, comparing lexically.
+func within(root, path string) bool {
+	rel, err := filepath.Rel(root, path)
+	if err != nil {
+		return false
+	}
+	return rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator))
+}
+
 // checkCodewikiHeadings holds slugs to being unique and derived from their headings. A
 // duplicated heading gives two sections one anchor, and a link to it lands on whichever
 // came first.

internal/validate/knowledge_test.go

@@ -384,3 +384,125 @@ func TestEverythingMergesAndCounts(t *testing.T) {
 		}
 	}
 }
+
+// A citation is a path inside the checkout. A traversing target must be refused before
+// it reaches os.ReadFile, or a Markdown page decides what scc opens on the machine.
+func TestCodewikiCitationsCannotEscapeTheWorkspace(t *testing.T) {
+	root := t.TempDir()
+	// A real file one level above the workspace, which the traversal below would reach.
+	write(t, filepath.Join(root, "..", "outside-the-workspace.txt"), "secret\nsecret\n")
+	defer os.Remove(filepath.Join(root, "..", "outside-the-workspace.txt"))
+
+	for _, target := range []string{
+		"../outside-the-workspace.txt",
+		"docs/../../outside-the-workspace.txt",
+	} {
+		t.Run(target, func(t *testing.T) {
+			dir := t.TempDir()
+			write(t, filepath.Join(dir, "..", "outside-the-workspace.txt"), "secret\nsecret\n")
+			defer os.Remove(filepath.Join(dir, "..", "outside-the-workspace.txt"))
+			// Past the end of the two-line fixture, so the out-of-range assertion
+			// below can actually distinguish a blocked citation from a read one.
+			write(t, filepath.Join(paths.Codewiki(dir), "app.md"),
+				"# App\n\n## Section\n\n["+target+":1-3]()\n")
+
+			got := runValidator(t, Codewiki, dir)
+			if !contains(got, "codewiki.citation-invalid") {
+				t.Errorf("rules = %v, want codewiki.citation-invalid for %q", got, target)
+			}
+			// The give-away that the read happened anyway: a resolved citation is
+			// range-checked, so out-of-range means scc opened the outside file.
+			if contains(got, "codewiki.citation-out-of-range") {
+				t.Errorf("rules = %v: %q was read despite escaping the workspace", got, target)
+			}
+		})
+	}
+}
+
+// An absolute citation escapes just as surely, and filepath.Join would silently graft
+// it onto root on Windows while honoring it on Unix.
+func TestCodewikiCitationsCannotBeAbsolute(t *testing.T) {
+	root := t.TempDir()
+	write(t, filepath.Join(paths.Codewiki(root), "app.md"),
+		"# App\n\n## Section\n\n[/etc/passwd:1-2]()\n")
+
+	got := runValidator(t, Codewiki, root)
+	if !contains(got, "codewiki.citation-invalid") {
+		t.Errorf("rules = %v, want codewiki.citation-invalid", got)
+	}
+}
+
+// baseName drops a Go major-version suffix so the last real path segment is what gets
+// compared against stack.md. Every major from 2 up is a valid suffix, and the two-digit
+// ones are the easy range to lose: a leading-digit character class that starts at 2
+// silently rejects v10 through v19.
+func TestBaseNameDropsEveryMajorVersionSuffix(t *testing.T) {
+	for _, tc := range []struct{ module, want string }{
+		{"github.com/go-chi/chi/v5", "chi"},
+		{"github.com/go-chi/chi/v2", "chi"},
+		{"github.com/go-chi/chi/v9", "chi"},
+		{"github.com/go-chi/chi/v10", "chi"},
+		{"github.com/go-chi/chi/v11", "chi"},
+		{"github.com/go-chi/chi/v19", "chi"},
+		{"github.com/go-chi/chi/v20", "chi"},
+		{"github.com/go-chi/chi/v100", "chi"},
+		// v1 is never written as a path suffix, and v0 is not a major version. Neither
+		// is a segment that merely starts with v.
+		{"github.com/go-chi/chi/v1", "v1"},
+		{"github.com/go-chi/chi/v0", "v0"},
+		{"github.com/spf13/viper", "viper"},
+		{"gopkg.in/yaml.v3", "yaml.v3"},
+	} {
+		if got := baseName(tc.module); got != tc.want {
+			t.Errorf("baseName(%q) = %q, want %q", tc.module, got, tc.want)
+		}
+	}
+}
+
+// Lexical containment is not enough. A symlink inside the workspace pointing out of it
+// spells no "..", so a Clean-based check passes it through — and isFile and os.ReadFile
+// both follow it. The escape is silent in exactly the way the traversal case was.
+func TestCodewikiCitationsCannotEscapeThroughASymlink(t *testing.T) {
+	root := t.TempDir()
+	outside := t.TempDir()
+
+	secret := filepath.Join(outside, "secret.txt")
+	write(t, secret, "secret\nsecret\n")
+	link := filepath.Join(root, "link.txt")
+	if err := os.Symlink(secret, link); err != nil {
+		t.Skipf("symlinks unavailable: %v", err) // unprivileged Windows
+	}
+
+	// The cited range runs past the end of the two-line fixture on purpose. Citing 1-2
+	// would sit inside the file, so out-of-range would stay silent whether or not the
+	// read happened and the second assertion below would prove nothing.
+	write(t, filepath.Join(paths.Codewiki(root), "app.md"),
+		"# App\n\n## Section\n\n[link.txt:1-3]()\n")
+
+	got := runValidator(t, Codewiki, root)
+	if !contains(got, "codewiki.citation-invalid") {
+		t.Errorf("rules = %v, want codewiki.citation-invalid: the link left the workspace", got)
+	}
+	if contains(got, "codewiki.citation-out-of-range") {
+		t.Errorf("rules = %v: the outside file was read through the symlink", got)
+	}
+}
+
+// The counterpart, and the reason the root is resolved rather than compared raw: a
+// workspace that itself sits under a symlink still validates its own files. macOS does
+// this to every t.TempDir() by putting /var behind /private/var.
+func TestCodewikiValidatesAWorkspaceBehindASymlink(t *testing.T) {
+	real := t.TempDir()
+	write(t, filepath.Join(real, "main.go"), "one\ntwo\n")
+	write(t, filepath.Join(paths.Codewiki(real), "app.md"),
+		"# App\n\n## Section\n\n[main.go:1-2]()\n")
+
+	link := filepath.Join(t.TempDir(), "workspace")
+	if err := os.Symlink(real, link); err != nil {
+		t.Skipf("symlinks unavailable: %v", err)
+	}
+
+	if got := runValidator(t, Codewiki, link); len(got) != 0 {
+		t.Errorf("rules = %v, want none: a citation inside a symlinked workspace is not an escape", got)
+	}
+}

internal/validate/skill.go

@@ -7,6 +7,7 @@ import (
 	"regexp"
 	"sort"
 	"strings"
+	"unicode/utf8"
 
 	"github.com/protonspy/spec-claude-code/internal/finding"
 	"github.com/protonspy/spec-claude-code/internal/mdscan"
@@ -25,6 +26,12 @@ const (
 	skillBodyMaxLines     = 500  // "Keep your main SKILL.md under 500 lines"
 )
 
+// The spec counts characters, so scc counts runes. len() on a string is bytes, which
+// agrees with the spec only for ASCII and reports a shorter budget than the standard
+// grants for anything else — an accented description would be failed at 1024 bytes
+// while still inside its 1024 characters.
+func charLen(s string) int { return utf8.RuneCountInString(s) }
+
 // skillNameRe is the name charset: lowercase alphanumerics and hyphens, no leading
 // or trailing hyphen, no consecutive hyphens.
 var skillNameRe = regexp.MustCompile(`^[a-z0-9]+(-[a-z0-9]+)*$`)
@@ -95,9 +102,9 @@ func skill(set *finding.Set, root, dir, dirName string) error {
 	checkSkillName(set, file, fm, dirName)
 	checkSkillDescription(set, file, fm)
 
-	if v, ok := fm.Get("compatibility"); ok && len(v) > skillCompatibilityMax {
+	if v, ok := fm.Get("compatibility"); ok && charLen(v) > skillCompatibilityMax {
 		set.Addf(file, 1, "skill.compatibility-too-long",
-			"`compatibility` is %d characters; the spec allows %d", len(v), skillCompatibilityMax)
+			"`compatibility` is %d characters; the spec allows %d", charLen(v), skillCompatibilityMax)
 	}
 
 	// The body budget is a SHOULD in the spec, and it is reported as one: the agent
@@ -119,9 +126,9 @@ func checkSkillName(set *finding.Set, file string, fm mdscan.Frontmatter, dirNam
 		set.Addf(file, 1, "skill.missing-name", "`name` is required and must be 1-%d characters", skillNameMax)
 		return
 	}
-	if len(name) > skillNameMax {
+	if charLen(name) > skillNameMax {
 		set.Addf(file, 1, "skill.name-too-long",
-			"`name` is %d characters; the spec allows 1-%d", len(name), skillNameMax)
+			"`name` is %d characters; the spec allows 1-%d", charLen(name), skillNameMax)
 	}
 	if !skillNameRe.MatchString(name) {
 		set.Addf(file, 1, "skill.name-invalid",
@@ -142,9 +149,9 @@ func checkSkillDescription(set *finding.Set, file string, fm mdscan.Frontmatter)
 			"`description` is required: say what the skill does and when to use it")
 		return
 	}
-	if len(desc) > skillDescriptionMax {
+	if charLen(desc) > skillDescriptionMax {
 		set.Addf(file, 1, "skill.description-too-long",
-			"`description` is %d characters; the spec allows 1-%d", len(desc), skillDescriptionMax)
+			"`description` is %d characters; the spec allows 1-%d", charLen(desc), skillDescriptionMax)
 	}
 }
 

internal/validate/skill_test.go

@@ -294,3 +294,35 @@ func TestFindingsAreOrderedAcrossSkills(t *testing.T) {
 		t.Errorf("first finding is %s, want the alpha skill", got[0].File)
 	}
 }
+
+// The spec's limits are in characters. Counting bytes agrees only for ASCII, and would
+// fail a description that is inside its 1024 characters but past 1024 bytes — the
+// false positive on a valid skill that TestLengthLimitsComeFromTheSpec guards for ASCII.
+func TestLengthLimitsCountCharactersNotBytes(t *testing.T) {
+	// Two bytes per rune, so a description exactly at the limit is twice over in bytes.
+	desc := strings.Repeat("é", skillDescriptionMax)
+	if len(desc) <= skillDescriptionMax {
+		t.Fatalf("test premise broken: %d bytes is not over the %d limit", len(desc), skillDescriptionMax)
+	}
+	root := t.TempDir()
+	writeSkill(t, root, "accented", "---\nname: accented\ndescription: "+desc+"\n---\n\nBody.\n")
+	if hasRule(skills(t, root), "skill.description-too-long") {
+		t.Errorf("a description of exactly %d characters was reported as too long", skillDescriptionMax)
+	}
+
+	// And one character over is still caught, so the fix did not simply disable the check.
+	root = t.TempDir()
+	writeSkill(t, root, "accented", "---\nname: accented\ndescription: "+desc+"é\n---\n\nBody.\n")
+	if !hasRule(skills(t, root), "skill.description-too-long") {
+		t.Error("a description one character over the limit was accepted")
+	}
+}
+
+func TestCompatibilityLimitCountsCharacters(t *testing.T) {
+	root := t.TempDir()
+	writeSkill(t, root, "picky", "---\nname: picky\ndescription: A skill. Use when testing.\ncompatibility: "+
+		strings.Repeat("é", skillCompatibilityMax)+"\n---\n\nBody.\n")
+	if hasRule(skills(t, root), "skill.compatibility-too-long") {
+		t.Errorf("a compatibility field of exactly %d characters was reported as too long", skillCompatibilityMax)
+	}
+}

internal/validate/stack.go

@@ -179,4 +179,8 @@ func baseName(module string) string {
 }
 
 // majorVersionRe matches a Go module's major-version path suffix: v2, v5, v11.
-var majorVersionRe = regexp.MustCompile(`^v[2-9]\d*$`)
+//
+// Every integer from 2 up, which is two alternatives rather than one character class:
+// a bare 2-9, or a multi-digit number whose leading digit may be 1 (v10, v11). A single
+// `v[2-9]\d*` looks equivalent and is not — it rejects the whole v10-v19 range.
+var majorVersionRe = regexp.MustCompile(`^v([2-9]|[1-9]\d+)$`)