From 9f84b23bfff5d645502508805c2047e0190bb435 Mon Sep 17 00:00:00 2001 From: "Ronny.Herrgesell" Date: Sun, 24 May 2026 17:34:20 +0200 Subject: [PATCH 1/2] feat: strict directive validation at parse time Validate `@timeout`, `@poll`, and `@env` directives during parsing, rejecting malformed values with line-numbered error messages before execution begins. - Add DirectiveError type with structured line/directive/message fields - Validate @timeout (integer >= 0), @poll (integer > 0), @env (KEY=VALUE) - Refactor Timeout/Poll to *int to distinguish unset from explicit zero - Support file-level @timeout in frontmatter (inherited by entries) - ParseFile returns []error for multi-error reporting - Add E2E tests for directive validation - Update SPEC.md with validation rules and file-level @timeout docs Closes #43 --- ROADMAP.md | 2 +- SPEC.md | 37 +++++++++- cmd/clitest/run.go | 31 +++++++- internal/executor/background.go | 16 +++-- internal/executor/background_test.go | 26 +++---- internal/executor/executor.go | 10 ++- internal/executor/executor_test.go | 2 +- internal/parser/directives.go | 70 +++++++++++++++---- internal/parser/parser.go | 37 +++++----- internal/parser/parser_test.go | 67 +++++++++--------- internal/types/types.go | 5 +- test/_fixtures/syntax/bad_env.clitest | 3 + test/_fixtures/syntax/bad_poll.clitest | 3 + test/_fixtures/syntax/bad_timeout.clitest | 3 + .../syntax/file_timeout_inherit.clitest | 6 ++ .../syntax/it_validates_directives.clitest | 24 +++++++ 16 files changed, 247 insertions(+), 95 deletions(-) create mode 100644 test/_fixtures/syntax/bad_env.clitest create mode 100644 test/_fixtures/syntax/bad_poll.clitest create mode 100644 test/_fixtures/syntax/bad_timeout.clitest create mode 100644 test/_fixtures/syntax/file_timeout_inherit.clitest create mode 100644 test/e2e/syntax/it_validates_directives.clitest diff --git a/ROADMAP.md b/ROADMAP.md index 261eba4..1dae137 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -12,6 +12,7 @@ - [x] `[Finally]` section + `later` assert modifier — Send a signal to background processes at file-end and assert exit code + output. `later` keyword defers assert evaluation to file-end. Execution order: entries → later asserts → [Finally] LIFO → @defer LIFO. See #19. - [x] `[Prompts]` — Interactive prompt/response section: match stdout patterns and send responses via stdin. Pipe-based (no PTY). Supports substring and regex matching, multiplier syntax, ambiguity detection, and unmatched prompt failure. Default timeout 30s. - [x] `--no-color` — Disable ANSI color codes in output. Also respects `NO_COLOR` env var (https://no-color.org/) and auto-disables when stdout is not a TTY. +- [x] Strict directive validation — reject malformed `@timeout`, `@poll`, `@env` at parse time with line-numbered errors; file-level `@timeout` support [#43](https://github.com/sleipi/cli-t/issues/43) - [x] `--fail-fast` — Stop execution on the first test failure instead of running all entries - [x] Refactor `cmd/clitest/` package structure — Extracted display, resolve, filter, vars, and executor logic into dedicated `internal/` packages. Reduced `cmd/clitest/` from 17 to 7 files. - [x] Linting — Introduced `golangci-lint` with CI integration and resolved all issues @@ -48,7 +49,6 @@ - [ ] Register Domain - [ ] Publish VS Code extension to Marketplace (requires publisher account) - [ ] Syntax overhaul before 1.0 release [#40](https://github.com/sleipi/cli-t/issues/40) -- [ ] Strict directive validation — reject malformed directives at parse time [#43](https://github.com/sleipi/cli-t/issues/43) ## Won't Do diff --git a/SPEC.md b/SPEC.md index 1174168..d861379 100644 --- a/SPEC.md +++ b/SPEC.md @@ -499,7 +499,7 @@ EXIT 0 |-----------|-------|--------|-------------| | `@group` | file, entry | `@group tag1 tag2 ...` | Space-separated tags for filtering | | `@skip` | file, entry | `@skip [reason]` | Skip this entry/file (reason optional) | -| `@timeout` | entry | `@timeout MS` | Max time to wait (required for `EXIT NEVER`) | +| `@timeout` | file, entry | `@timeout MS` | Max time to wait; file-level sets default for all entries | | `@poll` | entry | `@poll MS` | Polling interval for `EXIT NEVER` asserts (default: 100ms) | | `@defer` | entry | `@defer` | Marks entry as cleanup — runs at file end (LIFO), always, not a test | | `@workdir` | file, entry | `@workdir ` | Run command in specified directory | @@ -604,7 +604,7 @@ Sets a real environment variable on the child process. Supported at both file-le **Scoping:** Entry-level env vars are isolated to that entry only — they do NOT leak to subsequent entries. File-level env vars apply to all entries in the file, including `@defer` entries. -**Values:** The value is everything after the first `=`. Values may contain `=`, spaces, and special characters. Empty values (`@env KEY=`) are valid. Malformed directives without `=` are silently ignored. +**Values:** The value is everything after the first `=`. Values may contain `=`, spaces, and special characters. Empty values (`@env KEY=`) are valid. Malformed directives without `=` are rejected at parse time. **Variable substitution:** `--var` placeholders (`{{name}}`) in values are expanded (since substitution runs before parsing). @@ -628,6 +628,39 @@ EXIT 0 stdout == "false" ``` +#### Directive Validation + +All directives are validated at parse time. If any directive has an invalid value, clitest reports the error(s) and does not execute the file. Multiple errors may be reported at once. + +**Error format:** `:line : @: ` + +| Directive | Valid values | Notes | +|-----------|-------------|-------| +| `@timeout` | Integer >= 0 (milliseconds) | `0` means no timeout (infinite wait). No duration suffixes. | +| `@poll` | Integer > 0 (milliseconds) | Must be positive. No duration suffixes. | +| `@env` | `KEY=VALUE` with non-empty key | Empty value after `=` is valid. Missing `=` is rejected. | + +Unknown directives are silently ignored. + +#### File-level `@timeout` + +`@timeout` may appear in frontmatter to set a default timeout for all entries in the file. Entry-level `@timeout` overrides the file-level value. + +``` +--- +@timeout 5000 +--- + +# This entry inherits @timeout 5000 +echo "quick" +EXIT 0 + +# This entry overrides to 10s +@timeout 10000 +echo "slow" +EXIT 0 +``` + ### Planned Directives (roadmap) | Directive | Description | diff --git a/cmd/clitest/run.go b/cmd/clitest/run.go index 9fabc2f..df63bd3 100644 --- a/cmd/clitest/run.go +++ b/cmd/clitest/run.go @@ -1,6 +1,7 @@ package main import ( + "errors" "fmt" "os" "path/filepath" @@ -219,15 +220,27 @@ func loadAndParse(path string, v map[string]string) (*types.File, error) { return nil, fmt.Errorf("reading %s: %w", path, err) } input := vars.Substitute(string(content), v) - f, err := parser.ParseFile(input) - if err != nil { - return nil, fmt.Errorf("parsing %s: %w", path, err) + f, errs := parser.ParseFile(input) + if len(errs) > 0 { + // Check if these are DirectiveErrors (validation) or structural parse errors + var de *parser.DirectiveError + if errors.As(errs[0], &de) { + // Validation errors: format with file prefix + var msg string + for _, e := range errs { + msg += fmt.Sprintf("%s:%s\n", path, e.Error()) + } + return nil, fmt.Errorf("%s", msg[:len(msg)-1]) + } + // Structural parse errors: use legacy format + return nil, fmt.Errorf("parsing %s: %w", path, errs[0]) } f.Path = path if err := resolveWorkdirs(f); err != nil { return nil, fmt.Errorf("resolving workdir in %s: %w", path, err) } resolveEnvs(f) + resolveTimeouts(f) return f, nil } @@ -295,3 +308,15 @@ func resolveEnvs(f *types.File) { } } } + +// resolveTimeouts inherits file-level @timeout into entries that don't define their own. +func resolveTimeouts(f *types.File) { + if f.Directives.Timeout == nil { + return + } + for i := range f.Entries { + if f.Entries[i].Directives.Timeout == nil { + f.Entries[i].Directives.Timeout = f.Directives.Timeout + } + } +} diff --git a/internal/executor/background.go b/internal/executor/background.go index 764cb96..0b7fbcb 100644 --- a/internal/executor/background.go +++ b/internal/executor/background.go @@ -50,13 +50,17 @@ func backgroundEntry(entry types.Entry, cmd string, captures map[string]string) keepAlive := hasLaterAsserts(entry) || entry.Finally != nil - timeout := entry.Directives.Timeout - if timeout <= 0 { - timeout = DefaultBackgroundTimeout + timeout := DefaultBackgroundTimeout + if entry.Directives.Timeout != nil { + if *entry.Directives.Timeout == 0 { + timeout = 0 // explicitly no timeout + } else { + timeout = *entry.Directives.Timeout + } } - poll := entry.Directives.Poll - if poll <= 0 { - poll = DefaultPollInterval + poll := DefaultPollInterval + if entry.Directives.Poll != nil { + poll = *entry.Directives.Poll } deadline := time.Now().Add(time.Duration(timeout) * time.Millisecond) diff --git a/internal/executor/background_test.go b/internal/executor/background_test.go index 718fc58..1254db4 100644 --- a/internal/executor/background_test.go +++ b/internal/executor/background_test.go @@ -6,14 +6,16 @@ import ( "github.com/sleipi/cli-t/internal/types" ) +func intPtr(v int) *int { return &v } + func TestBackgroundEntry_PassingAsserts(t *testing.T) { entry := types.Entry{ Command: `sh -c 'echo ready; sleep 10'`, ExitNever: true, Asserts: []types.Assert{{Query: "stdout", Predicate: "contains", Value: "ready"}}, Directives: types.EntryDirectives{ - Timeout: 3000, - Poll: 50, + Timeout: intPtr(3000), + Poll: intPtr(50), }, } captures := map[string]string{} @@ -37,8 +39,8 @@ func TestBackgroundEntry_KeepAliveWithLater(t *testing.T) { {Query: "stdout", Predicate: "contains", Value: "later_output", Later: true}, }, Directives: types.EntryDirectives{ - Timeout: 3000, - Poll: 50, + Timeout: intPtr(3000), + Poll: intPtr(50), }, } captures := map[string]string{} @@ -59,8 +61,8 @@ func TestBackgroundEntry_KeepAliveWithFinally(t *testing.T) { Asserts: []types.Assert{{Query: "stdout", Predicate: "contains", Value: "ready"}}, Finally: &types.Finally{Signal: "KILL", ExitCode: 137, Timeout: 1000}, Directives: types.EntryDirectives{ - Timeout: 3000, - Poll: 50, + Timeout: intPtr(3000), + Poll: intPtr(50), }, } captures := map[string]string{} @@ -80,8 +82,8 @@ func TestBackgroundEntry_Timeout(t *testing.T) { ExitNever: true, Asserts: []types.Assert{{Query: "stdout", Predicate: "contains", Value: "never_appears"}}, Directives: types.EntryDirectives{ - Timeout: 200, - Poll: 50, + Timeout: intPtr(200), + Poll: intPtr(50), }, } captures := map[string]string{} @@ -110,8 +112,8 @@ func TestBackgroundEntry_UnexpectedExit(t *testing.T) { ExitNever: true, Asserts: []types.Assert{{Query: "stdout", Predicate: "contains", Value: "ready"}}, Directives: types.EntryDirectives{ - Timeout: 3000, - Poll: 50, + Timeout: intPtr(3000), + Poll: intPtr(50), }, } captures := map[string]string{} @@ -140,8 +142,8 @@ func TestBackgroundEntry_CapturesStored(t *testing.T) { Asserts: []types.Assert{{Query: "stdout", Predicate: "contains", Value: "ready"}}, Captures: []types.Capture{{Name: "bgpid", Query: "pid"}}, Directives: types.EntryDirectives{ - Timeout: 3000, - Poll: 50, + Timeout: intPtr(3000), + Poll: intPtr(50), }, } captures := map[string]string{} diff --git a/internal/executor/executor.go b/internal/executor/executor.go index a81d499..a34202d 100644 --- a/internal/executor/executor.go +++ b/internal/executor/executor.go @@ -30,9 +30,13 @@ func Entry(entry types.Entry, captures map[string]string) Result { // promptEntry runs a command with interactive prompts. func promptEntry(entry types.Entry, cmd string, captures map[string]string) Result { - timeout := entry.Directives.Timeout - if timeout <= 0 { - timeout = 30000 // default 30s + timeout := 30000 // default 30s + if entry.Directives.Timeout != nil { + if *entry.Directives.Timeout == 0 { + timeout = 0 // explicitly no timeout + } else { + timeout = *entry.Directives.Timeout + } } prompts := make([]runner.PromptDef, len(entry.Prompts)) diff --git a/internal/executor/executor_test.go b/internal/executor/executor_test.go index e39cdee..9b19e88 100644 --- a/internal/executor/executor_test.go +++ b/internal/executor/executor_test.go @@ -154,7 +154,7 @@ func TestEntry_PromptResponds(t *testing.T) { Prompts: []types.Prompt{ {Pattern: "Enter name:", IsRegex: false, Response: "Alice", Repeat: 1}, }, - Directives: types.EntryDirectives{Timeout: 3000}, + Directives: types.EntryDirectives{Timeout: func() *int { v := 3000; return &v }()}, } captures := map[string]string{} er := Entry(entry, captures) diff --git a/internal/parser/directives.go b/internal/parser/directives.go index 44a9751..df8c263 100644 --- a/internal/parser/directives.go +++ b/internal/parser/directives.go @@ -12,10 +12,23 @@ import ( type directive struct { Name string Value string + Line int // 1-based line number in the file +} + +// DirectiveError represents a validation error for a malformed directive. +type DirectiveError struct { + Line int + Directive string + Message string +} + +func (e *DirectiveError) Error() string { + return fmt.Sprintf("line %d: @%s: %s", e.Line, e.Directive, e.Message) } // parseDirective parses a line like "@group BUG-1234 smoke" into a directive. -func parseDirective(line string) (*directive, error) { +// lineNum is the 1-based line number in the file. +func parseDirective(line string, lineNum int) (*directive, error) { if !strings.HasPrefix(line, "@") { return nil, fmt.Errorf("not a directive: %s", line) } @@ -32,17 +45,18 @@ func parseDirective(line string) (*directive, error) { value = strings.TrimSpace(parts[1]) } - return &directive{Name: name, Value: value}, nil + return &directive{Name: name, Value: value, Line: lineNum}, nil } // parseEntryDirective parses and appends a directive to an entry builder. -func parseEntryDirective(current *entryBuilder, line string) error { +// lineNum is the 1-based line number in the file. +func parseEntryDirective(current *entryBuilder, line string, lineNum int) error { if current.command != "" { return fmt.Errorf("directive must appear before command: %s", line) } - d, err := parseDirective(strings.TrimSpace(line)) + d, err := parseDirective(strings.TrimSpace(line), lineNum) if err != nil { - return fmt.Errorf("line: %w", err) + return fmt.Errorf("line %d: %w", lineNum, err) } if d != nil { current.directives = append(current.directives, *d) @@ -57,11 +71,14 @@ func parseFrontmatter(lines []string, file *types.File) (int, error) { for i < len(lines) { line := strings.TrimSpace(lines[i]) if line == "---" { - interpretFileDirectives(file, fileDirectives) + errs := interpretFileDirectives(file, fileDirectives) + if len(errs) > 0 { + return 0, errs[0] // TODO: return all errors once ParseFile supports []error + } return i + 1, nil } if strings.HasPrefix(line, "@") { - d, err := parseDirective(line) + d, err := parseDirective(line, i+1) if err != nil { return 0, fmt.Errorf("frontmatter line %d: %w", i+1, err) } @@ -75,7 +92,9 @@ func parseFrontmatter(lines []string, file *types.File) (int, error) { } // interpretFileDirectives interprets raw directives into typed FileDirectives. -func interpretFileDirectives(f *types.File, directives []directive) { +// Returns validation errors for malformed directive values. +func interpretFileDirectives(f *types.File, directives []directive) []error { + var errs []error for _, d := range directives { switch d.Name { case "group": @@ -87,9 +106,18 @@ func interpretFileDirectives(f *types.File, directives []directive) { f.Directives.SkipReason = d.Value case "workdir": f.Directives.Workdir = d.Value + case "timeout": + v, err := strconv.Atoi(d.Value) + if err != nil || v < 0 { + errs = append(errs, &DirectiveError{Line: d.Line, Directive: d.Name, Message: fmt.Sprintf("value must be a non-negative integer (milliseconds), got %q", d.Value)}) + } else { + f.Directives.Timeout = &v + } case "env": parts := strings.SplitN(d.Value, "=", 2) - if len(parts) == 2 && parts[0] != "" { + if len(parts) < 2 || parts[0] == "" { + errs = append(errs, &DirectiveError{Line: d.Line, Directive: d.Name, Message: fmt.Sprintf("value must be KEY=VALUE with non-empty key, got %q", d.Value)}) + } else { if f.Directives.Env == nil { f.Directives.Env = make(map[string]string) } @@ -97,10 +125,13 @@ func interpretFileDirectives(f *types.File, directives []directive) { } } } + return errs } // interpretEntryDirectives interprets raw directives into typed EntryDirectives. -func interpretEntryDirectives(e *types.Entry, directives []directive) { +// Returns validation errors for malformed directive values. +func interpretEntryDirectives(e *types.Entry, directives []directive) []error { + var errs []error for _, d := range directives { switch d.Name { case "group": @@ -113,18 +144,26 @@ func interpretEntryDirectives(e *types.Entry, directives []directive) { case "defer": e.Directives.Defer = true case "timeout": - if v, err := strconv.Atoi(d.Value); err == nil { - e.Directives.Timeout = v + v, err := strconv.Atoi(d.Value) + if err != nil || v < 0 { + errs = append(errs, &DirectiveError{Line: d.Line, Directive: d.Name, Message: fmt.Sprintf("value must be a non-negative integer (milliseconds), got %q", d.Value)}) + } else { + e.Directives.Timeout = &v } case "poll": - if v, err := strconv.Atoi(d.Value); err == nil { - e.Directives.Poll = v + v, err := strconv.Atoi(d.Value) + if err != nil || v <= 0 { + errs = append(errs, &DirectiveError{Line: d.Line, Directive: d.Name, Message: fmt.Sprintf("value must be a positive integer (milliseconds), got %q", d.Value)}) + } else { + e.Directives.Poll = &v } case "workdir": e.Directives.Workdir = d.Value case "env": parts := strings.SplitN(d.Value, "=", 2) - if len(parts) == 2 && parts[0] != "" { + if len(parts) < 2 || parts[0] == "" { + errs = append(errs, &DirectiveError{Line: d.Line, Directive: d.Name, Message: fmt.Sprintf("value must be KEY=VALUE with non-empty key, got %q", d.Value)}) + } else { if e.Directives.Env == nil { e.Directives.Env = make(map[string]string) } @@ -132,4 +171,5 @@ func interpretEntryDirectives(e *types.Entry, directives []directive) { } } } + return errs } diff --git a/internal/parser/parser.go b/internal/parser/parser.go index 0d779b0..e44e228 100644 --- a/internal/parser/parser.go +++ b/internal/parser/parser.go @@ -9,7 +9,7 @@ import ( ) // ParseFile parses a .clitest file content into a File with frontmatter and entries. -func ParseFile(input string) (*types.File, error) { +func ParseFile(input string) (*types.File, []error) { lines := strings.Split(input, "\n") // Remove trailing empty line from split if len(lines) > 0 && lines[len(lines)-1] == "" { @@ -24,14 +24,14 @@ func ParseFile(input string) (*types.File, error) { var err error startLine, err = parseFrontmatter(lines, file) if err != nil { - return nil, err + return nil, []error{err} } } // Parse entries - entries, err := parseEntries(lines[startLine:]) - if err != nil { - return nil, err + entries, errs := parseEntries(lines[startLine:], startLine) + if len(errs) > 0 { + return nil, errs } file.Entries = entries return file, nil @@ -39,20 +39,23 @@ func ParseFile(input string) (*types.File, error) { // Parse parses a .clitest file content into a list of entries (legacy API). func Parse(input string) ([]types.Entry, error) { - f, err := ParseFile(input) - if err != nil { - return nil, err + f, errs := ParseFile(input) + if len(errs) > 0 { + return nil, errs[0] } return f.Entries, nil } -func parseEntries(lines []string) ([]types.Entry, error) { +func parseEntries(lines []string, lineOffset int) ([]types.Entry, []error) { var entries []types.Entry + var allErrors []error var current *entryBuilder flush := func() { if current != nil { - entries = append(entries, current.build()) + entry, errs := current.build() + entries = append(entries, entry) + allErrors = append(allErrors, errs...) current = nil } } @@ -80,8 +83,8 @@ func parseEntries(lines []string) ([]types.Entry, error) { if current == nil { current = &entryBuilder{} } - if err := parseEntryDirective(current, line); err != nil { - return nil, err + if err := parseEntryDirective(current, line, lineOffset+i+1); err != nil { + return nil, []error{err} } i++ continue @@ -102,12 +105,12 @@ func parseEntries(lines []string) ([]types.Entry, error) { var err error i, err = parsePostCommand(lines, i, current) if err != nil { - return nil, err + return nil, []error{err} } } flush() - return entries, nil + return entries, allErrors } // parsePostCommand handles lines after the command: EXIT, [Asserts], [Captures], fenced/implicit body. @@ -208,7 +211,7 @@ type entryBuilder struct { directives []directive } -func (b *entryBuilder) build() types.Entry { +func (b *entryBuilder) build() (types.Entry, []error) { entry := types.Entry{ Comment: b.comment, Command: b.command, @@ -220,6 +223,6 @@ func (b *entryBuilder) build() types.Entry { Prompts: b.prompts, Finally: b.finally, } - interpretEntryDirectives(&entry, b.directives) - return entry + errs := interpretEntryDirectives(&entry, b.directives) + return entry, errs } diff --git a/internal/parser/parser_test.go b/internal/parser/parser_test.go index 8b563d0..a29569c 100644 --- a/internal/parser/parser_test.go +++ b/internal/parser/parser_test.go @@ -1,6 +1,7 @@ package parser import ( + "strings" "testing" "github.com/sleipi/cli-t/internal/types" @@ -243,9 +244,9 @@ func TestParseFrontmatter(t *testing.T) { echo "hello" EXIT 0 ` - f, err := ParseFile(input) - if err != nil { - t.Fatalf("unexpected error: %v", err) + f, errs := ParseFile(input) + if len(errs) > 0 { + t.Fatalf("unexpected error: %v", errs) } if len(f.Directives.Groups) != 2 { t.Fatalf("expected 2 file groups, got %d: %v", len(f.Directives.Groups), f.Directives.Groups) @@ -266,8 +267,8 @@ func TestParseFrontmatterUnclosed(t *testing.T) { @group test echo "hello" ` - _, err := ParseFile(input) - if err == nil { + _, errs := ParseFile(input) + if len(errs) == 0 { t.Fatal("expected error for unclosed frontmatter") } } @@ -330,9 +331,9 @@ Add your tests below echo "hello" ` - f, err := ParseFile(input) - if err != nil { - t.Fatalf("unexpected error: %v", err) + f, errs := ParseFile(input) + if len(errs) > 0 { + t.Fatalf("unexpected error: %v", errs) } if len(f.Directives.Groups) != 2 { t.Fatalf("expected 2 groups, got %d", len(f.Directives.Groups)) @@ -372,11 +373,11 @@ stdout contains "ready" if !e.ExitNever { t.Fatal("expected ExitNever to be true") } - if e.Directives.Timeout != 5000 { - t.Fatalf("expected Timeout 5000, got %d", e.Directives.Timeout) + if e.Directives.Timeout == nil || *e.Directives.Timeout != 5000 { + t.Fatalf("expected Timeout 5000, got %v", e.Directives.Timeout) } - if e.Directives.Poll != 200 { - t.Fatalf("expected Poll 200, got %d", e.Directives.Poll) + if e.Directives.Poll == nil || *e.Directives.Poll != 200 { + t.Fatalf("expected Poll 200, got %v", e.Directives.Poll) } if len(e.Captures) != 1 || e.Captures[0].Query != "pid" { t.Fatalf("expected pid capture, got %+v", e.Captures) @@ -633,9 +634,9 @@ func TestParseEnvDirective_EntryLevel(t *testing.T) { echo test EXIT 0 ` - f, err := ParseFile(input) - if err != nil { - t.Fatalf("unexpected error: %v", err) + f, errs := ParseFile(input) + if len(errs) > 0 { + t.Fatalf("unexpected error: %v", errs) } if len(f.Entries) != 1 { t.Fatalf("expected 1 entry, got %d", len(f.Entries)) @@ -653,9 +654,9 @@ func TestParseEnvDirective_ValueContainsEquals(t *testing.T) { echo test EXIT 0 ` - f, err := ParseFile(input) - if err != nil { - t.Fatalf("unexpected error: %v", err) + f, errs := ParseFile(input) + if len(errs) > 0 { + t.Fatalf("unexpected error: %v", errs) } assertEqual(t, f.Entries[0].Directives.Env["CONN"], "host=localhost;port=5432") } @@ -665,9 +666,9 @@ func TestParseEnvDirective_EmptyValue(t *testing.T) { echo test EXIT 0 ` - f, err := ParseFile(input) - if err != nil { - t.Fatalf("unexpected error: %v", err) + f, errs := ParseFile(input) + if len(errs) > 0 { + t.Fatalf("unexpected error: %v", errs) } env := f.Entries[0].Directives.Env val, exists := env["KEY"] @@ -679,17 +680,17 @@ EXIT 0 } } -func TestParseEnvDirective_NoEquals_Ignored(t *testing.T) { +func TestParseEnvDirective_NoEquals_Error(t *testing.T) { input := `@env NOVALUE echo test EXIT 0 ` - f, err := ParseFile(input) - if err != nil { - t.Fatalf("unexpected error: %v", err) + _, errs := ParseFile(input) + if len(errs) == 0 { + t.Fatal("expected error for @env without =, got none") } - if f.Entries[0].Directives.Env != nil { - t.Errorf("expected nil env map, got %v", f.Entries[0].Directives.Env) + if !strings.Contains(errs[0].Error(), "@env") { + t.Fatalf("expected @env error, got: %v", errs[0]) } } @@ -702,9 +703,9 @@ func TestParseEnvDirective_FileLevel(t *testing.T) { echo test EXIT 0 ` - f, err := ParseFile(input) - if err != nil { - t.Fatalf("unexpected error: %v", err) + f, errs := ParseFile(input) + if len(errs) > 0 { + t.Fatalf("unexpected error: %v", errs) } env := f.Directives.Env if len(env) != 2 { @@ -720,9 +721,9 @@ func TestParseEnvDirective_DuplicateKey_LastWins(t *testing.T) { echo test EXIT 0 ` - f, err := ParseFile(input) - if err != nil { - t.Fatalf("unexpected error: %v", err) + f, errs := ParseFile(input) + if len(errs) > 0 { + t.Fatalf("unexpected error: %v", errs) } assertEqual(t, f.Entries[0].Directives.Env["X"], "second") } diff --git a/internal/types/types.go b/internal/types/types.go index a1e7d25..03830db 100644 --- a/internal/types/types.go +++ b/internal/types/types.go @@ -6,8 +6,8 @@ type EntryDirectives struct { Skip bool SkipReason string Defer bool - Timeout int // @timeout in ms (0 = not set) - Poll int // @poll in ms (0 = default 100ms) + Timeout *int // @timeout in ms (nil = not set, 0 = no timeout) + Poll *int // @poll in ms (nil = not set, use default) Workdir string // @workdir path (empty = not set) Env map[string]string // @env KEY=VALUE (nil = not set) } @@ -17,6 +17,7 @@ type FileDirectives struct { Groups []string Skip bool SkipReason string + Timeout *int // @timeout in ms (nil = not set, 0 = no timeout) Workdir string // @workdir path (empty = not set) Env map[string]string // @env KEY=VALUE (nil = not set) } diff --git a/test/_fixtures/syntax/bad_env.clitest b/test/_fixtures/syntax/bad_env.clitest new file mode 100644 index 0000000..4832b50 --- /dev/null +++ b/test/_fixtures/syntax/bad_env.clitest @@ -0,0 +1,3 @@ +@env NOEQUALS +echo hello +EXIT 0 diff --git a/test/_fixtures/syntax/bad_poll.clitest b/test/_fixtures/syntax/bad_poll.clitest new file mode 100644 index 0000000..0b0022b --- /dev/null +++ b/test/_fixtures/syntax/bad_poll.clitest @@ -0,0 +1,3 @@ +@poll -5 +echo hello +EXIT NEVER diff --git a/test/_fixtures/syntax/bad_timeout.clitest b/test/_fixtures/syntax/bad_timeout.clitest new file mode 100644 index 0000000..f29b94b --- /dev/null +++ b/test/_fixtures/syntax/bad_timeout.clitest @@ -0,0 +1,3 @@ +@timeout abc +echo hello +EXIT 0 diff --git a/test/_fixtures/syntax/file_timeout_inherit.clitest b/test/_fixtures/syntax/file_timeout_inherit.clitest new file mode 100644 index 0000000..6fcbcbc --- /dev/null +++ b/test/_fixtures/syntax/file_timeout_inherit.clitest @@ -0,0 +1,6 @@ +--- +@timeout 5000 +--- + +echo hello +EXIT 0 diff --git a/test/e2e/syntax/it_validates_directives.clitest b/test/e2e/syntax/it_validates_directives.clitest new file mode 100644 index 0000000..47b9fa3 --- /dev/null +++ b/test/e2e/syntax/it_validates_directives.clitest @@ -0,0 +1,24 @@ +# Validates that malformed @timeout is rejected at parse time +./clitest test/_fixtures/syntax/bad_timeout.clitest +EXIT 1 +[Asserts] +stdout contains "@timeout" +stdout contains "non-negative integer" + +# Validates that malformed @poll is rejected at parse time +./clitest test/_fixtures/syntax/bad_poll.clitest +EXIT 1 +[Asserts] +stdout contains "@poll" +stdout contains "positive integer" + +# Validates that malformed @env is rejected at parse time +./clitest test/_fixtures/syntax/bad_env.clitest +EXIT 1 +[Asserts] +stdout contains "@env" +stdout contains "KEY=VALUE" + +# Validates that file-level @timeout is inherited by entries +./clitest test/_fixtures/syntax/file_timeout_inherit.clitest +EXIT 0 From 26b6b3a3f624792a457ca396a789e4dc6d6b6592 Mon Sep 17 00:00:00 2001 From: "Ronny.Herrgesell" Date: Sun, 24 May 2026 17:39:58 +0200 Subject: [PATCH 2/2] refactor: resolve stale TODO, return all frontmatter errors - parseFrontmatter now returns []error (was single error) - Replace manual newline trimming with strings.Join in loadAndParse --- cmd/clitest/run.go | 9 +++++---- internal/parser/directives.go | 11 ++++------- internal/parser/parser.go | 8 ++++---- 3 files changed, 13 insertions(+), 15 deletions(-) diff --git a/cmd/clitest/run.go b/cmd/clitest/run.go index df63bd3..86bab79 100644 --- a/cmd/clitest/run.go +++ b/cmd/clitest/run.go @@ -5,6 +5,7 @@ import ( "fmt" "os" "path/filepath" + "strings" "sync/atomic" "github.com/sleipi/cli-t/internal/display" @@ -226,11 +227,11 @@ func loadAndParse(path string, v map[string]string) (*types.File, error) { var de *parser.DirectiveError if errors.As(errs[0], &de) { // Validation errors: format with file prefix - var msg string - for _, e := range errs { - msg += fmt.Sprintf("%s:%s\n", path, e.Error()) + msgs := make([]string, len(errs)) + for i, e := range errs { + msgs[i] = fmt.Sprintf("%s:%s", path, e.Error()) } - return nil, fmt.Errorf("%s", msg[:len(msg)-1]) + return nil, fmt.Errorf("%s", strings.Join(msgs, "\n")) } // Structural parse errors: use legacy format return nil, fmt.Errorf("parsing %s: %w", path, errs[0]) diff --git a/internal/parser/directives.go b/internal/parser/directives.go index df8c263..171d27e 100644 --- a/internal/parser/directives.go +++ b/internal/parser/directives.go @@ -65,22 +65,19 @@ func parseEntryDirective(current *entryBuilder, line string, lineNum int) error } // parseFrontmatter parses the frontmatter block between --- delimiters. -func parseFrontmatter(lines []string, file *types.File) (int, error) { +func parseFrontmatter(lines []string, file *types.File) (int, []error) { var fileDirectives []directive i := 1 for i < len(lines) { line := strings.TrimSpace(lines[i]) if line == "---" { errs := interpretFileDirectives(file, fileDirectives) - if len(errs) > 0 { - return 0, errs[0] // TODO: return all errors once ParseFile supports []error - } - return i + 1, nil + return i + 1, errs } if strings.HasPrefix(line, "@") { d, err := parseDirective(line, i+1) if err != nil { - return 0, fmt.Errorf("frontmatter line %d: %w", i+1, err) + return 0, []error{fmt.Errorf("frontmatter line %d: %w", i+1, err)} } if d != nil { fileDirectives = append(fileDirectives, *d) @@ -88,7 +85,7 @@ func parseFrontmatter(lines []string, file *types.File) (int, error) { } i++ } - return 0, fmt.Errorf("unclosed frontmatter (missing closing ---)") + return 0, []error{fmt.Errorf("unclosed frontmatter (missing closing ---)")} } // interpretFileDirectives interprets raw directives into typed FileDirectives. diff --git a/internal/parser/parser.go b/internal/parser/parser.go index e44e228..16d2da1 100644 --- a/internal/parser/parser.go +++ b/internal/parser/parser.go @@ -21,10 +21,10 @@ func ParseFile(input string) (*types.File, []error) { // Parse frontmatter if present if len(lines) > 0 && strings.TrimSpace(lines[0]) == "---" { - var err error - startLine, err = parseFrontmatter(lines, file) - if err != nil { - return nil, []error{err} + var fmErrs []error + startLine, fmErrs = parseFrontmatter(lines, file) + if len(fmErrs) > 0 { + return nil, fmErrs } }