From 7ea56055f76844c3af7d1c8b4b29a9f5d9d8160c Mon Sep 17 00:00:00 2001 From: Lucas Machado Date: Fri, 10 Jul 2026 09:55:09 +0200 Subject: [PATCH] refactor: honest single-walk + dedupe presets + version pragma MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up PR that honestly delivers what the earlier roadmap PRs oversold. Everything is behind broadened parity goldens so any drift is a build failure. ## 1. True single-walk rule engine (fixes spec 005 dishonesty) The prior "rule engine" PR (#17) shipped an architectural wrapper but kept the seven per-rule filepath.Walk calls untouched — every rule still walked the tree via the old Validate* method it was wrapping. Net cost: 8 walks per run instead of the previous 7. This commit does the actual conversion: - Each rule iterates ctx.Tree.Entries in walk order. - dirStructureRule tracks a skipTracker to preserve filepath.SkipDir semantics from the legacy walk (disallowed subtree suppresses descendant checks). - requiredPathsRule and requiredGroupsRule keep their os.Stat / glob helpers — the legacy quirk of seeing through ignored directories is a parity requirement. - placementRule and boundariesRule preserve their Successes counter quirks (once per (file, rule) pair even on violations). - The exported Validate* methods stay as thin wrappers over runSingleRule(path, rule) so root validator_test.go and any external callers keep working. Broadened parity fixtures cover boundaries, requiredGroups, and disallowed-subtree SkipDir semantics — three cases the earlier goldens didn't touch. 21 parity subtests, all byte-identical. ## 2. Init templates now live in one place (fixes template dup) Deletes the 220-line projectTemplates map in internal/cli/init.go. `init --type ` now reads the same embedded preset files that `extends: -standard` resolves against, via config.ReadPreset, and prepends a one-line project-typed header comment. Two ways to reach the same rules; one source of truth. - init.go: 322 → 168 lines. - test/init_presets_test.go proves init output body is byte-identical to the corresponding preset AND that init --type go and extends: go-standard produce equivalent validation on the same tree. ## 3. Version pragma actually enforced (fixes docs-only mitigation) The prior mitigation for "old binary + new config with `extends`" was just a suggestion to add `# requires structlint >= vX.Y` in comments. Nothing enforced it — users saw the raw yaml `field not found` error. New internal/config/versioncheck.go parses the pragma from the raw bytes BEFORE strict parsing and prints an actionable message when the running binary is older than required. Dev builds (Version=="dev") and malformed pragmas skip the check. Tests: 10 in-package unit tests over parse/compare edge cases, plus 2 binary-based end-to-end tests that build pinned binaries with ldflags-injected versions and prove the error surfaces cleanly. ## 4. Deeper SKILL.md validation skill_contract_test.go had shallow checks (file exists, frontmatter delimiter present). skill_deep_test.go adds: - Frontmatter parses as real YAML with the expected shape. - Trigger phrases from the description survive block-scalar folding (parses YAML first, then normalizes whitespace before searching). - Every subcommand referenced in setup recipes actually boots via app.Run(--help) — catches renames. - JSON v1 contract mentioned + all four exit codes documented. ## Numbers 167 tests before this PR, 191 tests after. Every existing parity golden byte-identical. Self-validation still passes. --- docs/user/configuration.md | 18 +- internal/cli/init.go | 290 ++++-------------- internal/config/config.go | 6 + internal/config/merge.go | 23 ++ internal/config/versioncheck.go | 111 +++++++ internal/config/versioncheck_test.go | 118 +++++++ internal/validator/engine.go | 250 +++++++++++++-- internal/validator/validator.go | 275 +++-------------- test/init_presets_test.go | 91 ++++++ test/skill_deep_test.go | 194 ++++++++++++ .../parity/boundaries/.structlint.yaml | 17 + test/testdata/parity/boundaries/README.md | 1 + test/testdata/parity/boundaries/go.mod | 3 + .../parity/boundaries/internal/db/db.go | 3 + .../boundaries/internal/domain/clean.go | 3 + .../boundaries/internal/domain/model.go | 6 + .../disallowed-subtree/.structlint.yaml | 13 + .../parity/disallowed-subtree/README.md | 1 + .../parity/disallowed-subtree/src/main.go | 1 + .../parity/disallowed-subtree/tmp/leftover.go | 1 + .../disallowed-subtree/tmp/nested/deeper.go | 1 + test/testdata/parity/goldens/boundaries.exit | 1 + test/testdata/parity/goldens/boundaries.json | 33 ++ test/testdata/parity/goldens/boundaries.text | 15 + .../parity/goldens/disallowed-subtree.exit | 1 + .../parity/goldens/disallowed-subtree.json | 33 ++ .../parity/goldens/disallowed-subtree.text | 15 + .../parity/goldens/required-groups.exit | 1 + .../parity/goldens/required-groups.json | 50 +++ .../parity/goldens/required-groups.text | 19 ++ .../parity/required-groups/.structlint.yaml | 20 ++ .../testdata/parity/required-groups/README.md | 1 + .../required-groups/cmd/broken/helper.go | 3 + .../required-groups/cmd/complete/main.go | 3 + test/version_pragma_test.go | 69 +++++ 35 files changed, 1212 insertions(+), 478 deletions(-) create mode 100644 internal/config/versioncheck.go create mode 100644 internal/config/versioncheck_test.go create mode 100644 test/init_presets_test.go create mode 100644 test/skill_deep_test.go create mode 100644 test/testdata/parity/boundaries/.structlint.yaml create mode 100644 test/testdata/parity/boundaries/README.md create mode 100644 test/testdata/parity/boundaries/go.mod create mode 100644 test/testdata/parity/boundaries/internal/db/db.go create mode 100644 test/testdata/parity/boundaries/internal/domain/clean.go create mode 100644 test/testdata/parity/boundaries/internal/domain/model.go create mode 100644 test/testdata/parity/disallowed-subtree/.structlint.yaml create mode 100644 test/testdata/parity/disallowed-subtree/README.md create mode 100644 test/testdata/parity/disallowed-subtree/src/main.go create mode 100644 test/testdata/parity/disallowed-subtree/tmp/leftover.go create mode 100644 test/testdata/parity/disallowed-subtree/tmp/nested/deeper.go create mode 100644 test/testdata/parity/goldens/boundaries.exit create mode 100644 test/testdata/parity/goldens/boundaries.json create mode 100644 test/testdata/parity/goldens/boundaries.text create mode 100644 test/testdata/parity/goldens/disallowed-subtree.exit create mode 100644 test/testdata/parity/goldens/disallowed-subtree.json create mode 100644 test/testdata/parity/goldens/disallowed-subtree.text create mode 100644 test/testdata/parity/goldens/required-groups.exit create mode 100644 test/testdata/parity/goldens/required-groups.json create mode 100644 test/testdata/parity/goldens/required-groups.text create mode 100644 test/testdata/parity/required-groups/.structlint.yaml create mode 100644 test/testdata/parity/required-groups/README.md create mode 100644 test/testdata/parity/required-groups/cmd/broken/helper.go create mode 100644 test/testdata/parity/required-groups/cmd/complete/main.go create mode 100644 test/version_pragma_test.go diff --git a/docs/user/configuration.md b/docs/user/configuration.md index 73fff35..756168a 100644 --- a/docs/user/configuration.md +++ b/docs/user/configuration.md @@ -68,7 +68,23 @@ dir_structure: - `placement`, `requiredGroups`, `boundaries` — keyed by `id`. Same ID → child rule replaces the parent's wholesale. New IDs append. - Parents resolve depth-first. Chains are cycle-checked and capped at depth 10. -**Compatibility warning:** the `extends` key requires structlint **v0.6.0 or newer**. Older binaries strict-parse the file and reject it with `field extends not found in type config.Config` — pin your CI action and pre-commit rev to a version that supports it, and add a `# requires structlint >= vX.Y` comment at the top of configs that use `extends`. +**Compatibility warning:** the `extends` key requires structlint **v0.6.0 or newer**. Older binaries would strict-parse the file and reject it with `field extends not found in type config.Config` — a confusing error for the actual problem. + +To make old-binary failures actionable, add a `# requires structlint >= vX.Y` comment at the top of any config that uses `extends`: + +```yaml +# requires structlint >= v0.6.0 +extends: go-standard +``` + +When a binary older than the pragma sees this comment, it stops **before** strict parsing and prints: + +``` +.structlint.yaml requires structlint >= v0.6.0, but running version is v0.5.0. +Upgrade the binary (go install github.com/AxeForging/structlint/cmd/structlint@latest) … +``` + +The pragma is checked before strict parsing, so it works even on binaries too old to know about `extends`. Dev builds (unstamped `go run`) skip the check. ### Editor autocomplete via JSON Schema diff --git a/internal/cli/init.go b/internal/cli/init.go index 7721888..0b3fe04 100644 --- a/internal/cli/init.go +++ b/internal/cli/init.go @@ -6,11 +6,34 @@ import ( "fmt" "os" "path/filepath" + "strings" + "github.com/AxeForging/structlint/internal/config" "github.com/AxeForging/structlint/internal/infer" "github.com/urfave/cli/v3" ) +// projectTypeToPreset maps `init --type` values to preset names in +// internal/config/presets/. Kept as a distinct map (instead of using the +// preset names directly as --type values) so the shorter --type flag is +// still ergonomic and the preset names remain the source of truth. +var projectTypeToPreset = map[string]string{ + "go": "go-standard", + "node": "node-standard", + "python": "python-standard", + "generic": "generic", +} + +// projectTypeHeaders is the single-line header prepended to the preset +// content so the generated config file reads like a starter template +// instead of a bare preset dump. +var projectTypeHeaders = map[string]string{ + "go": "# structlint configuration for Go projects\n", + "node": "# structlint configuration for Node.js projects\n", + "python": "# structlint configuration for Python projects\n", + "generic": "# structlint configuration\n", +} + // NewInitCmd creates the init command for generating starter configs. func NewInitCmd() *cli.Command { return &cli.Command{ @@ -62,12 +85,11 @@ func NewInitCmd() *cli.Command { projectType = detectProjectType(".") } - template, ok := projectTemplates[projectType] - if !ok { - return fmt.Errorf("unknown project type: %s (available: go, node, python, generic)", projectType) + data, err := renderProjectTemplate(projectType) + if err != nil { + return err } - - if err := os.WriteFile(configPath, []byte(template), 0o644); err != nil { + if err := os.WriteFile(configPath, data, 0o644); err != nil { return fmt.Errorf("failed to write configuration: %w", err) } @@ -78,6 +100,43 @@ func NewInitCmd() *cli.Command { } } +// renderProjectTemplate builds a starter config for the requested project +// type by reading the corresponding preset (source of truth for the +// baseline rules) and prepending a project-typed comment header. This +// keeps init's output and `extends:` presets from drifting. +func renderProjectTemplate(projectType string) ([]byte, error) { + presetName, ok := projectTypeToPreset[projectType] + if !ok { + return nil, fmt.Errorf("unknown project type: %s (available: %s)", + projectType, strings.Join(projectTypeList(), ", ")) + } + body, err := config.ReadPreset(presetName) + if err != nil { + return nil, fmt.Errorf("read preset for %s: %w", projectType, err) + } + header := projectTypeHeaders[projectType] + buf := make([]byte, 0, len(header)+len(body)) + buf = append(buf, header...) + buf = append(buf, body...) + return buf, nil +} + +func projectTypeList() []string { + out := make([]string, 0, len(projectTypeToPreset)) + for k := range projectTypeToPreset { + out = append(out, k) + } + // Alphabetical for stable error messages. + for i := 1; i < len(out); i++ { + j := i + for j > 0 && out[j-1] > out[j] { + out[j-1], out[j] = out[j], out[j-1] + j-- + } + } + return out +} + // detectProjectType guesses the project type from files in the directory. func detectProjectType(dir string) string { checks := []struct { @@ -99,224 +158,3 @@ func detectProjectType(dir string) string { return "generic" } - -var projectTemplates = map[string]string{ - "go": `# structlint configuration for Go projects -dir_structure: - allowedPaths: - - "." - - "cmd/**" - - "internal/**" - - "pkg/**" - - "api/**" - - "test/**" - - "docs/**" - - "scripts/**" - - ".github/**" - disallowedPaths: - - "vendor/**" - - "node_modules/**" - - "tmp/**" - requiredPaths: - - "cmd" - -file_naming_pattern: - allowed: - - "*.go" - - "*.mod" - - "*.sum" - - "*.yaml" - - "*.yml" - - "*.json" - - "*.md" - - "*.txt" - - "Makefile" - - "Dockerfile*" - - "*.sh" - - ".gitignore" - - ".goreleaser.yaml" - - "LICENSE*" - disallowed: - - "*.env*" - - "*.key" - - "*.pem" - - ".DS_Store" - - "*.log" - - "*.tmp" - - "*~" - - "*.swp" - required: - - "go.mod" - - "README.md" - - ".gitignore" - -ignore: - - ".git" - - "vendor" - - "bin" - - "dist" -`, - - "node": `# structlint configuration for Node.js projects -dir_structure: - allowedPaths: - - "." - - "src/**" - - "lib/**" - - "test/**" - - "tests/**" - - "__tests__/**" - - "docs/**" - - "scripts/**" - - "public/**" - - "config/**" - - ".github/**" - disallowedPaths: - - "node_modules/**" - - "tmp/**" - - "temp/**" - requiredPaths: - - "src" - -file_naming_pattern: - allowed: - - "*.js" - - "*.ts" - - "*.jsx" - - "*.tsx" - - "*.json" - - "*.yaml" - - "*.yml" - - "*.md" - - "*.css" - - "*.scss" - - "*.html" - - "*.svg" - - "*.png" - - "*.jpg" - - ".gitignore" - - ".eslintrc*" - - ".prettierrc*" - - "*.config.*" - - "Dockerfile*" - - "LICENSE*" - disallowed: - - "*.env*" - - "*.key" - - "*.pem" - - ".DS_Store" - - "*.log" - - "*~" - - "*.swp" - required: - - "package.json" - - "README.md" - -ignore: - - ".git" - - "node_modules" - - "dist" - - "build" - - "coverage" -`, - - "python": `# structlint configuration for Python projects -dir_structure: - allowedPaths: - - "." - - "src/**" - - "tests/**" - - "test/**" - - "docs/**" - - "scripts/**" - - ".github/**" - disallowedPaths: - - "__pycache__/**" - - ".tox/**" - - "*.egg-info/**" - - "node_modules/**" - requiredPaths: [] - -file_naming_pattern: - allowed: - - "*.py" - - "*.pyi" - - "*.toml" - - "*.cfg" - - "*.ini" - - "*.txt" - - "*.yaml" - - "*.yml" - - "*.json" - - "*.md" - - "*.rst" - - "Makefile" - - "Dockerfile*" - - "*.sh" - - ".gitignore" - - ".flake8" - - "LICENSE*" - - "MANIFEST.in" - disallowed: - - "*.env*" - - "*.key" - - "*.pem" - - ".DS_Store" - - "*.log" - - "*~" - - "*.swp" - - "*.pyc" - required: - - "README.md" - -ignore: - - ".git" - - "__pycache__" - - ".tox" - - ".venv" - - "venv" - - "dist" - - "build" - - "*.egg-info" -`, - - "generic": `# structlint configuration -dir_structure: - allowedPaths: - - "." - - "src/**" - - "lib/**" - - "test/**" - - "tests/**" - - "docs/**" - - "scripts/**" - - ".github/**" - disallowedPaths: - - "tmp/**" - - "temp/**" - - "node_modules/**" - requiredPaths: [] - -file_naming_pattern: - allowed: - - "*.*" - disallowed: - - "*.env*" - - "*.key" - - "*.pem" - - ".DS_Store" - - "*.log" - - "*.tmp" - - "*~" - - "*.swp" - required: - - "README.md" - -ignore: - - ".git" - - "node_modules" - - "vendor" - - "dist" - - "build" -`, -} diff --git a/internal/config/config.go b/internal/config/config.go index f8cffc3..456502a 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -122,6 +122,12 @@ func parseConfigFile(path string) (*Config, error) { if err != nil { return nil, err } + // Version pragma is checked BEFORE strict parsing so users of newer + // features (e.g. `extends`) get an actionable "upgrade required" + // error instead of a raw yaml `field ... not found` message. + if err := enforceRequiresComment(path, data); err != nil { + return nil, err + } return parseConfigBytes(data, filepath.Ext(path)) } diff --git a/internal/config/merge.go b/internal/config/merge.go index 52553bf..10dfbc7 100644 --- a/internal/config/merge.go +++ b/internal/config/merge.go @@ -21,6 +21,29 @@ var presetNames = map[string]string{ "generic": "presets/generic.yaml", } +// ReadPreset returns the raw YAML bytes of a preset by name (as it would +// appear in `extends:`). Exposed for `structlint init` so the four +// project templates are stored in one place instead of duplicated as +// Go string literals in internal/cli/init.go. +func ReadPreset(name string) ([]byte, error) { + path, ok := presetNames[name] + if !ok { + return nil, fmt.Errorf("unknown preset %q (valid: %s)", name, presetsList()) + } + return presetFS.ReadFile(path) +} + +// PresetNames returns the sorted list of built-in preset names for +// error messages and completion. +func PresetNames() []string { + out := make([]string, 0, len(presetNames)) + for name := range presetNames { + out = append(out, name) + } + sort.Strings(out) + return out +} + const maxExtendsDepth = 10 // loadResolved parses path, resolves its extends chain depth-first with diff --git a/internal/config/versioncheck.go b/internal/config/versioncheck.go new file mode 100644 index 0000000..3a02d03 --- /dev/null +++ b/internal/config/versioncheck.go @@ -0,0 +1,111 @@ +package config + +import ( + "fmt" + "regexp" + "strconv" + "strings" + + "github.com/AxeForging/structlint/internal/build" +) + +// requiresRE matches a `# requires structlint >= vX.Y[.Z]` comment +// anywhere in the config file. Whitespace around tokens is tolerated; +// the version literal is left unquoted, in vX.Y or vX.Y.Z form. +var requiresRE = regexp.MustCompile(`(?m)^\s*#\s*requires\s+structlint\s*>=\s*v?(\d+)\.(\d+)(?:\.(\d+))?`) + +// enforceRequiresComment inspects raw config bytes for a version pragma +// and fails with a helpful message when the running binary is older than +// the required version. Skips when the running binary is a dev build — +// unstamped local builds can't be meaningfully compared to a tag. +func enforceRequiresComment(configPath string, data []byte) error { + match := requiresRE.FindSubmatch(data) + if match == nil { + return nil + } + need, err := parseSemver(string(match[1]), string(match[2]), string(match[3])) + if err != nil { + return nil // malformed comment; ignore + } + current, ok := parseBinaryVersion(build.Version) + if !ok { + // dev build or otherwise unparseable; don't second-guess it. + return nil + } + if semverLess(current, need) { + return fmt.Errorf( + "%s requires structlint >= v%s, but running version is v%s.\n"+ + "Upgrade the binary (go install github.com/AxeForging/structlint/cmd/structlint@latest) "+ + "or pin the older feature set by removing the pragma.", + configPath, semverString(need), semverString(current), + ) + } + return nil +} + +type semver struct{ Major, Minor, Patch int } + +func parseSemver(major, minor, patch string) (semver, error) { + M, err := strconv.Atoi(major) + if err != nil { + return semver{}, err + } + m, err := strconv.Atoi(minor) + if err != nil { + return semver{}, err + } + p := 0 + if patch != "" { + p, err = strconv.Atoi(patch) + if err != nil { + return semver{}, err + } + } + return semver{Major: M, Minor: m, Patch: p}, nil +} + +// parseBinaryVersion extracts a semver from build.Version. Accepts the +// leading `v`, tolerates trailing metadata (e.g. `v0.6.0-3-gabcdef-dirty` +// or `v0.6.0+meta`). Returns ok=false for `dev` or unparseable inputs so +// callers can skip the check. +func parseBinaryVersion(v string) (semver, bool) { + v = strings.TrimSpace(v) + if v == "" || v == "dev" || v == "unknown" { + return semver{}, false + } + v = strings.TrimPrefix(v, "v") + // Split on the first hyphen or plus so metadata like `-3-gabc-dirty` or + // `+meta` doesn't confuse the parse. + for _, sep := range []string{"-", "+"} { + if idx := strings.Index(v, sep); idx > 0 { + v = v[:idx] + } + } + parts := strings.Split(v, ".") + if len(parts) < 2 { + return semver{}, false + } + patch := "0" + if len(parts) >= 3 { + patch = parts[2] + } + sv, err := parseSemver(parts[0], parts[1], patch) + if err != nil { + return semver{}, false + } + return sv, true +} + +func semverLess(a, b semver) bool { + if a.Major != b.Major { + return a.Major < b.Major + } + if a.Minor != b.Minor { + return a.Minor < b.Minor + } + return a.Patch < b.Patch +} + +func semverString(s semver) string { + return fmt.Sprintf("%d.%d.%d", s.Major, s.Minor, s.Patch) +} diff --git a/internal/config/versioncheck_test.go b/internal/config/versioncheck_test.go new file mode 100644 index 0000000..ac22dee --- /dev/null +++ b/internal/config/versioncheck_test.go @@ -0,0 +1,118 @@ +package config + +import ( + "strings" + "testing" + + "github.com/AxeForging/structlint/internal/build" +) + +// withBuildVersion temporarily overrides build.Version for the duration +// of fn, restoring it via t.Cleanup so parallel tests don't leak state. +func withBuildVersion(t *testing.T, v string) { + t.Helper() + prev := build.Version + build.Version = v + t.Cleanup(func() { build.Version = prev }) +} + +func TestEnforceRequiresComment_NoPragmaIsFine(t *testing.T) { + withBuildVersion(t, "v0.5.0") + err := enforceRequiresComment("test.yaml", []byte("dir_structure:\n allowedPaths: [\".\"]\n")) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } +} + +func TestEnforceRequiresComment_SatisfiedVersion(t *testing.T) { + withBuildVersion(t, "v0.6.1") + err := enforceRequiresComment("test.yaml", []byte(`# requires structlint >= v0.6.0 +extends: go-standard +`)) + if err != nil { + t.Fatalf("v0.6.1 should satisfy >= v0.6.0, got: %v", err) + } +} + +func TestEnforceRequiresComment_ExactVersion(t *testing.T) { + withBuildVersion(t, "v0.6.0") + err := enforceRequiresComment("test.yaml", []byte("# requires structlint >= v0.6.0\n")) + if err != nil { + t.Fatalf("v0.6.0 should satisfy >= v0.6.0, got: %v", err) + } +} + +func TestEnforceRequiresComment_TooOld(t *testing.T) { + withBuildVersion(t, "v0.5.0") + err := enforceRequiresComment("path.yaml", []byte("# requires structlint >= v0.6.0\n")) + if err == nil { + t.Fatal("expected error when running version is older than required") + } + msg := err.Error() + if !strings.Contains(msg, "requires structlint >= v0.6.0") { + t.Errorf("error should quote the required version, got: %s", msg) + } + if !strings.Contains(msg, "running version is v0.5.0") { + t.Errorf("error should quote the running version, got: %s", msg) + } + if !strings.Contains(msg, "path.yaml") { + t.Errorf("error should reference the config path, got: %s", msg) + } +} + +func TestEnforceRequiresComment_DevVersionSkipsCheck(t *testing.T) { + withBuildVersion(t, "dev") + err := enforceRequiresComment("test.yaml", []byte("# requires structlint >= v99.0.0\n")) + if err != nil { + t.Fatalf("dev binary should never fail the pragma check, got: %v", err) + } +} + +func TestEnforceRequiresComment_UnstampedDirtyVersion(t *testing.T) { + // Build metadata (e.g. `-3-gabcdef-dirty`) should not confuse the check. + withBuildVersion(t, "v0.6.0-3-gabcdef-dirty") + err := enforceRequiresComment("test.yaml", []byte("# requires structlint >= v0.6.0\n")) + if err != nil { + t.Fatalf("v0.6.0-with-metadata should satisfy >= v0.6.0, got: %v", err) + } +} + +func TestEnforceRequiresComment_MinorVersionOnly(t *testing.T) { + // Pragma without patch should still work. + withBuildVersion(t, "v0.5.9") + err := enforceRequiresComment("test.yaml", []byte("# requires structlint >= v0.6\n")) + if err == nil { + t.Fatal("expected error: v0.5.9 < v0.6.0") + } +} + +func TestEnforceRequiresComment_MalformedPragmaIgnored(t *testing.T) { + withBuildVersion(t, "v0.5.0") + // A comment mentioning the phrase but with garbage version → tolerate. + err := enforceRequiresComment("test.yaml", []byte("# structlint is a linter\n")) + if err != nil { + t.Fatalf("comment without a real pragma should not error: %v", err) + } +} + +func TestParseBinaryVersion_DevBuild(t *testing.T) { + if _, ok := parseBinaryVersion("dev"); ok { + t.Error("dev version must return ok=false") + } + if _, ok := parseBinaryVersion(""); ok { + t.Error("empty version must return ok=false") + } + if _, ok := parseBinaryVersion("unknown"); ok { + t.Error("unknown version must return ok=false") + } +} + +func TestParseBinaryVersion_WithMetadata(t *testing.T) { + sv, ok := parseBinaryVersion("v1.2.3-rc.1") + if !ok { + t.Fatal("expected parse to succeed") + } + if sv.Major != 1 || sv.Minor != 2 || sv.Patch != 3 { + t.Errorf("got %+v, want 1.2.3", sv) + } +} diff --git a/internal/validator/engine.go b/internal/validator/engine.go index 7566847..8bbaa9d 100644 --- a/internal/validator/engine.go +++ b/internal/validator/engine.go @@ -1,11 +1,17 @@ package validator -import "github.com/AxeForging/structlint/internal/config" +import ( + "fmt" + "path/filepath" + "strings" + + "github.com/AxeForging/structlint/internal/config" +) // Rule is the pluggable unit of validation. Each family (dir structure, -// file naming, placement, ...) is a Rule. Rules run in the order Registry -// returns them; that order matches the legacy Validate* invocation -// sequence and is part of the observable contract (spec 005). +// file naming, placement, ...) is a Rule. Rules run in Registry order — +// that order matches the legacy Validate* invocation sequence and is +// part of the observable contract enforced by the parity goldens. type Rule interface { Name() string Run(ctx *RunContext) @@ -18,14 +24,13 @@ type RunContext struct { V *Validator // Skip returns true when the given relative path (dir or file) is out // of scope for --changed-only / --staged. Rules that are meant to be - // global (required-* families) do not consult Skip. + // global (required-* families) never call Skip. Skip func(relPath string, isDir bool) bool } // Registry returns the seven rules in legacy execution order: // dir_structure, file_naming, required_paths, required_files, -// placement, required_groups, boundaries. Spec 010's suggest command -// enumerates rules through this registry. +// placement, required_groups, boundaries. func Registry(cfg *config.Config) []Rule { return []Rule{ dirStructureRule{}, @@ -39,8 +44,8 @@ func Registry(cfg *config.Config) []Rule { } // Run snapshots the tree once, then executes each rule in Registry order -// against the same snapshot. Wire from validate.go instead of calling the -// seven Validate* methods individually. +// against that single snapshot. This is the single-walk architecture: +// rules do not perform their own filepath.Walk — they iterate ctx.Tree. func (v *Validator) Run(path string) { tree := Snapshot(path, v.Config.Ignore) ctx := &RunContext{ @@ -59,57 +64,260 @@ func (v *Validator) Run(path string) { } } -// The seven rule types below are thin wrappers around the existing -// Validate* method bodies. They call the legacy method, which walks the -// tree itself. Consolidating the walks is a follow-up refactor (see spec -// 005's non-goals); this iteration lands the enumerable Registry / Tree -// surface that specs 009 and 010 depend on without touching output. +// skipTracker tracks path prefixes whose subtree should be skipped from +// iteration. Used to preserve filepath.SkipDir semantics when we hit a +// disallowed directory in the middle of a single walk. +type skipTracker struct { + prefixes []string +} + +// mark adds relPath as the root of a skipped subtree. +func (s *skipTracker) mark(relPath string) { + if relPath == "" || relPath == "." { + return + } + s.prefixes = append(s.prefixes, relPath) +} +// under reports whether relPath is inside any skipped subtree. +func (s *skipTracker) under(relPath string) bool { + for _, p := range s.prefixes { + if relPath == p || strings.HasPrefix(relPath, p+"/") { + return true + } + } + return false +} + +// dirStructureRule validates directory structure by iterating Tree.Entries. +// Preserves filepath.SkipDir semantics: hitting a disallowed directory +// suppresses violations for its entire subtree (matching the legacy walk). type dirStructureRule struct{} func (dirStructureRule) Name() string { return "dir_structure" } + func (dirStructureRule) Run(ctx *RunContext) { - ctx.V.ValidateDirStructure(ctx.Tree.Root) + if ctx.Tree.WalkErr != nil { + ctx.V.addViolation("walk_error", "error", ctx.Tree.Root, "filesystem", + fmt.Sprintf("Error walking directory: %s", ctx.Tree.WalkErr)) + } + skipped := skipTracker{} + for _, e := range ctx.Tree.Entries { + if !e.IsDir { + continue + } + if skipped.under(e.RelPath) { + continue + } + if ctx.Skip(e.RelPath, true) { + // Not part of the changed-set; skip its subtree too, matching + // the legacy behavior where the walker returned SkipDir. + skipped.mark(e.RelPath) + continue + } + // Check against disallowed paths. + matched := false + for _, disallowed := range ctx.Cfg.DirStructure.DisallowedPaths { + if pathMatches(e.RelPath, disallowed) { + msg := fmt.Sprintf("Disallowed directory found: %s", e.RelPath) + ctx.V.addViolation("disallowed_directory", "error", e.RelPath, disallowed, msg) + skipped.mark(e.RelPath) + matched = true + break + } + } + if matched { + continue + } + // Check against allowed paths. + isAllowed := false + for _, allowed := range ctx.Cfg.DirStructure.AllowedPaths { + if pathMatches(e.RelPath, allowed) || isParentOfPattern(e.RelPath, allowed) { + isAllowed = true + break + } + } + if isAllowed { + ctx.V.printSuccess(fmt.Sprintf("Allowed directory found: %s", e.RelPath)) + ctx.V.Successes++ + } else { + msg := fmt.Sprintf("Directory not in allowed list: %s", e.RelPath) + ctx.V.addViolation("unallowed_directory", "error", e.RelPath, "dir_structure.allowedPaths", msg) + } + } } +// fileNamingRule validates file naming patterns via Tree iteration. type fileNamingRule struct{} func (fileNamingRule) Name() string { return "file_naming" } + func (fileNamingRule) Run(ctx *RunContext) { - ctx.V.ValidateFileNaming(ctx.Tree.Root) + if ctx.Tree.WalkErr != nil { + ctx.V.addViolation("walk_error", "error", ctx.Tree.Root, "filesystem", + fmt.Sprintf("Error walking directory: %s", ctx.Tree.WalkErr)) + } + for _, e := range ctx.Tree.Entries { + if e.IsDir { + continue + } + if ctx.Skip(e.RelPath, false) { + continue + } + // Disallowed patterns first — matching a disallowed pattern short- + // circuits, matching legacy behavior of `return nil`. + matched := false + for _, disallowed := range ctx.Cfg.FileNamingPattern.Disallowed { + if pathMatches(e.Name, disallowed) || pathMatches(e.RelPath, disallowed) { + msg := fmt.Sprintf("Disallowed file naming pattern found: %s", e.RelPath) + ctx.V.addViolation("disallowed_file_pattern", "error", e.RelPath, disallowed, msg) + matched = true + break + } + } + if matched { + continue + } + isAllowed := false + for _, allowed := range ctx.Cfg.FileNamingPattern.Allowed { + if pathMatches(e.Name, allowed) || pathMatches(e.RelPath, allowed) { + isAllowed = true + break + } + } + if isAllowed { + ctx.V.printSuccess(fmt.Sprintf("Allowed file naming pattern found: %s", e.RelPath)) + ctx.V.Successes++ + } else { + msg := fmt.Sprintf("File not in allowed naming pattern: %s", e.RelPath) + ctx.V.addViolation("unallowed_file_pattern", "error", e.RelPath, "file_naming_pattern.allowed", msg) + } + } } +// requiredPathsRule intentionally does NOT use the Tree — the legacy code +// stats the joined path directly, so a required path inside an ignored +// directory still counts as present. Preserving that quirk is spec 005's +// parity requirement. type requiredPathsRule struct{} func (requiredPathsRule) Name() string { return "required_paths" } + func (requiredPathsRule) Run(ctx *RunContext) { - ctx.V.ValidateRequiredPaths(ctx.Tree.Root) + ctx.V.validateRequiredPathsDirect(ctx.Tree.Root) } +// requiredFilesRule finds required patterns via Tree lookup. Unlike required +// paths, the legacy code walked with ignore filtering, so using the Tree +// matches behavior. type requiredFilesRule struct{} func (requiredFilesRule) Name() string { return "required_files" } + func (requiredFilesRule) Run(ctx *RunContext) { - ctx.V.ValidateRequiredFiles(ctx.Tree.Root) + for _, requiredFile := range ctx.Cfg.FileNamingPattern.Required { + found := false + for _, e := range ctx.Tree.Entries { + if e.IsDir { + continue + } + if pathMatches(e.Name, requiredFile) || pathMatches(e.RelPath, requiredFile) { + found = true + break + } + } + if found { + ctx.V.printSuccess(fmt.Sprintf("Required file pattern found: %s", requiredFile)) + ctx.V.Successes++ + } else { + msg := fmt.Sprintf("Required file pattern missing: %s", requiredFile) + ctx.V.addViolation("missing_required_file", "error", requiredFile, requiredFile, msg) + } + } } +// placementRule iterates Tree files and applies placement rules. Preserves +// the counter quirk from the legacy walk: v.Successes++ is called for +// every (file, rule) pair that passes — a file matching N placement rules +// contributes N successes. type placementRule struct{} func (placementRule) Name() string { return "placement" } + func (placementRule) Run(ctx *RunContext) { - ctx.V.ValidatePlacement(ctx.Tree.Root) + for _, e := range ctx.Tree.Entries { + if e.IsDir { + continue + } + if ctx.Skip(e.RelPath, false) { + continue + } + for _, rule := range ctx.Cfg.Placement { + if !matchesAnyFile(e.RelPath, e.Name, rule.Files) { + continue + } + if underAny(e.RelPath, rule.MustBeUnder) { + ctx.V.Successes++ + continue + } + msg := fmt.Sprintf("File placement violation: %s must be under %s", + e.RelPath, strings.Join(rule.MustBeUnder, ", ")) + ctx.V.addViolation("placement_violation", severity(rule.Severity), e.RelPath, rule.ID, msg) + } + } } +// requiredGroupsRule uses stat-based helpers (existsAt, existsAny, +// matchingDirs) which look through ignored directories on purpose — same +// quirk as requiredPathsRule. type requiredGroupsRule struct{} func (requiredGroupsRule) Name() string { return "required_groups" } + func (requiredGroupsRule) Run(ctx *RunContext) { - ctx.V.ValidateRequiredGroups(ctx.Tree.Root) + ctx.V.validateRequiredGroupsDirect(ctx.Tree.Root) } +// boundariesRule iterates supported source files in the Tree, parses +// imports, and reports violations. Preserves the counter quirk: Successes++ +// is incremented once per (file, matching rule) pair even when that pair +// produced boundary violations, matching legacy behavior. type boundariesRule struct{} func (boundariesRule) Name() string { return "boundaries" } + func (boundariesRule) Run(ctx *RunContext) { - ctx.V.ValidateBoundaries(ctx.Tree.Root) + modulePath := readGoModule(ctx.Tree.Root) + for _, e := range ctx.Tree.Entries { + if e.IsDir { + continue + } + if !isSupportedBoundaryFile(e.RelPath) { + continue + } + if ctx.Skip(e.RelPath, false) { + continue + } + for _, rule := range ctx.Cfg.Boundaries { + if !pathMatches(e.RelPath, rule.From) { + continue + } + imports, err := sourceImports(filepath.Join(ctx.Tree.Root, e.RelPath), e.RelPath) + if err != nil { + ctx.V.addViolation("parse_error", "error", e.RelPath, rule.ID, + fmt.Sprintf("Failed to parse imports: %s", err)) + continue + } + for _, imp := range imports { + localImport := importToLocalPath(modulePath, imp, e.RelPath) + for _, forbidden := range rule.CannotImport { + if pathMatches(imp, forbidden) || pathMatches(localImport, forbidden) { + msg := fmt.Sprintf("Boundary violation: %s imports %s", e.RelPath, imp) + ctx.V.addViolation("boundary_violation", severity(rule.Severity), e.RelPath, rule.ID, msg) + } + } + } + ctx.V.Successes++ + } + } } diff --git a/internal/validator/validator.go b/internal/validator/validator.go index 049fbad..1386ddd 100644 --- a/internal/validator/validator.go +++ b/internal/validator/validator.go @@ -43,62 +43,30 @@ func New(cfg *config.Config, logger *slog.Logger) *Validator { } } -// ValidateDirStructure validates the directory structure. +// ValidateDirStructure is a thin wrapper kept for external callers (e.g. the +// root validator_test.go). It snapshots the tree and runs the corresponding +// rule against it — same output as v.Run() would produce. func (v *Validator) ValidateDirStructure(path string) { - root := cleanRoot(path) - err := filepath.Walk(path, func(currentPath string, info os.FileInfo, err error) error { - if err != nil { - return err - } - relPath := relativePath(root, currentPath) - - // Check if the path should be ignored - for _, ignored := range v.Config.Ignore { - if pathMatches(relPath, ignored) { - if info.IsDir() { - return filepath.SkipDir - } - return nil - } - } - - if info.IsDir() { - if v.shouldSkipChangedDir(relPath) { - return filepath.SkipDir - } - - // Check against disallowed paths - for _, disallowed := range v.Config.DirStructure.DisallowedPaths { - if pathMatches(relPath, disallowed) { - msg := fmt.Sprintf("Disallowed directory found: %s", relPath) - v.addViolation("disallowed_directory", "error", relPath, disallowed, msg) - return filepath.SkipDir // Skip validating contents of disallowed directories - } - } - - // Check against allowed paths - isAllowed := false - for _, allowed := range v.Config.DirStructure.AllowedPaths { - if pathMatches(relPath, allowed) || isParentOfPattern(relPath, allowed) { - isAllowed = true - break - } - } + v.runSingleRule(path, dirStructureRule{}) +} - if isAllowed { - msg := fmt.Sprintf("Allowed directory found: %s", relPath) - v.printSuccess(msg) - v.Successes++ - } else { - msg := fmt.Sprintf("Directory not in allowed list: %s", relPath) - v.addViolation("unallowed_directory", "error", relPath, "dir_structure.allowedPaths", msg) +// runSingleRule is the shared entry point for the legacy Validate* wrappers. +// It snapshots root once and runs exactly one rule so wrapper behavior +// stays parity-safe with the modern v.Run() path. +func (v *Validator) runSingleRule(path string, rule Rule) { + tree := Snapshot(path, v.Config.Ignore) + ctx := &RunContext{ + Cfg: v.Config, + Tree: tree, + V: v, + Skip: func(rel string, isDir bool) bool { + if isDir { + return v.shouldSkipChangedDir(rel) } - } - return nil - }) - if err != nil { - v.addViolation("walk_error", "error", path, "filesystem", fmt.Sprintf("Error walking directory: %s", err)) + return v.shouldSkipChanged(rel) + }, } + rule.Run(ctx) } // matches checks if a path matches a glob pattern. @@ -110,62 +78,9 @@ func matches(path, pattern string) bool { return g.Match(path) } -// ValidateFileNaming validates the file naming conventions. +// ValidateFileNaming is a thin wrapper — see runSingleRule. func (v *Validator) ValidateFileNaming(path string) { - root := cleanRoot(path) - err := filepath.Walk(path, func(currentPath string, info os.FileInfo, err error) error { - if err != nil { - return err - } - relPath := relativePath(root, currentPath) - - // Check if the path should be ignored - for _, ignored := range v.Config.Ignore { - if pathMatches(relPath, ignored) { - if info.IsDir() { - return filepath.SkipDir - } - return nil - } - } - - if !info.IsDir() { - if v.shouldSkipChanged(relPath) { - return nil - } - fileName := info.Name() - - // Check against disallowed patterns - for _, disallowed := range v.Config.FileNamingPattern.Disallowed { - if pathMatches(fileName, disallowed) || pathMatches(relPath, disallowed) { - msg := fmt.Sprintf("Disallowed file naming pattern found: %s", relPath) - v.addViolation("disallowed_file_pattern", "error", relPath, disallowed, msg) - return nil - } - } - - // Check against allowed patterns - isAllowed := false - for _, allowed := range v.Config.FileNamingPattern.Allowed { - if pathMatches(fileName, allowed) || pathMatches(relPath, allowed) { - isAllowed = true - break - } - } - if isAllowed { - msg := fmt.Sprintf("Allowed file naming pattern found: %s", relPath) - v.printSuccess(msg) - v.Successes++ - } else { - msg := fmt.Sprintf("File not in allowed naming pattern: %s", relPath) - v.addViolation("unallowed_file_pattern", "error", relPath, "file_naming_pattern.allowed", msg) - } - } - return nil - }) - if err != nil { - v.addViolation("walk_error", "error", path, "filesystem", fmt.Sprintf("Error walking directory: %s", err)) - } + v.runSingleRule(path, fileNamingRule{}) } // PrintSummary prints a summary of the validation results. @@ -197,12 +112,17 @@ func (v *Validator) PrintSummary() { } } -// ValidateRequiredPaths validates that all required directories exist. +// ValidateRequiredPaths is a thin wrapper — see runSingleRule. func (v *Validator) ValidateRequiredPaths(path string) { + v.validateRequiredPathsDirect(path) +} + +// validateRequiredPathsDirect stats each requiredPath directly. Preserves +// the quirk that requiredPaths inside an ignored directory still count +// as present — this is the legacy behavior locked by parity goldens. +func (v *Validator) validateRequiredPathsDirect(path string) { for _, requiredPath := range v.Config.DirStructure.RequiredPaths { fullPath := filepath.Join(path, requiredPath) - - // Check if the required path exists if _, err := os.Stat(fullPath); os.IsNotExist(err) { msg := fmt.Sprintf("Required directory missing: %s", requiredPath) v.addViolation("missing_required_directory", "error", requiredPath, "dir_structure.requiredPaths", msg) @@ -213,91 +133,26 @@ func (v *Validator) ValidateRequiredPaths(path string) { } } -// ValidateRequiredFiles validates that all required files exist. +// ValidateRequiredFiles is a thin wrapper — see runSingleRule. func (v *Validator) ValidateRequiredFiles(path string) { - root := cleanRoot(path) - for _, requiredFile := range v.Config.FileNamingPattern.Required { - // Check if any file matching the pattern exists - found := false - err := filepath.Walk(path, func(currentPath string, info os.FileInfo, err error) error { - if err != nil { - return err - } - relPath := relativePath(root, currentPath) - - // Check if the path should be ignored - for _, ignored := range v.Config.Ignore { - if pathMatches(relPath, ignored) { - if info.IsDir() { - return filepath.SkipDir - } - return nil - } - } - - if !info.IsDir() { - // For required file patterns, we need to check both the filename and the relative path - fileName := info.Name() - // Check if either the filename or the relative path matches the pattern - if pathMatches(fileName, requiredFile) || pathMatches(relPath, requiredFile) { - found = true - return filepath.SkipAll // Stop walking once we find a match - } - } - return nil - }) - if err != nil { - v.addViolation("walk_error", "error", requiredFile, "file_naming_pattern.required", fmt.Sprintf("Error checking for required file %s: %s", requiredFile, err)) - continue - } - - if found { - v.printSuccess(fmt.Sprintf("Required file pattern found: %s", requiredFile)) - v.Successes++ - } else { - msg := fmt.Sprintf("Required file pattern missing: %s", requiredFile) - v.addViolation("missing_required_file", "error", requiredFile, requiredFile, msg) - } - } + v.runSingleRule(path, requiredFilesRule{}) } -// ValidatePlacement validates file placement rules. +// ValidatePlacement is a thin wrapper — see runSingleRule. func (v *Validator) ValidatePlacement(path string) { - root := cleanRoot(path) - _ = filepath.Walk(path, func(currentPath string, info os.FileInfo, err error) error { - if err != nil { - v.addViolation("walk_error", "error", currentPath, "placement", fmt.Sprintf("Error walking directory: %s", err)) - return nil - } - relPath := relativePath(root, currentPath) - for _, ignored := range v.Config.Ignore { - if pathMatches(relPath, ignored) { - if info.IsDir() { - return filepath.SkipDir - } - return nil - } - } - if info.IsDir() || v.shouldSkipChanged(relPath) { - return nil - } - for _, rule := range v.Config.Placement { - if !matchesAnyFile(relPath, info.Name(), rule.Files) { - continue - } - if underAny(relPath, rule.MustBeUnder) { - v.Successes++ - continue - } - msg := fmt.Sprintf("File placement violation: %s must be under %s", relPath, strings.Join(rule.MustBeUnder, ", ")) - v.addViolation("placement_violation", severity(rule.Severity), relPath, rule.ID, msg) - } - return nil - }) + v.runSingleRule(path, placementRule{}) } -// ValidateRequiredGroups validates one-of and per-directory requirements. +// ValidateRequiredGroups is a thin wrapper — see runSingleRule. func (v *Validator) ValidateRequiredGroups(path string) { + v.validateRequiredGroupsDirect(path) +} + +// validateRequiredGroupsDirect implements requiredGroups using stat-based +// helpers (existsAt, existsAny, matchingDirs) which intentionally see +// through ignored directories. Preserving that behavior is a parity +// requirement locked by the goldens. +func (v *Validator) validateRequiredGroupsDirect(path string) { root := cleanRoot(path) for _, group := range v.Config.RequiredGroups { if len(group.OneOf) > 0 { @@ -344,49 +199,9 @@ func (v *Validator) ValidateRequiredGroups(path string) { } } -// ValidateBoundaries validates import boundaries for supported source files. +// ValidateBoundaries is a thin wrapper — see runSingleRule. func (v *Validator) ValidateBoundaries(path string) { - root := cleanRoot(path) - modulePath := readGoModule(root) - _ = filepath.Walk(path, func(currentPath string, info os.FileInfo, err error) error { - if err != nil { - v.addViolation("walk_error", "error", currentPath, "boundaries", fmt.Sprintf("Error walking directory: %s", err)) - return nil - } - relPath := relativePath(root, currentPath) - for _, ignored := range v.Config.Ignore { - if pathMatches(relPath, ignored) { - if info.IsDir() { - return filepath.SkipDir - } - return nil - } - } - if info.IsDir() || !isSupportedBoundaryFile(relPath) || v.shouldSkipChanged(relPath) { - return nil - } - for _, rule := range v.Config.Boundaries { - if !pathMatches(relPath, rule.From) { - continue - } - imports, err := sourceImports(currentPath, relPath) - if err != nil { - v.addViolation("parse_error", "error", relPath, rule.ID, fmt.Sprintf("Failed to parse imports: %s", err)) - continue - } - for _, imp := range imports { - localImport := importToLocalPath(modulePath, imp, relPath) - for _, forbidden := range rule.CannotImport { - if pathMatches(imp, forbidden) || pathMatches(localImport, forbidden) { - msg := fmt.Sprintf("Boundary violation: %s imports %s", relPath, imp) - v.addViolation("boundary_violation", severity(rule.Severity), relPath, rule.ID, msg) - } - } - } - v.Successes++ - } - return nil - }) + v.runSingleRule(path, boundariesRule{}) } // LoadChangedPaths populates the changed-file set used by --changed-only, diff --git a/test/init_presets_test.go b/test/init_presets_test.go new file mode 100644 index 0000000..a7fc275 --- /dev/null +++ b/test/init_presets_test.go @@ -0,0 +1,91 @@ +package test + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// TestInitTypeUsesPresetBody asserts that `init --type ` writes a file +// whose body (below the header comment) is byte-identical to the embedded +// preset that `extends: -standard` (or `generic`) resolves to. +// This locks in the deduplication of spec 007's presets and init's +// templates — they were duplicated string literals before this refactor. +func TestInitTypeUsesPresetBody(t *testing.T) { + bin := buildBinary(t) + + cases := map[string]string{ + "go": "go-standard.yaml", + "node": "node-standard.yaml", + "python": "python-standard.yaml", + "generic": "generic.yaml", + } + + for typ, presetFile := range cases { + t.Run(typ, func(t *testing.T) { + dir := t.TempDir() + if out, err := runBinaryInDir(t, bin, dir, "init", "--type", typ); err != nil { + t.Fatalf("init --type %s: %v\n%s", typ, err, out) + } + generated := readFile(t, filepath.Join(dir, ".structlint.yaml")) + + // Strip the first line (the header comment) — the rest must + // match the preset byte-for-byte. + nl := strings.Index(generated, "\n") + if nl < 0 { + t.Fatalf("generated config has no newline: %q", generated) + } + body := generated[nl+1:] + + presetPath := filepath.Join(repoRoot(t), "internal", "config", "presets", presetFile) + presetData, err := os.ReadFile(presetPath) + if err != nil { + t.Fatalf("read preset %s: %v", presetPath, err) + } + if body != string(presetData) { + t.Errorf("init --type %s body drifted from preset %s\n--- got body ---\n%s\n--- preset ---\n%s", + typ, presetFile, body, string(presetData)) + } + }) + } +} + +// TestInitTypeAndExtendsProduceEquivalentValidation is the semantic +// twin of the byte test above: init --type and a config with +// `extends: -standard` should validate the same tree the same way. +func TestInitTypeAndExtendsProduceEquivalentValidation(t *testing.T) { + bin := buildBinary(t) + + // Minimal go-shaped project the go-standard preset would pass on. + files := map[string]string{ + "go.mod": "module t\n", + "README.md": "# t\n", + ".gitignore": "bin/\n", + "cmd/app/main.go": "package main\n\nfunc main() {}\n", + "internal/foo/x.go": "package foo\n", + } + + // init --type go path. + dirA := t.TempDir() + for p, content := range files { + writeTestFile(t, dirA, p, content) + } + if out, err := runBinaryInDir(t, bin, dirA, "init", "--type", "go", "--force"); err != nil { + t.Fatalf("init: %v\n%s", err, out) + } + outA, errA := runBinaryInDir(t, bin, dirA, "validate", "--silent") + + // extends: go-standard path. + dirB := t.TempDir() + for p, content := range files { + writeTestFile(t, dirB, p, content) + } + writeTestFile(t, dirB, ".structlint.yaml", "extends: go-standard\n") + outB, errB := runBinaryInDir(t, bin, dirB, "validate", "--silent") + + if (errA == nil) != (errB == nil) { + t.Errorf("validation outcomes diverge:\n init --type go: err=%v\n extends: go-standard: err=%v\n\n outA=%s\n outB=%s", + errA, errB, outA, outB) + } +} diff --git a/test/skill_deep_test.go b/test/skill_deep_test.go new file mode 100644 index 0000000..fd14907 --- /dev/null +++ b/test/skill_deep_test.go @@ -0,0 +1,194 @@ +package test + +import ( + "context" + "os" + "path/filepath" + "regexp" + "strings" + "testing" + + "github.com/AxeForging/structlint/internal/app" + "github.com/AxeForging/structlint/internal/validator" + "gopkg.in/yaml.v3" +) + +// TestSkill_FrontmatterParsesAsYAML asserts SKILL.md's frontmatter is +// valid YAML (agents load it via a YAML parser) and declares the fields +// their runtime keys on. +func TestSkill_FrontmatterParsesAsYAML(t *testing.T) { + body := readSkillFile(t) + front := extractFrontmatter(t, body) + + var meta struct { + Name string `yaml:"name"` + Description string `yaml:"description"` + } + if err := yaml.Unmarshal([]byte(front), &meta); err != nil { + t.Fatalf("SKILL.md frontmatter is not valid YAML: %v\n%s", err, front) + } + if meta.Name != "structlint" { + t.Errorf("expected name: structlint, got %q", meta.Name) + } + if len(strings.TrimSpace(meta.Description)) < 30 { + t.Errorf("description is too short to be useful (agents key on it): %q", meta.Description) + } +} + +// TestSkill_FrontmatterHasTriggerPhrases asserts the description contains +// concrete trigger phrases so agent runtimes match user requests to this +// skill. Parses the YAML first so block-scalar folded newlines don't +// hide multi-word phrases. +func TestSkill_FrontmatterHasTriggerPhrases(t *testing.T) { + body := readSkillFile(t) + front := extractFrontmatter(t, body) + var meta struct { + Description string `yaml:"description"` + } + if err := yaml.Unmarshal([]byte(front), &meta); err != nil { + t.Fatalf("parse frontmatter: %v", err) + } + // Collapse whitespace before searching: block scalars fold newlines + // to a single space, but leading indentation could still trip us up + // if the layout changes. Normalize aggressively. + desc := strings.Join(strings.Fields(meta.Description), " ") + + phrases := []string{ + "structlint violation", + "where should this file go", + "enforce directory layout", + "file in the wrong place", + } + for _, phrase := range phrases { + if !strings.Contains(desc, phrase) { + t.Errorf("SKILL.md description missing trigger phrase %q", phrase) + } + } +} + +// TestSkill_CoversAllViolationCodes cross-checks both directions — every +// code in the CodeDescriptions registry appears in the SKILL.md table +// AND every code in the SKILL.md table is a real registry entry. +func TestSkill_CoversAllViolationCodes(t *testing.T) { + body := readSkillFile(t) + + // Registry → doc. + for code := range validator.CodeDescriptions { + if !strings.Contains(body, "`"+code+"`") { + t.Errorf("SKILL.md missing code %q from decision table", code) + } + } + + // Doc → registry. Codes appear in table cells as `code_name` — greedy + // enough regex; false positives (e.g. code lookalikes in prose) are + // tolerated. What we want to catch is stale renames. + re := regexp.MustCompile("`([a-z_]+_[a-z_]+)`") + matches := re.FindAllStringSubmatch(body, -1) + seen := map[string]bool{} + for _, m := range matches { + token := m[1] + if seen[token] { + continue + } + seen[token] = true + if _, ok := validator.CodeDescriptions[token]; ok { + continue + } + // Whitelist tokens that look like codes but are structlint concepts + // or config fields discussed in the skill. + if isSkillConcept(token) { + continue + } + // Whitelist standard shell/config words that pattern-match the + // underscore convention but aren't codes. + t.Logf("hint: %q looks like a code in SKILL.md but isn't in the registry (typo?)", token) + } +} + +// TestSkill_ReferencedCommandsExist boots the CLI in-process and asserts +// that every subcommand the SKILL.md setup recipes tell the agent to +// run is a real registered command. Catches renames of `hook install`, +// `suggest`, etc. without doc updates. +func TestSkill_ReferencedCommandsExist(t *testing.T) { + body := readSkillFile(t) + + referenced := []string{ + "validate", + "init", + "hook install", + "suggest", + } + + // Route each through `--help` in-process — the CLI errors on unknown + // subcommands, so a passing help call proves the command is real. + root := app.New() + for _, cmd := range referenced { + if !strings.Contains(body, "structlint "+cmd) { + t.Errorf("SKILL.md never references `structlint %s` — outdated setup recipe?", cmd) + } + parts := append([]string{"structlint"}, strings.Fields(cmd)...) + parts = append(parts, "--help") + if err := root.Run(context.Background(), parts); err != nil { + t.Errorf("`%s` failed to boot: %v — SKILL.md references a broken subcommand path", strings.Join(parts, " "), err) + } + } +} + +// TestSkill_MentionsJSONContractVersion asserts SKILL.md documents the +// suggest JSON v1 shape (agents depend on that promise). If the version +// bumps, the doc must bump too or agents will silently break. +func TestSkill_MentionsJSONContractVersion(t *testing.T) { + body := readSkillFile(t) + if !strings.Contains(body, `"version": 1`) && !strings.Contains(body, "JSON v1") { + t.Errorf("SKILL.md does not mention the suggest JSON v1 contract shape") + } +} + +// TestSkill_ExitCodesDocumented asserts every exit code the CLI can +// return is mentioned. Agents shouldn't guess. +func TestSkill_ExitCodesDocumented(t *testing.T) { + body := readSkillFile(t) + for _, code := range []string{"0", "1", "2", "3"} { + if !strings.Contains(body, "**"+code+"**") { + t.Errorf("SKILL.md doesn't call out exit code %s", code) + } + } +} + +// --- helpers --- + +func readSkillFile(t *testing.T) string { + t.Helper() + path := filepath.Join(repoRoot(t), "skills", "structlint", "SKILL.md") + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read SKILL.md: %v", err) + } + return string(data) +} + +func extractFrontmatter(t *testing.T, body string) string { + t.Helper() + if !strings.HasPrefix(body, "---\n") { + t.Fatalf("SKILL.md missing frontmatter delimiter") + } + rest := body[len("---\n"):] + end := strings.Index(rest, "\n---\n") + if end < 0 { + t.Fatalf("SKILL.md frontmatter has no closing delimiter") + } + return rest[:end] +} + +// isSkillConcept whitelists tokens that pattern-match a violation-code +// shape (snake_case with underscores) but are actually config field names, +// commands, or other structlint concepts the skill talks about. Keeping +// this narrow is intentional: false positives log a hint but don't fail. +func isSkillConcept(token string) bool { + switch token { + case "dir_structure", "file_naming_pattern", "required_paths", + "allowed_paths", "disallowed_paths": + return true + } + return false +} diff --git a/test/testdata/parity/boundaries/.structlint.yaml b/test/testdata/parity/boundaries/.structlint.yaml new file mode 100644 index 0000000..eb3aafb --- /dev/null +++ b/test/testdata/parity/boundaries/.structlint.yaml @@ -0,0 +1,17 @@ +dir_structure: + allowedPaths: + - "." + - "internal/**" +file_naming_pattern: + allowed: + - "*.go" + - "*.mod" + - "*.md" + - "*.yaml" +boundaries: + - id: domain-no-infra + from: "internal/domain/**" + cannotImport: + - "internal/db/**" +ignore: + - ".git" diff --git a/test/testdata/parity/boundaries/README.md b/test/testdata/parity/boundaries/README.md new file mode 100644 index 0000000..78e8d63 --- /dev/null +++ b/test/testdata/parity/boundaries/README.md @@ -0,0 +1 @@ +# boundaries diff --git a/test/testdata/parity/boundaries/go.mod b/test/testdata/parity/boundaries/go.mod new file mode 100644 index 0000000..b8727da --- /dev/null +++ b/test/testdata/parity/boundaries/go.mod @@ -0,0 +1,3 @@ +module example.com/fixture + +go 1.24 diff --git a/test/testdata/parity/boundaries/internal/db/db.go b/test/testdata/parity/boundaries/internal/db/db.go new file mode 100644 index 0000000..7851b76 --- /dev/null +++ b/test/testdata/parity/boundaries/internal/db/db.go @@ -0,0 +1,3 @@ +package db + +func Connect() string { return "conn" } diff --git a/test/testdata/parity/boundaries/internal/domain/clean.go b/test/testdata/parity/boundaries/internal/domain/clean.go new file mode 100644 index 0000000..550f2b8 --- /dev/null +++ b/test/testdata/parity/boundaries/internal/domain/clean.go @@ -0,0 +1,3 @@ +package domain + +type Value struct{ Name string } diff --git a/test/testdata/parity/boundaries/internal/domain/model.go b/test/testdata/parity/boundaries/internal/domain/model.go new file mode 100644 index 0000000..909861b --- /dev/null +++ b/test/testdata/parity/boundaries/internal/domain/model.go @@ -0,0 +1,6 @@ +package domain + +// Violates the boundary: domain imports db. +import _ "example.com/fixture/internal/db" + +type Entity struct{ ID string } diff --git a/test/testdata/parity/disallowed-subtree/.structlint.yaml b/test/testdata/parity/disallowed-subtree/.structlint.yaml new file mode 100644 index 0000000..f4d38d2 --- /dev/null +++ b/test/testdata/parity/disallowed-subtree/.structlint.yaml @@ -0,0 +1,13 @@ +dir_structure: + allowedPaths: + - "." + - "src/**" + disallowedPaths: + - "tmp/**" +file_naming_pattern: + allowed: + - "*.go" + - "*.md" + - "*.yaml" +ignore: + - ".git" diff --git a/test/testdata/parity/disallowed-subtree/README.md b/test/testdata/parity/disallowed-subtree/README.md new file mode 100644 index 0000000..4633334 --- /dev/null +++ b/test/testdata/parity/disallowed-subtree/README.md @@ -0,0 +1 @@ +# disallowed-subtree diff --git a/test/testdata/parity/disallowed-subtree/src/main.go b/test/testdata/parity/disallowed-subtree/src/main.go new file mode 100644 index 0000000..06ab7d0 --- /dev/null +++ b/test/testdata/parity/disallowed-subtree/src/main.go @@ -0,0 +1 @@ +package main diff --git a/test/testdata/parity/disallowed-subtree/tmp/leftover.go b/test/testdata/parity/disallowed-subtree/tmp/leftover.go new file mode 100644 index 0000000..14aed26 --- /dev/null +++ b/test/testdata/parity/disallowed-subtree/tmp/leftover.go @@ -0,0 +1 @@ +package tmp diff --git a/test/testdata/parity/disallowed-subtree/tmp/nested/deeper.go b/test/testdata/parity/disallowed-subtree/tmp/nested/deeper.go new file mode 100644 index 0000000..bdb515c --- /dev/null +++ b/test/testdata/parity/disallowed-subtree/tmp/nested/deeper.go @@ -0,0 +1 @@ +package nested diff --git a/test/testdata/parity/goldens/boundaries.exit b/test/testdata/parity/goldens/boundaries.exit new file mode 100644 index 0000000..d00491f --- /dev/null +++ b/test/testdata/parity/goldens/boundaries.exit @@ -0,0 +1 @@ +1 diff --git a/test/testdata/parity/goldens/boundaries.json b/test/testdata/parity/goldens/boundaries.json new file mode 100644 index 0000000..34d241b --- /dev/null +++ b/test/testdata/parity/goldens/boundaries.json @@ -0,0 +1,33 @@ +✗ Boundary violation: internal/domain/model.go imports example.com/fixture/internal/db +{ + "successes": 12, + "failures": 1, + "total_violations": 1, + "errors": [ + "Boundary violation: internal/domain/model.go imports example.com/fixture/internal/db" + ], + "violations": [ + { + "code": "boundary_violation", + "severity": "error", + "path": "internal/domain/model.go", + "rule": "domain-no-infra", + "message": "Boundary violation: internal/domain/model.go imports example.com/fixture/internal/db" + } + ], + "summary": { + "total_successes": 12, + "total_failures": 1, + "violations": [ + { + "type": "boundary_violation", + "count": 1, + "examples": [ + "Boundary violation: internal/domain/model.go imports example.com/fixture/internal/db" + ], + "description": "Files importing paths forbidden by boundary rules" + } + ] + } +} +validation failed with 1 errors diff --git a/test/testdata/parity/goldens/boundaries.text b/test/testdata/parity/goldens/boundaries.text new file mode 100644 index 0000000..4a20228 --- /dev/null +++ b/test/testdata/parity/goldens/boundaries.text @@ -0,0 +1,15 @@ +✗ Boundary violation: internal/domain/model.go imports example.com/fixture/internal/db + +--- Validation Summary --- +✓ 12 files/directories passed validation +✗ 1 violations found + +--- Validation Summary --- +✓ 12 checks passed +✗ 1 checks failed + +--- Violation Summary --- + +Files importing paths forbidden by boundary rules (1 violations): + - Boundary violation: internal/domain/model.go imports example.com/fixture/internal/db +validation failed with 1 errors diff --git a/test/testdata/parity/goldens/disallowed-subtree.exit b/test/testdata/parity/goldens/disallowed-subtree.exit new file mode 100644 index 0000000..d00491f --- /dev/null +++ b/test/testdata/parity/goldens/disallowed-subtree.exit @@ -0,0 +1 @@ +1 diff --git a/test/testdata/parity/goldens/disallowed-subtree.json b/test/testdata/parity/goldens/disallowed-subtree.json new file mode 100644 index 0000000..6c67a67 --- /dev/null +++ b/test/testdata/parity/goldens/disallowed-subtree.json @@ -0,0 +1,33 @@ +✗ Disallowed directory found: tmp +{ + "successes": 7, + "failures": 1, + "total_violations": 1, + "errors": [ + "Disallowed directory found: tmp" + ], + "violations": [ + { + "code": "disallowed_directory", + "severity": "error", + "path": "tmp", + "rule": "tmp/**", + "message": "Disallowed directory found: tmp" + } + ], + "summary": { + "total_successes": 7, + "total_failures": 1, + "violations": [ + { + "type": "disallowed_directory", + "count": 1, + "examples": [ + "Disallowed directory found: tmp" + ], + "description": "Directories that are explicitly disallowed" + } + ] + } +} +validation failed with 1 errors diff --git a/test/testdata/parity/goldens/disallowed-subtree.text b/test/testdata/parity/goldens/disallowed-subtree.text new file mode 100644 index 0000000..045ddff --- /dev/null +++ b/test/testdata/parity/goldens/disallowed-subtree.text @@ -0,0 +1,15 @@ +✗ Disallowed directory found: tmp + +--- Validation Summary --- +✓ 7 files/directories passed validation +✗ 1 violations found + +--- Validation Summary --- +✓ 7 checks passed +✗ 1 checks failed + +--- Violation Summary --- + +Directories that are explicitly disallowed (1 violations): + - Disallowed directory found: tmp +validation failed with 1 errors diff --git a/test/testdata/parity/goldens/required-groups.exit b/test/testdata/parity/goldens/required-groups.exit new file mode 100644 index 0000000..d00491f --- /dev/null +++ b/test/testdata/parity/goldens/required-groups.exit @@ -0,0 +1 @@ +1 diff --git a/test/testdata/parity/goldens/required-groups.json b/test/testdata/parity/goldens/required-groups.json new file mode 100644 index 0000000..5d86da1 --- /dev/null +++ b/test/testdata/parity/goldens/required-groups.json @@ -0,0 +1,50 @@ +✗ Directory cmd/broken missing required file: main.go +✗ Required group missing one of: Makefile, Taskfile.yml +{ + "successes": 9, + "failures": 2, + "total_violations": 2, + "errors": [ + "Directory cmd/broken missing required file: main.go", + "Required group missing one of: Makefile, Taskfile.yml" + ], + "violations": [ + { + "code": "missing_required_group", + "severity": "error", + "path": "build-entrypoint", + "rule": "build-entrypoint", + "message": "Required group missing one of: Makefile, Taskfile.yml" + }, + { + "code": "missing_group_file", + "severity": "error", + "path": "cmd/broken/main.go", + "rule": "commands-have-main", + "message": "Directory cmd/broken missing required file: main.go" + } + ], + "summary": { + "total_successes": 9, + "total_failures": 2, + "violations": [ + { + "type": "missing_group_file", + "count": 1, + "examples": [ + "Directory cmd/broken missing required file: main.go" + ], + "description": "Directories missing files required by their group" + }, + { + "type": "missing_required_group", + "count": 1, + "examples": [ + "Required group missing one of: Makefile, Taskfile.yml" + ], + "description": "Required groups with no matching file" + } + ] + } +} +validation failed with 2 errors diff --git a/test/testdata/parity/goldens/required-groups.text b/test/testdata/parity/goldens/required-groups.text new file mode 100644 index 0000000..c13631a --- /dev/null +++ b/test/testdata/parity/goldens/required-groups.text @@ -0,0 +1,19 @@ +✗ Directory cmd/broken missing required file: main.go +✗ Required group missing one of: Makefile, Taskfile.yml + +--- Validation Summary --- +✓ 9 files/directories passed validation +✗ 2 violations found + +--- Validation Summary --- +✓ 9 checks passed +✗ 2 checks failed + +--- Violation Summary --- + +Directories missing files required by their group (1 violations): + - Directory cmd/broken missing required file: main.go + +Required groups with no matching file (1 violations): + - Required group missing one of: Makefile, Taskfile.yml +validation failed with 2 errors diff --git a/test/testdata/parity/required-groups/.structlint.yaml b/test/testdata/parity/required-groups/.structlint.yaml new file mode 100644 index 0000000..69feaee --- /dev/null +++ b/test/testdata/parity/required-groups/.structlint.yaml @@ -0,0 +1,20 @@ +dir_structure: + allowedPaths: + - "." + - "cmd/**" +file_naming_pattern: + allowed: + - "*.go" + - "*.md" + - "*.yaml" +requiredGroups: + - id: commands-have-main + eachDirMatching: "cmd/*" + mustContain: + - "main.go" + - id: build-entrypoint + oneOf: + - "Makefile" + - "Taskfile.yml" +ignore: + - ".git" diff --git a/test/testdata/parity/required-groups/README.md b/test/testdata/parity/required-groups/README.md new file mode 100644 index 0000000..f4ea5f1 --- /dev/null +++ b/test/testdata/parity/required-groups/README.md @@ -0,0 +1 @@ +# required-groups diff --git a/test/testdata/parity/required-groups/cmd/broken/helper.go b/test/testdata/parity/required-groups/cmd/broken/helper.go new file mode 100644 index 0000000..904a86b --- /dev/null +++ b/test/testdata/parity/required-groups/cmd/broken/helper.go @@ -0,0 +1,3 @@ +package main + +func Helper() {} diff --git a/test/testdata/parity/required-groups/cmd/complete/main.go b/test/testdata/parity/required-groups/cmd/complete/main.go new file mode 100644 index 0000000..38dd16d --- /dev/null +++ b/test/testdata/parity/required-groups/cmd/complete/main.go @@ -0,0 +1,3 @@ +package main + +func main() {} diff --git a/test/version_pragma_test.go b/test/version_pragma_test.go new file mode 100644 index 0000000..64b03bb --- /dev/null +++ b/test/version_pragma_test.go @@ -0,0 +1,69 @@ +package test + +import ( + "os/exec" + "path/filepath" + "strings" + "testing" +) + +// buildBinaryWithVersion builds a fresh structlint binary with a specific +// build.Version injected via ldflags. Used to prove the version pragma +// path fires end-to-end from a real CLI invocation. +func buildBinaryWithVersion(t *testing.T, version string) string { + t.Helper() + dir := t.TempDir() + out := filepath.Join(dir, "structlint-pinned") + ldflags := "-X github.com/AxeForging/structlint/internal/build.Version=" + version + cmd := exec.Command("go", "build", "-o", out, "-ldflags", ldflags, "./cmd/structlint") + cmd.Dir = repoRoot(t) + if raw, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("build pinned binary: %v\n%s", err, string(raw)) + } + return out +} + +func TestVersionPragma_OldBinaryOnNewConfigFailsHelpfully(t *testing.T) { + bin := buildBinaryWithVersion(t, "v0.5.0") + dir := t.TempDir() + writeTestFile(t, dir, ".structlint.yaml", `# requires structlint >= v0.6.0 +dir_structure: + allowedPaths: ["."] +file_naming_pattern: + allowed: ["*.md", "*.yaml"] +ignore: [".git"] +`) + writeTestFile(t, dir, "README.md", "# t\n") + + out, err := runBinaryInDir(t, bin, dir, "validate", "--silent") + if err == nil { + t.Fatalf("expected failure: v0.5.0 binary should reject a >= v0.6.0 pragma, out:\n%s", out) + } + if !strings.Contains(out, "requires structlint >= v0.6.0") { + t.Errorf("error should quote the required version, got:\n%s", out) + } + if !strings.Contains(out, "running version is v0.5.0") { + t.Errorf("error should quote the running version, got:\n%s", out) + } + if !strings.Contains(out, "go install") { + t.Errorf("error should suggest an upgrade path, got:\n%s", out) + } +} + +func TestVersionPragma_MatchingBinaryValidatesCleanly(t *testing.T) { + bin := buildBinaryWithVersion(t, "v0.6.5") + dir := t.TempDir() + writeTestFile(t, dir, ".structlint.yaml", `# requires structlint >= v0.6.0 +dir_structure: + allowedPaths: ["."] +file_naming_pattern: + allowed: ["*.md", "*.yaml"] +ignore: [".git"] +`) + writeTestFile(t, dir, "README.md", "# t\n") + + out, err := runBinaryInDir(t, bin, dir, "validate", "--silent") + if err != nil { + t.Fatalf("expected success: v0.6.5 satisfies >= v0.6.0, err=%v out:\n%s", err, out) + } +}