diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index ddfdc31..5c3a935 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -146,7 +146,7 @@ jobs: with: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} AI_PROVIDER: gemini - AI_MODEL: gemini-2.5-flash + AI_MODEL: gemini-3.1-flash-lite-preview AI_API_KEY: ${{ secrets.GEMINI_API_KEY }} SHOW_TOKEN_USAGE: true INCREMENTAL: false diff --git a/actions/list.go b/actions/list.go index b0ddb76..efdf590 100644 --- a/actions/list.go +++ b/actions/list.go @@ -2,6 +2,7 @@ package actions import ( "fmt" + "sort" "strings" "github.com/AxeForging/yamlspec/services" @@ -71,10 +72,16 @@ func (a *ListAction) listTags(testDir string) error { return nil } + tags := make([]string, 0, len(tagSet)) + for tag := range tagSet { + tags = append(tags, tag) + } + sort.Strings(tags) + fmt.Printf("%-30s %s\n", "TAG", "SPECS") fmt.Printf("%-30s %s\n", strings.Repeat("-", 28), strings.Repeat("-", 8)) - for tag, count := range tagSet { - fmt.Printf("%-30s %d\n", tag, count) + for _, tag := range tags { + fmt.Printf("%-30s %d\n", tag, tagSet[tag]) } return nil diff --git a/actions/validate.go b/actions/validate.go index 69b47dc..7c3da8b 100644 --- a/actions/validate.go +++ b/actions/validate.go @@ -31,6 +31,7 @@ func (a *ValidateAction) Execute(c *cli.Context) error { Tags: c.StringSlice("tag"), Workers: c.Int("workers"), FailFast: c.Bool("fail-fast"), + PreRunTimeout: c.Duration("pre-run-timeout"), Verbose: c.GlobalBool("verbose"), Quiet: c.Bool("quiet"), JSONOutput: c.String("json-output"), @@ -40,6 +41,11 @@ func (a *ValidateAction) Execute(c *cli.Context) error { JUnitOutput: c.String("junit-output"), } + if config.FailFast && config.Workers > 1 { + return fmt.Errorf("--fail-fast cannot be used with --workers > 1 " + + "(parallel workers can't honor ordered short-circuit; pick one)") + } + specs, err := a.discovery.DiscoverSpecs(config.TestDir, config.Tags) if err != nil { return fmt.Errorf("discovery failed: %w", err) diff --git a/domain/config.go b/domain/config.go index 1bdce77..48fc4c4 100644 --- a/domain/config.go +++ b/domain/config.go @@ -1,13 +1,16 @@ package domain +import "time" + // Config holds all CLI configuration type Config struct { - TestDir string - Tags []string - Workers int - FailFast bool - Verbose bool - Quiet bool + TestDir string + Tags []string + Workers int + FailFast bool + Verbose bool + Quiet bool + PreRunTimeout time.Duration // Output format flags (file paths; empty means skip) JSONOutput string @@ -20,7 +23,8 @@ type Config struct { // DefaultConfig returns a Config with sensible defaults func DefaultConfig() *Config { return &Config{ - TestDir: "tests", - Workers: 1, + TestDir: "tests", + Workers: 1, + PreRunTimeout: 60 * time.Second, } } diff --git a/domain/models.go b/domain/models.go index d371142..d7bb81c 100644 --- a/domain/models.go +++ b/domain/models.go @@ -69,6 +69,12 @@ type Assertion struct { ToHaveMaxLength *int `yaml:"toHaveMaxLength,omitempty"` } +// HasAnyOperator returns true if the assertion has any operator set, +// including existence/null checks. +func (a *Assertion) HasAnyOperator() bool { + return a.ToExist != nil || a.ToBeNull != nil || a.HasValueOperators() +} + // HasValueOperators returns true if the assertion has any value-based operators set func (a *Assertion) HasValueOperators() bool { return a.ToEqual != nil || diff --git a/flags.go b/flags.go index 9013751..f6a458f 100644 --- a/flags.go +++ b/flags.go @@ -1,6 +1,10 @@ package main -import "github.com/urfave/cli" +import ( + "time" + + "github.com/urfave/cli" +) var ( verboseFlag = cli.BoolFlag{ @@ -27,7 +31,12 @@ var ( } failFastFlag = cli.BoolFlag{ Name: "fail-fast", - Usage: "Stop on first failure", + Usage: "Stop on first failure (not compatible with --workers > 1)", + } + preRunTimeoutFlag = cli.DurationFlag{ + Name: "pre-run-timeout", + Usage: "Max duration for each pre_run command (e.g. 30s, 2m)", + Value: 60 * time.Second, } jsonOutputFlag = cli.StringFlag{ Name: "json-output", diff --git a/go.mod b/go.mod index 225c6fa..eed8673 100644 --- a/go.mod +++ b/go.mod @@ -1,6 +1,6 @@ module github.com/AxeForging/yamlspec -go 1.25.0 +go 1.25.8 require ( github.com/itchyny/gojq v0.12.18 diff --git a/integration/list_test.go b/integration/list_test.go index 65436d9..8dd487a 100644 --- a/integration/list_test.go +++ b/integration/list_test.go @@ -40,3 +40,36 @@ func TestList_Tags(t *testing.T) { t.Error("expected 'staging' tag in output") } } + +func TestList_TagsSorted(t *testing.T) { + bin := buildBinary(t) + + output, exitCode := run(t, bin, "list", "--test-dir", "integration/testdata", "--tags") + if exitCode != 0 { + t.Fatalf("expected exit code 0, got %d", exitCode) + } + + // Extract tag column (first token of each data line) + var tags []string + for _, line := range strings.Split(output, "\n") { + line = strings.TrimSpace(line) + if line == "" || strings.HasPrefix(line, "TAG") || strings.HasPrefix(line, "-") { + continue + } + fields := strings.Fields(line) + if len(fields) < 2 { + continue + } + tags = append(tags, fields[0]) + } + + if len(tags) < 2 { + t.Fatalf("expected multiple tags, got: %v", tags) + } + for i := 1; i < len(tags); i++ { + if tags[i] < tags[i-1] { + t.Errorf("tags not sorted: %v (out of order: %q before %q)", tags, tags[i-1], tags[i]) + break + } + } +} diff --git a/integration/validate_test.go b/integration/validate_test.go index 3591cba..c53eb94 100644 --- a/integration/validate_test.go +++ b/integration/validate_test.go @@ -8,6 +8,7 @@ import ( "path/filepath" "strings" "testing" + "time" ) func repoRoot(t *testing.T) string { @@ -959,3 +960,65 @@ func TestE2E_Help(t *testing.T) { t.Error("expected 'init' command in help") } } + +// --- Correctness hardening --- + +func TestE2E_FailFastWithWorkersRejected(t *testing.T) { + bin := buildBinary(t) + + output, exitCode := run(t, bin, "validate", + "--test-dir", "integration/testdata", + "--fail-fast", "--workers", "4", + ) + + if exitCode == 0 { + t.Errorf("expected non-zero exit when combining --fail-fast and --workers>1, got 0\n%s", output) + } + if !strings.Contains(output, "fail-fast") || !strings.Contains(output, "workers") { + t.Errorf("expected error to mention both flags, got:\n%s", output) + } +} + +func TestE2E_PreRunTimeout(t *testing.T) { + bin := buildBinary(t) + tmpDir := t.TempDir() + specDir := filepath.Join(tmpDir, "slow-prerun") + if err := os.MkdirAll(specDir, 0755); err != nil { + t.Fatal(err) + } + spec := `name: "Slow pre-run" +tags: ["slow"] +pre_run: + - sleep 5 +describe: + - name: "never runs" + select: 'select(.kind == "Deployment")' + it: + - should: "placeholder" + expect: metadata.name + toExist: true +` + if err := os.WriteFile(filepath.Join(specDir, "spec.yaml"), []byte(spec), 0644); err != nil { + t.Fatal(err) + } + // A manifest has to exist for discovery; it won't be reached. + if err := os.WriteFile(filepath.Join(specDir, "dep.yaml"), []byte("kind: Deployment\nmetadata:\n name: foo\n"), 0644); err != nil { + t.Fatal(err) + } + + start := time.Now() + output, exitCode := run(t, bin, "validate", "--test-dir", tmpDir, "--pre-run-timeout", "500ms") + elapsed := time.Since(start) + + if exitCode == 0 { + t.Error("expected non-zero exit when pre_run times out") + } + if !strings.Contains(output, "timed out") { + t.Errorf("expected 'timed out' in output, got:\n%s", output) + } + // Should kill at ~500ms, well before the 5s sleep. Allow slack for process + // startup on slow CI, but anything past 3s means we didn't actually kill it. + if elapsed > 3*time.Second { + t.Errorf("pre-run timeout didn't cancel: took %s", elapsed) + } +} diff --git a/main.go b/main.go index ab71902..0762a38 100644 --- a/main.go +++ b/main.go @@ -50,6 +50,7 @@ func main() { tagFlag, workersFlag, failFastFlag, + preRunTimeoutFlag, quietFlag, jsonOutputFlag, yamlOutputFlag, diff --git a/services/assertion.go b/services/assertion.go index e9f047f..a274949 100644 --- a/services/assertion.go +++ b/services/assertion.go @@ -2,6 +2,7 @@ package services import ( "fmt" + "io" "reflect" "regexp" "strconv" @@ -80,59 +81,72 @@ func (ae *AssertionEngine) evaluateAssertion(assertion *domain.Assertion, resour fieldPath := assertion.Expect for _, resource := range resources { - fieldValue, exists, err := ae.extractFieldValue(fieldPath, resource) + values, err := ae.extractFieldValues(fieldPath, resource) if err != nil { result.Status = domain.StatusError result.Error = fmt.Sprintf("failed to access '%s': %v", fieldPath, err) return result } - result.Actual = fieldValue - - // Existence checks - if assertion.ToExist != nil { - if *assertion.ToExist && !exists { - result.Status = domain.StatusFailed - result.Error = fmt.Sprintf("expected '%s' to exist", fieldPath) - return result - } - if !*assertion.ToExist && exists { + // No values produced: the path yielded nothing (missing field, or wildcard + // over an empty/missing array). Treat as "does not exist" for a single + // virtual value, so toExist / toBeNull semantics still work. + if len(values) == 0 { + if failed, err := ae.checkSingle(assertion, fieldPath, nil, false); failed { result.Status = domain.StatusFailed - result.Error = fmt.Sprintf("expected '%s' to not exist, got: %v", fieldPath, fieldValue) + result.Error = err + result.Actual = nil return result } + continue } - // Null checks - if assertion.ToBeNull != nil { - isNull := fieldValue == nil - if *assertion.ToBeNull && !isNull { - result.Status = domain.StatusFailed - result.Error = fmt.Sprintf("expected '%s' to be null, got: %v", fieldPath, fieldValue) - return result - } - if !*assertion.ToBeNull && isNull { + for _, fv := range values { + result.Actual = fv.value + if failed, err := ae.checkSingle(assertion, fieldPath, fv.value, fv.exists); failed { result.Status = domain.StatusFailed - result.Error = fmt.Sprintf("expected '%s' to not be null", fieldPath) + result.Error = err return result } } + } - // If field doesn't exist and we have value operators, fail - if !exists && assertion.HasValueOperators() { - result.Status = domain.StatusFailed - result.Error = fmt.Sprintf("'%s' does not exist (cannot evaluate assertion)", fieldPath) - return result + return result +} + +// checkSingle evaluates all operators against a single (value, exists) pair. +// Returns (true, errorMessage) if the assertion fails, (false, "") if it passes. +func (ae *AssertionEngine) checkSingle(assertion *domain.Assertion, fieldPath string, value interface{}, exists bool) (bool, string) { + if assertion.ToExist != nil { + if *assertion.ToExist && !exists { + return true, fmt.Sprintf("expected '%s' to exist", fieldPath) + } + if !*assertion.ToExist && exists { + return true, fmt.Sprintf("expected '%s' to not exist, got: %v", fieldPath, value) } + } - if err := ae.evaluateOperators(assertion, fieldPath, fieldValue); err != nil { - result.Status = domain.StatusFailed - result.Error = err.Error() - return result + if assertion.ToBeNull != nil { + isNull := exists && value == nil + if *assertion.ToBeNull && !isNull { + if !exists { + return true, fmt.Sprintf("expected '%s' to be null, but field does not exist", fieldPath) + } + return true, fmt.Sprintf("expected '%s' to be null, got: %v", fieldPath, value) + } + if !*assertion.ToBeNull && isNull { + return true, fmt.Sprintf("expected '%s' to not be null", fieldPath) } } - return result + if !exists && assertion.HasValueOperators() { + return true, fmt.Sprintf("'%s' does not exist (cannot evaluate assertion)", fieldPath) + } + + if err := ae.evaluateOperators(assertion, fieldPath, value); err != nil { + return true, err.Error() + } + return false, "" } // evaluateOperators checks all operators specified on an assertion @@ -375,37 +389,210 @@ func (ae *AssertionEngine) applySelector(selector string, manifests []interface{ return results, nil } -// extractFieldValue extracts a value from a resource using a field path -func (ae *AssertionEngine) extractFieldValue(fieldPath string, resource interface{}) (interface{}, bool, error) { +// fieldValue is one result of extracting a path from a resource. +// For wildcard paths (e.g. containers[*].image) extractFieldValues returns +// one entry per matching element; for simple paths it returns exactly one +// entry when the path produces a result, or nothing when it doesn't. +type fieldValue struct { + value interface{} + exists bool +} + +// extractFieldValues pulls all values at the given path from a resource. +// It distinguishes "missing key" from "present but null" by probing the +// parent object with has() in a separate gojq query. +func (ae *AssertionEngine) extractFieldValues(fieldPath string, resource interface{}) ([]fieldValue, error) { jqPath := ae.fieldPathToJQ(fieldPath) query, err := gojq.Parse(jqPath) if err != nil { - return nil, false, fmt.Errorf("invalid field path: %w", err) + return nil, fmt.Errorf("invalid field path: %w", err) } + var raw []interface{} iter := query.Run(resource) - v, ok := iter.Next() - if !ok { - return nil, false, nil + for { + v, ok := iter.Next() + if !ok { + break + } + if e, isErr := v.(error); isErr { + // gojq path errors (e.g. indexing a number) mean the path cannot + // be reached — treat as producing no results, let existence handle it. + _ = e + continue + } + raw = append(raw, v) } - if _, ok := v.(error); ok { - return nil, false, nil + // Wildcard path: each produced element is considered to exist. + if isWildcardPath(jqPath) { + results := make([]fieldValue, 0, len(raw)) + for _, v := range raw { + results = append(results, fieldValue{value: v, exists: true}) + } + return results, nil } - return v, v != nil, nil + // Simple path: there should be at most one result. Probe parent for + // real existence so present-null is distinguished from missing. + if len(raw) == 0 { + return nil, nil + } + exists, err := ae.probeExistence(jqPath, resource) + if err != nil { + return nil, err + } + return []fieldValue{{value: raw[0], exists: exists}}, nil } -// fieldPathToJQ converts a field path to JQ syntax -func (ae *AssertionEngine) fieldPathToJQ(fieldPath string) string { - if strings.HasPrefix(fieldPath, ".") { - return fieldPath +// probeExistence runs a companion query against the parent of the path +// that returns true iff the final key is actually present on the parent. +// Falls back to (value != nil) if the path can't be split. +func (ae *AssertionEngine) probeExistence(jqPath string, resource interface{}) (bool, error) { + parent, probe, ok := splitLastSegment(jqPath) + if !ok { + // Fallback: evaluate the original path and treat any non-nil as existing. + q, err := gojq.Parse(jqPath) + if err != nil { + return false, nil + } + iter := q.Run(resource) + if v, ok := iter.Next(); ok { + if _, isErr := v.(error); isErr { + return false, nil + } + return v != nil, nil + } + return false, nil + } + + probeExpr := parent + " | " + probe + q, err := gojq.Parse(probeExpr) + if err != nil { + return false, nil + } + iter := q.Run(resource) + v, ok := iter.Next() + if !ok { + return false, nil } + if _, isErr := v.(error); isErr { + return false, nil + } + b, _ := v.(bool) + return b, nil +} + +// fieldPathToJQ converts a field path to JQ syntax. Normalizes [*] (a +// yamlspec-user convenience) to [] (jq's array iteration) regardless of +// whether the path already has a leading dot. +func (ae *AssertionEngine) fieldPathToJQ(fieldPath string) string { path := strings.ReplaceAll(fieldPath, "[*]", "[]") + if strings.HasPrefix(path, ".") { + return path + } return "." + path } +// isWildcardPath returns true if a jq path contains an unconstrained array +// iteration that can produce multiple results. +func isWildcardPath(jqPath string) bool { + inQuote := byte(0) + for i := 0; i < len(jqPath); i++ { + c := jqPath[i] + if inQuote != 0 { + if c == inQuote { + inQuote = 0 + } + continue + } + if c == '"' || c == '\'' { + inQuote = c + continue + } + if c == '[' && i+1 < len(jqPath) && jqPath[i+1] == ']' { + return true + } + } + return false +} + +// splitLastSegment splits a simple jq path into (parentExpr, probeExpr). +// probeExpr is a jq fragment that, when piped to parentExpr, returns true +// iff the final segment is present on that parent. +// Returns ok=false when the path is a wildcard path, the root (.), or +// can't be parsed — callers must fall back to value-based existence. +func splitLastSegment(jqPath string) (string, string, bool) { + if jqPath == "" || jqPath == "." { + return "", "", false + } + if isWildcardPath(jqPath) { + return "", "", false + } + + p := jqPath + if p[0] == '.' { + p = p[1:] + } + if p == "" { + return "", "", false + } + + // Walk to find the last top-level boundary ('.' or '['). + inQuote := byte(0) + lastBoundary := -1 + for i := 0; i < len(p); i++ { + c := p[i] + if inQuote != 0 { + if c == inQuote { + inQuote = 0 + } + continue + } + if c == '"' || c == '\'' { + inQuote = c + continue + } + if c == '.' || c == '[' { + lastBoundary = i + } + } + + var parent, last string + if lastBoundary == -1 { + parent = "." + last = "." + p + } else { + if lastBoundary == 0 { + parent = "." + } else { + parent = "." + p[:lastBoundary] + } + last = p[lastBoundary:] + } + + if strings.HasPrefix(last, ".") { + key := strings.TrimPrefix(last, ".") + if key == "" { + return "", "", false + } + return parent, fmt.Sprintf(`(type == "object" and has(%q))`, key), true + } + + if strings.HasPrefix(last, "[") && strings.HasSuffix(last, "]") { + inner := last[1 : len(last)-1] + if len(inner) >= 2 && (inner[0] == '"' || inner[0] == '\'') && inner[0] == inner[len(inner)-1] { + key := inner[1 : len(inner)-1] + return parent, fmt.Sprintf(`(type == "object" and has(%q))`, key), true + } + if idx, err := strconv.Atoi(inner); err == nil { + return parent, fmt.Sprintf(`(type == "array" and length > %d)`, idx), true + } + } + return "", "", false +} + // valuesEqual compares two values for equality with type coercion func (ae *AssertionEngine) valuesEqual(a, b interface{}) bool { if reflect.DeepEqual(a, b) { @@ -441,22 +628,23 @@ func (ae *AssertionEngine) toFloat64(v interface{}) (float64, error) { } } -// ParseManifests parses multi-document YAML into a slice of interfaces +// ParseManifests parses multi-document YAML into a slice of interfaces. +// Uses a real YAML decoder rather than splitting on "---" so that "---" +// appearing inside literal blocks, comments, or strings is not mistaken +// for a document separator. func ParseManifests(yamlContent string) ([]interface{}, error) { - var manifests []interface{} - - documents := strings.Split(yamlContent, "---") - for _, doc := range documents { - doc = strings.TrimSpace(doc) - if doc == "" || doc == "null" { - continue - } + decoder := yaml.NewDecoder(strings.NewReader(yamlContent)) + var manifests []interface{} + for { var manifest interface{} - if err := yaml.Unmarshal([]byte(doc), &manifest); err != nil { + err := decoder.Decode(&manifest) + if err == io.EOF { + break + } + if err != nil { return nil, fmt.Errorf("failed to parse YAML: %w", err) } - if manifest != nil { manifests = append(manifests, manifest) } diff --git a/services/assertion_test.go b/services/assertion_test.go index ab8d19c..a267f5a 100644 --- a/services/assertion_test.go +++ b/services/assertion_test.go @@ -486,11 +486,11 @@ func TestEvaluate_MultipleOperators(t *testing.T) { t.Run("string with multiple checks", func(t *testing.T) { r := evalSingle(ae, manifests, "Deployment", domain.Assertion{ - Should: "match constraints", - Expect: "spec.template.spec.containers[0].image", - ToStartWith: "nginx:", + Should: "match constraints", + Expect: "spec.template.spec.containers[0].image", + ToStartWith: "nginx:", ToNotEndWith: ":latest", - ToContain: "1.25", + ToContain: "1.25", }) assertStatus(t, r.Status, domain.StatusPassed) }) @@ -526,6 +526,122 @@ func TestParseManifests(t *testing.T) { t.Fatalf("expected 1 manifest, got %d", len(m)) } }) + + t.Run("literal block containing --- is not split", func(t *testing.T) { + // A ConfigMap value that literally contains "---" must stay inside + // the single document, not create phantom ones. + m, err := ParseManifests(`kind: ConfigMap +data: + script: | + #!/bin/sh + cat < 0 { + var cancel context.CancelFunc + ctx, cancel = context.WithTimeout(ctx, timeout) + defer cancel() + } cmd := exec.CommandContext(ctx, "sh", "-c", command) cmd.Dir = dir cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr + // Put the shell in its own process group so we can SIGKILL the whole + // tree on cancel — otherwise grandchildren like `sleep` orphan and may + // hold inherited fds, making cmd.Wait() block past the deadline. + setProcessGroup(cmd) + cmd.Cancel = func() error { + return killProcessGroup(cmd) + } + // After the cancel function fires, don't wait more than 2s for cleanup + // before force-closing our end and returning from Wait. + cmd.WaitDelay = 2 * time.Second + if err := cmd.Run(); err != nil { + if ctx.Err() == context.DeadlineExceeded { + return fmt.Errorf("command '%s' timed out after %s", command, timeout) + } return fmt.Errorf("command '%s' failed: %w", command, err) } return nil diff --git a/services/runner_unix.go b/services/runner_unix.go new file mode 100644 index 0000000..b3394e1 --- /dev/null +++ b/services/runner_unix.go @@ -0,0 +1,25 @@ +//go:build !windows + +package services + +import ( + "os/exec" + "syscall" +) + +func setProcessGroup(cmd *exec.Cmd) { + cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} +} + +func killProcessGroup(cmd *exec.Cmd) error { + if cmd.Process == nil { + return nil + } + // Negative PID targets the process group, killing the shell and any + // descendants it spawned (e.g. a sleep inside `sh -c "sleep 5"`). + if err := syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL); err != nil { + // Fall back to killing just the shell if the group is already gone. + return cmd.Process.Kill() + } + return nil +} diff --git a/services/runner_windows.go b/services/runner_windows.go new file mode 100644 index 0000000..28459ab --- /dev/null +++ b/services/runner_windows.go @@ -0,0 +1,17 @@ +//go:build windows + +package services + +import "os/exec" + +func setProcessGroup(cmd *exec.Cmd) { + // No process-group concept on Windows — exec.CommandContext's default + // Kill behavior is used by leaving cmd.SysProcAttr unchanged. +} + +func killProcessGroup(cmd *exec.Cmd) error { + if cmd.Process == nil { + return nil + } + return cmd.Process.Kill() +}