From 3e1752cd5e0e598579369bc8464e45cca5f431b5 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Fri, 27 Feb 2026 08:56:56 -0500 Subject: [PATCH 1/4] feat: template hyphen auto-fix, config imports, and pipeline template linting Three features to make workflow configs safer and more maintainable: Template engine: - preprocessTemplate() auto-rewrites hyphenated dot-access to index syntax before Go's parser sees it (e.g., .steps.my-step.field just works) - step/trigger helper functions for clean syntax without quoting issues (e.g., {{ step "parse-request" "path_params" "id" }}) - 16 new tests covering preprocessor, helpers, and end-to-end Config imports: - imports: field in WorkflowConfig for splitting large configs into domain-specific files with recursive loading and circular detection - Main file definitions take precedence; modules concatenated in order - 8 new tests covering basic imports, precedence, nesting, and errors Pipeline template validation (wfctl template validate --config): - Lints template expressions for undefined step references - Warns on forward references (step referenced before it executes) - Suggests step function syntax for hyphenated dot-access patterns - 5 new tests for the linter Documentation updated with template data context, hyphen handling guide, import syntax and merge rules, and ApplicationConfig comparison. Co-Authored-By: Claude Opus 4.6 --- DOCUMENTATION.md | 114 +++++- cmd/wfctl/template_validate.go | 151 +++++++- cmd/wfctl/template_validate_test.go | 202 ++++++++++ config/config.go | 103 ++++- config/config_import_test.go | 565 ++++++++++++++++++++++++++++ module/pipeline_template.go | 167 +++++++- module/pipeline_template_test.go | 187 +++++++++ 7 files changed, 1469 insertions(+), 20 deletions(-) create mode 100644 config/config_import_test.go diff --git a/DOCUMENTATION.md b/DOCUMENTATION.md index edacaf73..71d70aa7 100644 --- a/DOCUMENTATION.md +++ b/DOCUMENTATION.md @@ -124,12 +124,46 @@ Pipeline steps support Go template syntax with these built-in functions: | Function | Description | Example | |----------|-------------|---------| | `uuidv4` | Generates a UUID v4 | `{{ uuidv4 }}` | -| `now` | Current time in RFC3339 format | `{{ now }}` | +| `now` | Current time (RFC3339 default, or named/custom layout) | `{{ now }}`, `{{ now "DateOnly" }}`, `{{ now "2006-01-02" }}` | | `lower` | Lowercase string | `{{ lower .name }}` | | `default` | Default value when empty | `{{ default "pending" .status }}` | | `json` | Marshal value to JSON string | `{{ json .data }}` | +| `trimPrefix` | Remove prefix from string | `{{ .phone \| trimPrefix "+" }}` | +| `trimSuffix` | Remove suffix from string | `{{ .file \| trimSuffix ".txt" }}` | +| `step` | Access step outputs by name and keys | `{{ step "parse-request" "path_params" "id" }}` | +| `trigger` | Access trigger data by keys | `{{ trigger "path_params" "id" }}` | -Template expressions can reference previous step outputs via `{{ .steps.step-name.field }}` or for hyphenated names `{{index .steps "step-name" "field"}}`. +#### Template Data Context + +Templates have access to four top-level namespaces: + +| Variable | Source | Description | +|----------|--------|-------------| +| `{{ .field }}` | `pc.Current` | Merged trigger data + all prior step outputs (flat) | +| `{{ .steps.NAME.field }}` | `pc.StepOutputs` | Namespaced access to a specific step's output | +| `{{ .trigger.field }}` | `pc.TriggerData` | Original trigger data (immutable) | +| `{{ .meta.field }}` | `pc.Metadata` | Execution metadata (pipeline name, etc.) | + +#### Hyphenated Step Names + +Step names commonly contain hyphens (e.g., `parse-request`, `fetch-orders`). Go's template parser treats `-` as subtraction, so `{{ .steps.my-step.field }}` would normally fail. The engine handles this automatically: + +**Auto-fix (just works):** Write natural dot notation — the engine rewrites it before parsing: +```yaml +value: "{{ .steps.parse-request.path_params.id }}" +``` + +**Preferred syntax:** The `step` function avoids quoting issues entirely: +```yaml +value: '{{ step "parse-request" "path_params" "id" }}' +``` + +**Manual alternative:** The `index` function also works: +```yaml +value: '{{ index .steps "parse-request" "path_params" "id" }}' +``` + +`wfctl template validate --config workflow.yaml` lints template expressions and warns on undefined step references, forward references, and suggests the `step` function for hyphenated names. ### Infrastructure | Type | Description | @@ -816,6 +850,82 @@ triggers: action: "create-order" ``` +### Config Imports + +Large configs can be split into domain-specific files using `imports`. Each imported file is a standard workflow config — no special format required. Imports are recursive (imported files can import other files) with circular import detection. + +```yaml +# main.yaml +imports: + - config/modules.yaml + - config/routes/agents.yaml + - config/routes/tasks.yaml + - config/routes/requests.yaml + +modules: + - name: extra-module + type: http.server + config: + address: ":9090" + +pipelines: + health-check: + steps: + - name: respond + type: step.json_response + config: + body: '{"status": "ok"}' +``` + +```yaml +# config/modules.yaml +modules: + - name: my-db + type: storage.sqlite + config: + dbPath: ./data/app.db + + - name: my-router + type: http.router +``` + +```yaml +# config/routes/agents.yaml +pipelines: + list-agents: + steps: + - name: query + type: step.db_query + config: + query: "SELECT * FROM agents" + - name: respond + type: step.json_response + +triggers: + list-agents: + type: http + config: + path: /api/agents + method: GET +``` + +**Merge rules:** +- **Modules:** All modules from all files are included. Main file's modules appear first. +- **Pipelines, triggers, workflows, platform:** Main file's definitions take precedence. Imported values only fill in keys not already defined. +- **Recursive imports:** Imported files can themselves use `imports`. Circular imports are detected and produce an error. +- **Relative paths:** Import paths are resolved relative to the importing file's directory. + +**Comparison with ApplicationConfig:** + +| Feature | `imports` | `ApplicationConfig` | +|---------|-----------|---------------------| +| Format | Standard `WorkflowConfig` with `imports:` field | Separate `application:` top-level format | +| Conflict handling | Main file wins silently | Errors on name conflicts | +| Use case | Splitting a monolith incrementally | Composing independent workflow services | +| Nesting | Files can import other files recursively | Flat list of workflow references | + +Both approaches work with `wfctl template validate --config` for validation. + ## Visual Workflow Builder (UI) A React-based visual editor for composing workflow configurations (`ui/` directory). diff --git a/cmd/wfctl/template_validate.go b/cmd/wfctl/template_validate.go index 41b79718..c76f6af9 100644 --- a/cmd/wfctl/template_validate.go +++ b/cmd/wfctl/template_validate.go @@ -16,17 +16,17 @@ import ( // templateValidationResult holds the outcome of validating a single template. type templateValidationResult struct { - Name string - ModuleCount int - ModuleValid int - StepCount int - StepValid int - DepCount int - DepValid int - TriggerCount int - TriggerValid int - Warnings []string - Errors []string + Name string + ModuleCount int + ModuleValid int + StepCount int + StepValid int + DepCount int + DepValid int + TriggerCount int + TriggerValid int + Warnings []string + Errors []string } // pass returns true if there are no errors. @@ -416,6 +416,9 @@ func validateWorkflowConfig(name string, cfg *config.WorkflowConfig, knownModule } } + // 6. Validate template expressions in pipeline steps + validatePipelineTemplates(pipelineName, stepsRaw, &result) + // 4. Validate trigger types if triggerRaw, ok := pipelineMap["trigger"]; ok { if triggerMap, ok := triggerRaw.(map[string]any); ok { @@ -530,3 +533,129 @@ func printTemplateValidationResults(results []templateValidationResult, summary // templateFSReader allows reading from the embedded templateFS for validation. // It wraps around the existing templateFS embed.FS. var _ fs.FS = templateFS + +// --- Pipeline template expression linting --- + +// templateExprRe matches template actions {{ ... }}. +var templateExprRe = regexp.MustCompile(`\{\{(.*?)\}\}`) + +// stepRefDotRe matches .steps.STEP_NAME patterns (dot access). +var stepRefDotRe = regexp.MustCompile(`\.steps\.([a-zA-Z_][a-zA-Z0-9_-]*)`) + +// stepRefIndexRe matches index .steps "STEP_NAME" patterns. +var stepRefIndexRe = regexp.MustCompile(`index\s+\.steps\s+"([^"]+)"`) + +// stepRefFuncRe matches step "STEP_NAME" function calls. +var stepRefFuncRe = regexp.MustCompile(`\bstep\s+"([^"]+)"`) + +// hyphenDotRe matches dot-access chains with hyphens (e.g., .steps.my-step.field). +var hyphenDotRe = regexp.MustCompile(`\.[a-zA-Z_][a-zA-Z0-9_]*-[a-zA-Z0-9_-]*`) + +// validatePipelineTemplates checks template expressions in pipeline step configs for +// references to nonexistent or forward-declared steps and common template pitfalls. +func validatePipelineTemplates(pipelineName string, stepsRaw []any, result *templateValidationResult) { + // Build ordered step name list + stepNames := make(map[string]int) // step name -> index in pipeline + for i, stepRaw := range stepsRaw { + stepMap, ok := stepRaw.(map[string]any) + if !ok { + continue + } + name, _ := stepMap["name"].(string) + if name != "" { + stepNames[name] = i + } + } + + // Check each step's config for template expressions + for i, stepRaw := range stepsRaw { + stepMap, ok := stepRaw.(map[string]any) + if !ok { + continue + } + stepName, _ := stepMap["name"].(string) + if stepName == "" { + stepName = fmt.Sprintf("step[%d]", i) + } + + // Collect all string values from the step config recursively + templates := collectTemplateStrings(stepMap) + + for _, tmpl := range templates { + // Find all template actions + actions := templateExprRe.FindAllStringSubmatch(tmpl, -1) + for _, action := range actions { + if len(action) < 2 { + continue + } + actionContent := action[1] + + // Skip comments + trimmed := strings.TrimSpace(actionContent) + if strings.HasPrefix(trimmed, "/*") { + continue + } + + // Check for step name references via dot-access + dotMatches := stepRefDotRe.FindAllStringSubmatch(actionContent, -1) + for _, m := range dotMatches { + refName := m[1] + validateStepRef(pipelineName, stepName, refName, i, stepNames, result) + } + + // Check for step name references via index + indexMatches := stepRefIndexRe.FindAllStringSubmatch(actionContent, -1) + for _, m := range indexMatches { + refName := m[1] + validateStepRef(pipelineName, stepName, refName, i, stepNames, result) + } + + // Check for step name references via step function + funcMatches := stepRefFuncRe.FindAllStringSubmatch(actionContent, -1) + for _, m := range funcMatches { + refName := m[1] + validateStepRef(pipelineName, stepName, refName, i, stepNames, result) + } + + // Warn on hyphenated dot-access (auto-fixed but suggest preferred syntax) + if hyphenDotRe.MatchString(actionContent) { + result.Warnings = append(result.Warnings, + fmt.Sprintf("pipeline %q step %q: template uses hyphenated dot-access which is auto-fixed; prefer step \"name\" \"field\" syntax", pipelineName, stepName)) + } + } + } + } +} + +// validateStepRef checks that a referenced step name exists and appears before the +// current step in the pipeline execution order. +func validateStepRef(pipelineName, currentStep, refName string, currentIdx int, stepNames map[string]int, result *templateValidationResult) { + refIdx, exists := stepNames[refName] + if !exists { + result.Warnings = append(result.Warnings, + fmt.Sprintf("pipeline %q step %q: references step %q which does not exist in this pipeline", pipelineName, currentStep, refName)) + } else if refIdx >= currentIdx { + result.Warnings = append(result.Warnings, + fmt.Sprintf("pipeline %q step %q: references step %q which has not executed yet (appears later in pipeline)", pipelineName, currentStep, refName)) + } +} + +// collectTemplateStrings recursively finds all strings containing {{ in a value tree. +func collectTemplateStrings(data any) []string { + var results []string + switch v := data.(type) { + case string: + if strings.Contains(v, "{{") { + results = append(results, v) + } + case map[string]any: + for _, val := range v { + results = append(results, collectTemplateStrings(val)...) + } + case []any: + for _, item := range v { + results = append(results, collectTemplateStrings(item)...) + } + } + return results +} diff --git a/cmd/wfctl/template_validate_test.go b/cmd/wfctl/template_validate_test.go index 59bfd0f5..904d23f1 100644 --- a/cmd/wfctl/template_validate_test.go +++ b/cmd/wfctl/template_validate_test.go @@ -235,3 +235,205 @@ func TestRunTemplateUnknownSubcommand(t *testing.T) { t.Fatal("expected error for unknown subcommand") } } + +// --- Pipeline template expression linting tests --- + +func TestValidateConfigWithValidStepRefs(t *testing.T) { + dir := t.TempDir() + configContent := ` +pipelines: + api: + trigger: + type: http + config: + path: /items/:id + method: GET + steps: + - name: parse-request + type: step.set + config: + values: + item_id: "static-id" + - name: db-query + type: step.set + config: + values: + query: "{{ .steps.parse-request.path_params.id }}" +` + configPath := filepath.Join(dir, "workflow.yaml") + if err := os.WriteFile(configPath, []byte(configContent), 0644); err != nil { + t.Fatalf("failed to write test config: %v", err) + } + + err := runTemplateValidate([]string{"-config", configPath}) + if err != nil { + t.Fatalf("expected valid step refs to pass, got: %v", err) + } +} + +func TestValidateConfigWithMissingStepRef(t *testing.T) { + cfg := &config.WorkflowConfig{ + Pipelines: map[string]any{ + "api": map[string]any{ + "steps": []any{ + map[string]any{ + "name": "do-thing", + "type": "step.set", + "config": map[string]any{ + "values": map[string]any{ + "x": "{{ .steps.nonexistent.field }}", + }, + }, + }, + }, + }, + }, + } + knownModules := KnownModuleTypes() + knownSteps := KnownStepTypes() + knownTriggers := KnownTriggerTypes() + + result := validateWorkflowConfig("test", cfg, knownModules, knownSteps, knownTriggers) + + found := false + for _, w := range result.Warnings { + if strings.Contains(w, "nonexistent") && strings.Contains(w, "does not exist") { + found = true + break + } + } + if !found { + t.Errorf("expected warning about nonexistent step reference, got warnings: %v", result.Warnings) + } +} + +func TestValidateConfigWithForwardStepRef(t *testing.T) { + cfg := &config.WorkflowConfig{ + Pipelines: map[string]any{ + "api": map[string]any{ + "steps": []any{ + map[string]any{ + "name": "first", + "type": "step.set", + "config": map[string]any{ + "values": map[string]any{ + "x": "{{ .steps.second.output }}", + }, + }, + }, + map[string]any{ + "name": "second", + "type": "step.set", + "config": map[string]any{ + "values": map[string]any{ + "y": "hello", + }, + }, + }, + }, + }, + }, + } + knownModules := KnownModuleTypes() + knownSteps := KnownStepTypes() + knownTriggers := KnownTriggerTypes() + + result := validateWorkflowConfig("test", cfg, knownModules, knownSteps, knownTriggers) + + found := false + for _, w := range result.Warnings { + if strings.Contains(w, "second") && strings.Contains(w, "has not executed yet") { + found = true + break + } + } + if !found { + t.Errorf("expected warning about forward step reference, got warnings: %v", result.Warnings) + } +} + +func TestValidateConfigWithStepFunction(t *testing.T) { + cfg := &config.WorkflowConfig{ + Pipelines: map[string]any{ + "api": map[string]any{ + "steps": []any{ + map[string]any{ + "name": "parse-request", + "type": "step.set", + "config": map[string]any{ + "values": map[string]any{ + "x": "raw-body", + }, + }, + }, + map[string]any{ + "name": "process", + "type": "step.set", + "config": map[string]any{ + "values": map[string]any{ + "name": `{{ step "parse-request" "body" "name" }}`, + }, + }, + }, + }, + }, + }, + } + knownModules := KnownModuleTypes() + knownSteps := KnownStepTypes() + knownTriggers := KnownTriggerTypes() + + result := validateWorkflowConfig("test", cfg, knownModules, knownSteps, knownTriggers) + + // parse-request exists and is before process, so no warning about missing/forward ref + for _, w := range result.Warnings { + if strings.Contains(w, "parse-request") && (strings.Contains(w, "does not exist") || strings.Contains(w, "has not executed yet")) { + t.Errorf("unexpected warning about parse-request: %s", w) + } + } +} + +func TestValidateConfigWithHyphenDotAccess(t *testing.T) { + cfg := &config.WorkflowConfig{ + Pipelines: map[string]any{ + "api": map[string]any{ + "steps": []any{ + map[string]any{ + "name": "my-step", + "type": "step.set", + "config": map[string]any{ + "values": map[string]any{ + "x": "hello", + }, + }, + }, + map[string]any{ + "name": "consumer", + "type": "step.set", + "config": map[string]any{ + "values": map[string]any{ + "y": "{{ .steps.my-step.field }}", + }, + }, + }, + }, + }, + }, + } + knownModules := KnownModuleTypes() + knownSteps := KnownStepTypes() + knownTriggers := KnownTriggerTypes() + + result := validateWorkflowConfig("test", cfg, knownModules, knownSteps, knownTriggers) + + found := false + for _, w := range result.Warnings { + if strings.Contains(w, "hyphenated dot-access") && strings.Contains(w, "prefer") { + found = true + break + } + } + if !found { + t.Errorf("expected informational warning about hyphenated dot-access, got warnings: %v", result.Warnings) + } +} diff --git a/config/config.go b/config/config.go index 7a261306..b298d965 100644 --- a/config/config.go +++ b/config/config.go @@ -92,6 +92,7 @@ type PluginRequirement struct { // WorkflowConfig represents the overall configuration for the workflow engine type WorkflowConfig struct { + Imports []string `json:"imports,omitempty" yaml:"imports,omitempty"` Modules []ModuleConfig `json:"modules" yaml:"modules"` Workflows map[string]any `json:"workflows" yaml:"workflows"` Triggers map[string]any `json:"triggers" yaml:"triggers"` @@ -113,9 +114,31 @@ func (c *WorkflowConfig) ResolveRelativePath(path string) string { return pathpkg.Join(c.ConfigDir, path) } -// LoadFromFile loads a workflow configuration from a YAML file +// LoadFromFile loads a workflow configuration from a YAML file. +// If the config contains an "imports" field, referenced files are loaded +// recursively and merged. The importing file's definitions take precedence +// over imported ones for map-based fields (workflows, triggers, pipelines, +// platform). Modules are concatenated with the main file's modules first. func LoadFromFile(filepath string) (*WorkflowConfig, error) { - data, err := os.ReadFile(filepath) + return loadFromFileWithImports(filepath, nil) +} + +func loadFromFileWithImports(filepath string, seen map[string]bool) (*WorkflowConfig, error) { + absPath, err := pathpkg.Abs(filepath) + if err != nil { + return nil, fmt.Errorf("failed to resolve path %s: %w", filepath, err) + } + + // Circular import detection + if seen == nil { + seen = make(map[string]bool) + } + if seen[absPath] { + return nil, fmt.Errorf("circular import detected: %s", filepath) + } + seen[absPath] = true + + data, err := os.ReadFile(absPath) if err != nil { return nil, fmt.Errorf("failed to read config file: %w", err) } @@ -125,16 +148,84 @@ func LoadFromFile(filepath string) (*WorkflowConfig, error) { return nil, fmt.Errorf("failed to parse config file: %w", err) } - // Store the config file's directory for relative path resolution - absPath, err := pathpkg.Abs(filepath) - if err == nil { - cfg.ConfigDir = pathpkg.Dir(absPath) + cfg.ConfigDir = pathpkg.Dir(absPath) + + // Process imports + if len(cfg.Imports) > 0 { + if err := cfg.processImports(seen); err != nil { + return nil, err + } } return &cfg, nil } +// processImports loads all imported config files and merges them into this config. +// Imported definitions provide defaults — the importing file's own definitions take +// precedence for map-based fields (workflows, triggers, pipelines, platform). +// Modules are appended after the main file's modules. +func (cfg *WorkflowConfig) processImports(seen map[string]bool) error { + for _, imp := range cfg.Imports { + impPath := imp + if !pathpkg.IsAbs(impPath) { + impPath = pathpkg.Join(cfg.ConfigDir, impPath) + } + + impCfg, err := loadFromFileWithImports(impPath, seen) + if err != nil { + return fmt.Errorf("import %q: %w", imp, err) + } + + // Merge imported modules (append after main file's modules) + cfg.Modules = append(cfg.Modules, impCfg.Modules...) + + // Merge maps — imported values only added if not already defined in main file + if cfg.Workflows == nil { + cfg.Workflows = make(map[string]any) + } + for k, v := range impCfg.Workflows { + if _, exists := cfg.Workflows[k]; !exists { + cfg.Workflows[k] = v + } + } + + if cfg.Triggers == nil { + cfg.Triggers = make(map[string]any) + } + for k, v := range impCfg.Triggers { + if _, exists := cfg.Triggers[k]; !exists { + cfg.Triggers[k] = v + } + } + + if cfg.Pipelines == nil { + cfg.Pipelines = make(map[string]any) + } + for k, v := range impCfg.Pipelines { + if _, exists := cfg.Pipelines[k]; !exists { + cfg.Pipelines[k] = v + } + } + + if impCfg.Platform != nil { + if cfg.Platform == nil { + cfg.Platform = make(map[string]any) + } + for k, v := range impCfg.Platform { + if _, exists := cfg.Platform[k]; !exists { + cfg.Platform[k] = v + } + } + } + } + + cfg.Imports = nil // clear after processing + return nil +} + // LoadFromString loads a workflow configuration from a YAML string. +// Note: imports are NOT processed when loading from a string because there is +// no file path context to resolve relative import paths against. func LoadFromString(yamlContent string) (*WorkflowConfig, error) { var cfg WorkflowConfig if err := yaml.Unmarshal([]byte(yamlContent), &cfg); err != nil { diff --git a/config/config_import_test.go b/config/config_import_test.go new file mode 100644 index 00000000..d45bdf83 --- /dev/null +++ b/config/config_import_test.go @@ -0,0 +1,565 @@ +package config + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +func TestLoadFromFile_WithImports(t *testing.T) { + dir := t.TempDir() + + // Create imported file with a module + modulesYAML := ` +modules: + - name: my-db + type: storage.sqlite + config: + path: ./data/app.db +` + if err := os.WriteFile(filepath.Join(dir, "modules.yaml"), []byte(modulesYAML), 0644); err != nil { + t.Fatal(err) + } + + // Create imported routes file with a pipeline + routesYAML := ` +pipelines: + get-items: + steps: + - name: query + type: step.db_query + config: + query: "SELECT * FROM items" + +triggers: + get-items-trigger: + type: http + config: + path: /items + method: GET +` + if err := os.WriteFile(filepath.Join(dir, "routes.yaml"), []byte(routesYAML), 0644); err != nil { + t.Fatal(err) + } + + // Create main file that imports both + mainYAML := ` +imports: + - modules.yaml + - routes.yaml + +modules: + - name: my-server + type: http.server + config: + address: ":8080" + +workflows: + main: + steps: + - name: init + type: step.log +` + mainPath := filepath.Join(dir, "main.yaml") + if err := os.WriteFile(mainPath, []byte(mainYAML), 0644); err != nil { + t.Fatal(err) + } + + cfg, err := LoadFromFile(mainPath) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + // Should have 2 modules (my-server from main + my-db from import) + if len(cfg.Modules) != 2 { + t.Errorf("expected 2 modules, got %d", len(cfg.Modules)) + } + + // Main file's module should come first + if cfg.Modules[0].Name != "my-server" { + t.Errorf("expected first module to be 'my-server', got %q", cfg.Modules[0].Name) + } + if cfg.Modules[1].Name != "my-db" { + t.Errorf("expected second module to be 'my-db', got %q", cfg.Modules[1].Name) + } + + // Should have the pipeline from import + if _, ok := cfg.Pipelines["get-items"]; !ok { + t.Error("expected 'get-items' pipeline from import") + } + + // Should have the trigger from import + if _, ok := cfg.Triggers["get-items-trigger"]; !ok { + t.Error("expected 'get-items-trigger' trigger from import") + } + + // Should have the workflow from main + if _, ok := cfg.Workflows["main"]; !ok { + t.Error("expected 'main' workflow from main file") + } + + // Imports field should be cleared after processing + if len(cfg.Imports) != 0 { + t.Errorf("expected imports to be cleared, got %v", cfg.Imports) + } +} + +func TestLoadFromFile_ImportPrecedence(t *testing.T) { + dir := t.TempDir() + + // Create imported file with a pipeline + importedYAML := ` +pipelines: + my-pipeline: + steps: + - name: imported-step + type: step.log + config: + message: "from import" + +triggers: + my-trigger: + type: http + config: + path: /imported +` + if err := os.WriteFile(filepath.Join(dir, "imported.yaml"), []byte(importedYAML), 0644); err != nil { + t.Fatal(err) + } + + // Create main file with same pipeline name — main should win + mainYAML := ` +imports: + - imported.yaml + +pipelines: + my-pipeline: + steps: + - name: main-step + type: step.log + config: + message: "from main" + +triggers: + my-trigger: + type: http + config: + path: /main +` + mainPath := filepath.Join(dir, "main.yaml") + if err := os.WriteFile(mainPath, []byte(mainYAML), 0644); err != nil { + t.Fatal(err) + } + + cfg, err := LoadFromFile(mainPath) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + // Main file's pipeline definition should win + pipeline, ok := cfg.Pipelines["my-pipeline"] + if !ok { + t.Fatal("expected 'my-pipeline' pipeline") + } + pMap, ok := pipeline.(map[string]any) + if !ok { + t.Fatal("expected pipeline to be a map") + } + steps, ok := pMap["steps"].([]any) + if !ok || len(steps) == 0 { + t.Fatal("expected pipeline to have steps") + } + step0, ok := steps[0].(map[string]any) + if !ok { + t.Fatal("expected step to be a map") + } + if step0["name"] != "main-step" { + t.Errorf("expected main file's step 'main-step', got %q", step0["name"]) + } + + // Main file's trigger definition should win + trigger, ok := cfg.Triggers["my-trigger"] + if !ok { + t.Fatal("expected 'my-trigger' trigger") + } + tMap, ok := trigger.(map[string]any) + if !ok { + t.Fatal("expected trigger to be a map") + } + tConfig, ok := tMap["config"].(map[string]any) + if !ok { + t.Fatal("expected trigger config to be a map") + } + if tConfig["path"] != "/main" { + t.Errorf("expected main file's trigger path '/main', got %q", tConfig["path"]) + } +} + +func TestLoadFromFile_CircularImport(t *testing.T) { + dir := t.TempDir() + + // a.yaml imports b.yaml + aYAML := ` +imports: + - b.yaml +modules: + - name: mod-a + type: test.a +` + if err := os.WriteFile(filepath.Join(dir, "a.yaml"), []byte(aYAML), 0644); err != nil { + t.Fatal(err) + } + + // b.yaml imports a.yaml — circular! + bYAML := ` +imports: + - a.yaml +modules: + - name: mod-b + type: test.b +` + if err := os.WriteFile(filepath.Join(dir, "b.yaml"), []byte(bYAML), 0644); err != nil { + t.Fatal(err) + } + + _, err := LoadFromFile(filepath.Join(dir, "a.yaml")) + if err == nil { + t.Fatal("expected circular import error, got nil") + } + if !strings.Contains(err.Error(), "circular import") { + t.Errorf("expected error to contain 'circular import', got: %v", err) + } +} + +func TestLoadFromFile_NestedImports(t *testing.T) { + dir := t.TempDir() + + // c.yaml — leaf, no imports + cYAML := ` +modules: + - name: mod-c + type: test.c + +pipelines: + pipeline-c: + steps: + - name: step-c + type: step.log +` + if err := os.WriteFile(filepath.Join(dir, "c.yaml"), []byte(cYAML), 0644); err != nil { + t.Fatal(err) + } + + // b.yaml imports c.yaml + bYAML := ` +imports: + - c.yaml + +modules: + - name: mod-b + type: test.b + +pipelines: + pipeline-b: + steps: + - name: step-b + type: step.log +` + if err := os.WriteFile(filepath.Join(dir, "b.yaml"), []byte(bYAML), 0644); err != nil { + t.Fatal(err) + } + + // a.yaml imports b.yaml + aYAML := ` +imports: + - b.yaml + +modules: + - name: mod-a + type: test.a + +pipelines: + pipeline-a: + steps: + - name: step-a + type: step.log +` + if err := os.WriteFile(filepath.Join(dir, "a.yaml"), []byte(aYAML), 0644); err != nil { + t.Fatal(err) + } + + cfg, err := LoadFromFile(filepath.Join(dir, "a.yaml")) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + // Should have 3 modules: mod-a (main), mod-b (from b.yaml), mod-c (from c.yaml via b.yaml) + if len(cfg.Modules) != 3 { + t.Errorf("expected 3 modules, got %d", len(cfg.Modules)) + } + + // Main file's module should be first + if cfg.Modules[0].Name != "mod-a" { + t.Errorf("expected first module 'mod-a', got %q", cfg.Modules[0].Name) + } + + // Should have all three pipelines + for _, name := range []string{"pipeline-a", "pipeline-b", "pipeline-c"} { + if _, ok := cfg.Pipelines[name]; !ok { + t.Errorf("expected pipeline %q", name) + } + } +} + +func TestLoadFromFile_ImportNotFound(t *testing.T) { + dir := t.TempDir() + + mainYAML := ` +imports: + - nonexistent.yaml + +modules: + - name: mod-main + type: test.main +` + mainPath := filepath.Join(dir, "main.yaml") + if err := os.WriteFile(mainPath, []byte(mainYAML), 0644); err != nil { + t.Fatal(err) + } + + _, err := LoadFromFile(mainPath) + if err == nil { + t.Fatal("expected error for missing import, got nil") + } + if !strings.Contains(err.Error(), "nonexistent.yaml") { + t.Errorf("expected error to reference 'nonexistent.yaml', got: %v", err) + } +} + +func TestLoadFromFile_NoImports(t *testing.T) { + dir := t.TempDir() + + // Standard config without imports — backward compatibility + mainYAML := ` +modules: + - name: my-server + type: http.server + config: + address: ":8080" + +workflows: + main: + steps: + - name: init + type: step.log + +triggers: + main-trigger: + type: http + config: + path: / + method: GET + +pipelines: + main-pipeline: + steps: + - name: step1 + type: step.log +` + mainPath := filepath.Join(dir, "main.yaml") + if err := os.WriteFile(mainPath, []byte(mainYAML), 0644); err != nil { + t.Fatal(err) + } + + cfg, err := LoadFromFile(mainPath) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if len(cfg.Modules) != 1 { + t.Errorf("expected 1 module, got %d", len(cfg.Modules)) + } + if cfg.Modules[0].Name != "my-server" { + t.Errorf("expected module 'my-server', got %q", cfg.Modules[0].Name) + } + if _, ok := cfg.Workflows["main"]; !ok { + t.Error("expected 'main' workflow") + } + if _, ok := cfg.Triggers["main-trigger"]; !ok { + t.Error("expected 'main-trigger' trigger") + } + if _, ok := cfg.Pipelines["main-pipeline"]; !ok { + t.Error("expected 'main-pipeline' pipeline") + } +} + +func TestLoadFromFile_ImportRelativePath(t *testing.T) { + dir := t.TempDir() + + // Create a subdirectory for imported files + subDir := filepath.Join(dir, "includes") + if err := os.MkdirAll(subDir, 0755); err != nil { + t.Fatal(err) + } + + // Create imported file in subdirectory + importedYAML := ` +modules: + - name: imported-mod + type: test.imported + +pipelines: + imported-pipeline: + steps: + - name: step1 + type: step.log +` + if err := os.WriteFile(filepath.Join(subDir, "shared.yaml"), []byte(importedYAML), 0644); err != nil { + t.Fatal(err) + } + + // Main file uses relative path to subdirectory + mainYAML := ` +imports: + - includes/shared.yaml + +modules: + - name: main-mod + type: test.main +` + mainPath := filepath.Join(dir, "main.yaml") + if err := os.WriteFile(mainPath, []byte(mainYAML), 0644); err != nil { + t.Fatal(err) + } + + cfg, err := LoadFromFile(mainPath) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if len(cfg.Modules) != 2 { + t.Errorf("expected 2 modules, got %d", len(cfg.Modules)) + } + if cfg.Modules[0].Name != "main-mod" { + t.Errorf("expected first module 'main-mod', got %q", cfg.Modules[0].Name) + } + if cfg.Modules[1].Name != "imported-mod" { + t.Errorf("expected second module 'imported-mod', got %q", cfg.Modules[1].Name) + } + if _, ok := cfg.Pipelines["imported-pipeline"]; !ok { + t.Error("expected 'imported-pipeline' from import") + } +} + +func TestLoadFromFile_ImportedImportsAlsoImport(t *testing.T) { + dir := t.TempDir() + + // Three-level deep: main -> domain1 -> shared + + // shared.yaml — common modules + sharedYAML := ` +modules: + - name: shared-db + type: storage.sqlite + config: + path: ./shared.db + +platform: + logging: + level: info +` + if err := os.WriteFile(filepath.Join(dir, "shared.yaml"), []byte(sharedYAML), 0644); err != nil { + t.Fatal(err) + } + + // domain1.yaml imports shared.yaml + domain1YAML := ` +imports: + - shared.yaml + +modules: + - name: domain1-service + type: http.server + config: + address: ":8081" + +pipelines: + domain1-handler: + steps: + - name: handle + type: step.log + +platform: + domain1: + feature: enabled +` + if err := os.WriteFile(filepath.Join(dir, "domain1.yaml"), []byte(domain1YAML), 0644); err != nil { + t.Fatal(err) + } + + // main.yaml imports domain1.yaml + mainYAML := ` +imports: + - domain1.yaml + +modules: + - name: main-gateway + type: http.server + config: + address: ":8080" + +workflows: + main: + steps: + - name: init + type: step.log + +platform: + main: + version: "1.0" +` + mainPath := filepath.Join(dir, "main.yaml") + if err := os.WriteFile(mainPath, []byte(mainYAML), 0644); err != nil { + t.Fatal(err) + } + + cfg, err := LoadFromFile(mainPath) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + // 3 modules: main-gateway, domain1-service, shared-db + if len(cfg.Modules) != 3 { + t.Errorf("expected 3 modules, got %d", len(cfg.Modules)) + } + if cfg.Modules[0].Name != "main-gateway" { + t.Errorf("expected first module 'main-gateway', got %q", cfg.Modules[0].Name) + } + if cfg.Modules[1].Name != "domain1-service" { + t.Errorf("expected second module 'domain1-service', got %q", cfg.Modules[1].Name) + } + if cfg.Modules[2].Name != "shared-db" { + t.Errorf("expected third module 'shared-db', got %q", cfg.Modules[2].Name) + } + + // Pipeline from domain1 + if _, ok := cfg.Pipelines["domain1-handler"]; !ok { + t.Error("expected 'domain1-handler' pipeline from domain1 import") + } + + // Workflow from main + if _, ok := cfg.Workflows["main"]; !ok { + t.Error("expected 'main' workflow") + } + + // Platform should have all three keys + if cfg.Platform == nil { + t.Fatal("expected platform config") + } + for _, key := range []string{"main", "domain1", "logging"} { + if _, ok := cfg.Platform[key]; !ok { + t.Errorf("expected platform key %q", key) + } + } +} diff --git a/module/pipeline_template.go b/module/pipeline_template.go index c66ed3f3..6883a603 100644 --- a/module/pipeline_template.go +++ b/module/pipeline_template.go @@ -4,6 +4,7 @@ import ( "bytes" "encoding/json" "fmt" + "regexp" "strings" "text/template" "time" @@ -40,6 +41,168 @@ func (te *TemplateEngine) templateData(pc *PipelineContext) map[string]any { return data } +// dotChainRe matches dot-access chains like .steps.my-step.field +var dotChainRe = regexp.MustCompile(`\.[a-zA-Z_][a-zA-Z0-9_-]*(?:\.[a-zA-Z_][a-zA-Z0-9_-]*)*`) + +// stringLiteralRe matches double-quoted and backtick-quoted string literals. +var stringLiteralRe = regexp.MustCompile(`"(?:[^"\\]|\\.)*"` + "|`[^`]*`") + +// preprocessTemplate rewrites dot-access chains containing hyphens into index +// syntax so that Go's text/template parser does not treat hyphens as minus. +// For example: {{ .steps.my-step.field }} → {{ (index .steps "my-step" "field") }} +func preprocessTemplate(tmplStr string) string { + // Quick exit: nothing to do if there are no actions or no hyphens. + if !strings.Contains(tmplStr, "{{") || !strings.Contains(tmplStr, "-") { + return tmplStr + } + + var out strings.Builder + rest := tmplStr + + for { + openIdx := strings.Index(rest, "{{") + if openIdx < 0 { + out.WriteString(rest) + break + } + closeIdx := strings.Index(rest[openIdx:], "}}") + if closeIdx < 0 { + out.WriteString(rest) + break + } + closeIdx += openIdx // absolute position + + // Write text before the action. + out.WriteString(rest[:openIdx]) + + action := rest[openIdx+2 : closeIdx] // content between {{ and }} + + // Skip template comments {{/* ... */}}. + trimmed := strings.TrimSpace(action) + if strings.HasPrefix(trimmed, "/*") && strings.HasSuffix(trimmed, "*/") { + out.WriteString("{{") + out.WriteString(action) + out.WriteString("}}") + rest = rest[closeIdx+2:] + continue + } + + // Strip string literals to avoid false matches on quoted hyphens. + var placeholders []string + stripped := stringLiteralRe.ReplaceAllStringFunc(action, func(m string) string { + placeholders = append(placeholders, m) + return "\x00" + }) + + // Rewrite hyphenated dot-chains in the stripped action. + rewritten := dotChainRe.ReplaceAllStringFunc(stripped, func(chain string) string { + segments := strings.Split(chain[1:], ".") // drop leading dot + hasHyphen := false + for _, seg := range segments { + if strings.Contains(seg, "-") { + hasHyphen = true + break + } + } + if !hasHyphen { + return chain // no hyphens → leave as-is + } + + // Find the first hyphenated segment. + firstHyphen := -1 + for i, seg := range segments { + if strings.Contains(seg, "-") { + firstHyphen = i + break + } + } + + // Build the prefix (non-hyphenated dot-access) and the quoted tail. + var prefix string + if firstHyphen == 0 { + prefix = "." + } else { + prefix = "." + strings.Join(segments[:firstHyphen], ".") + } + + var quoted []string + for _, seg := range segments[firstHyphen:] { + quoted = append(quoted, `"`+seg+`"`) + } + + return "(index " + prefix + " " + strings.Join(quoted, " ") + ")" + }) + + // Restore string literals from placeholders. + var restored string + if len(placeholders) > 0 { + phIdx := 0 + var final strings.Builder + for i := 0; i < len(rewritten); i++ { + if rewritten[i] == '\x00' && phIdx < len(placeholders) { + final.WriteString(placeholders[phIdx]) + phIdx++ + } else { + final.WriteByte(rewritten[i]) + } + } + restored = final.String() + } else { + restored = rewritten + } + + out.WriteString("{{") + out.WriteString(restored) + out.WriteString("}}") + rest = rest[closeIdx+2:] + } + + return out.String() +} + +// funcMapWithContext returns the base template functions plus context-aware +// helper functions (step, trigger) that access PipelineContext data directly. +func (te *TemplateEngine) funcMapWithContext(pc *PipelineContext) template.FuncMap { + fm := templateFuncMap() + + // step accesses step outputs by name and optional nested keys. + // Usage: {{ step "parse-request" "path_params" "id" }} + fm["step"] = func(name string, keys ...string) any { + stepMap, ok := pc.StepOutputs[name] + if !ok || stepMap == nil { + return nil + } + var val any = stepMap + for _, key := range keys { + m, ok := val.(map[string]any) + if !ok { + return nil + } + val = m[key] + } + return val + } + + // trigger accesses trigger data by nested keys. + // Usage: {{ trigger "path_params" "id" }} + fm["trigger"] = func(keys ...string) any { + if pc.TriggerData == nil { + return nil + } + var val any = map[string]any(pc.TriggerData) + for _, key := range keys { + m, ok := val.(map[string]any) + if !ok { + return nil + } + val = m[key] + } + return val + } + + return fm +} + // Resolve evaluates a template string against a PipelineContext. // If the string does not contain {{ }}, it is returned as-is. func (te *TemplateEngine) Resolve(tmplStr string, pc *PipelineContext) (string, error) { @@ -47,7 +210,9 @@ func (te *TemplateEngine) Resolve(tmplStr string, pc *PipelineContext) (string, return tmplStr, nil } - t, err := template.New("").Funcs(templateFuncMap()).Option("missingkey=zero").Parse(tmplStr) + tmplStr = preprocessTemplate(tmplStr) + + t, err := template.New("").Funcs(te.funcMapWithContext(pc)).Option("missingkey=zero").Parse(tmplStr) if err != nil { return "", fmt.Errorf("template parse error: %w", err) } diff --git a/module/pipeline_template_test.go b/module/pipeline_template_test.go index 029c179b..fa2b75d6 100644 --- a/module/pipeline_template_test.go +++ b/module/pipeline_template_test.go @@ -366,3 +366,190 @@ func TestTemplateEngine_ResolveMap_DoesNotMutateInput(t *testing.T) { t.Errorf("expected 'resolved', got %v", result["key"]) } } + +// --- preprocessTemplate tests --- + +func TestPreprocessTemplate_HyphenatedStepName(t *testing.T) { + input := "{{ .steps.my-step.field }}" + result := preprocessTemplate(input) + if !strings.Contains(result, `index .steps "my-step" "field"`) { + t.Errorf("expected index rewrite, got %q", result) + } +} + +func TestPreprocessTemplate_NoHyphens(t *testing.T) { + input := "{{ .steps.validate.result }}" + result := preprocessTemplate(input) + if result != input { + t.Errorf("expected unchanged %q, got %q", input, result) + } +} + +func TestPreprocessTemplate_SingleHyphenatedSegment(t *testing.T) { + input := "{{ .my-var }}" + result := preprocessTemplate(input) + if !strings.Contains(result, `index . "my-var"`) { + t.Errorf("expected index rewrite for single hyphenated segment, got %q", result) + } +} + +func TestPreprocessTemplate_MultipleHyphenatedSegments(t *testing.T) { + input := "{{ .steps.my-step.sub-field.more }}" + result := preprocessTemplate(input) + if !strings.Contains(result, `index .steps "my-step" "sub-field" "more"`) { + t.Errorf("expected index rewrite for multiple hyphenated segments, got %q", result) + } +} + +func TestPreprocessTemplate_WithPipe(t *testing.T) { + input := "{{ .steps.my-step.field | lower }}" + result := preprocessTemplate(input) + if !strings.Contains(result, `index .steps "my-step" "field"`) { + t.Errorf("expected index rewrite before pipe, got %q", result) + } + if !strings.Contains(result, "| lower") { + t.Errorf("expected pipe preserved, got %q", result) + } +} + +func TestPreprocessTemplate_WithFunction(t *testing.T) { + input := `{{ default "x" .steps.my-step.field }}` + result := preprocessTemplate(input) + if !strings.Contains(result, `index .steps "my-step" "field"`) { + t.Errorf("expected index rewrite with function, got %q", result) + } + if !strings.Contains(result, `default "x"`) { + t.Errorf("expected function preserved, got %q", result) + } +} + +func TestPreprocessTemplate_StringLiteralsSkipped(t *testing.T) { + input := `{{ index .steps "my-step" "field" }}` + result := preprocessTemplate(input) + if result != input { + t.Errorf("expected unchanged when hyphens are in string literals, got %q", result) + } +} + +func TestPreprocessTemplate_MixedContent(t *testing.T) { + input := "Hello {{ .steps.my-step.name }}!" + result := preprocessTemplate(input) + if !strings.HasPrefix(result, "Hello ") { + t.Errorf("expected text prefix preserved, got %q", result) + } + if !strings.HasSuffix(result, "!") { + t.Errorf("expected text suffix preserved, got %q", result) + } + if !strings.Contains(result, `index .steps "my-step" "name"`) { + t.Errorf("expected index rewrite in action, got %q", result) + } +} + +func TestPreprocessTemplate_NoTemplate(t *testing.T) { + input := "plain text" + result := preprocessTemplate(input) + if result != input { + t.Errorf("expected unchanged %q, got %q", input, result) + } +} + +func TestPreprocessTemplate_AlreadyUsingIndex(t *testing.T) { + input := `{{ index .steps "my-step" "field" }}` + result := preprocessTemplate(input) + if result != input { + t.Errorf("expected unchanged %q, got %q", input, result) + } +} + +// --- step and trigger helper function tests --- + +func TestTemplateEngine_StepFunction(t *testing.T) { + te := NewTemplateEngine() + pc := NewPipelineContext(nil, nil) + pc.MergeStepOutput("validate", map[string]any{"result": "passed"}) + + result, err := te.Resolve(`{{ step "validate" "result" }}`, pc) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if result != "passed" { + t.Errorf("expected 'passed', got %q", result) + } +} + +func TestTemplateEngine_StepFunctionHyphenated(t *testing.T) { + te := NewTemplateEngine() + pc := NewPipelineContext(nil, nil) + pc.MergeStepOutput("parse-request", map[string]any{ + "path_params": map[string]any{"id": "42"}, + }) + + result, err := te.Resolve(`{{ step "parse-request" "path_params" "id" }}`, pc) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if result != "42" { + t.Errorf("expected '42', got %q", result) + } +} + +func TestTemplateEngine_StepFunctionDeepNesting(t *testing.T) { + te := NewTemplateEngine() + pc := NewPipelineContext(nil, nil) + pc.MergeStepOutput("parse-request", map[string]any{ + "body": map[string]any{ + "nested": "deep-value", + }, + }) + + result, err := te.Resolve(`{{ step "parse-request" "body" "nested" }}`, pc) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if result != "deep-value" { + t.Errorf("expected 'deep-value', got %q", result) + } +} + +func TestTemplateEngine_StepFunctionMissing(t *testing.T) { + te := NewTemplateEngine() + pc := NewPipelineContext(nil, nil) + + result, err := te.Resolve(`{{ step "nonexistent" "field" }}`, pc) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + // nil renders as "" with missingkey=zero; just verify no error + _ = result +} + +func TestTemplateEngine_TriggerFunction(t *testing.T) { + te := NewTemplateEngine() + pc := NewPipelineContext(map[string]any{ + "path_params": map[string]any{"id": "99"}, + }, nil) + + result, err := te.Resolve(`{{ trigger "path_params" "id" }}`, pc) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if result != "99" { + t.Errorf("expected '99', got %q", result) + } +} + +func TestTemplateEngine_HyphenatedResolveEndToEnd(t *testing.T) { + te := NewTemplateEngine() + pc := NewPipelineContext(nil, nil) + pc.MergeStepOutput("parse-request", map[string]any{ + "path_params": map[string]any{"id": "123"}, + }) + + result, err := te.Resolve("{{ .steps.parse-request.path_params.id }}", pc) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if result != "123" { + t.Errorf("expected '123', got %q", result) + } +} From dcde0fa9911609ffa6d33191d50a4e4dd78705b0 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Fri, 27 Feb 2026 09:10:31 -0500 Subject: [PATCH 2/4] fix: address PR #202 review comments - Fix diamond import bug: use stack-based (defer delete) circular detection so shared dependencies imported via multiple paths don't false-positive - Fix hyphenDotRe regex to match full chains (e.g. .steps.my-step.field) - Tighten stepRefFuncRe regex to avoid false positives on non-function contexts - Use unique placeholder sentinel instead of bare \x00 for string literal stripping - Differentiate self-reference from forward-reference in validation warnings - Add diamond import test case - Add self-reference validation test case - Document depth-first import merge order and ambiguous hyphen/subtraction behavior - Add clarifying comments on collectTemplateStrings and stringLiteralRe Co-Authored-By: Claude Opus 4.6 --- DOCUMENTATION.md | 3 +- cmd/wfctl/template_validate.go | 17 +++++-- cmd/wfctl/template_validate_test.go | 36 +++++++++++++++ config/config.go | 7 +++ config/config_import_test.go | 71 +++++++++++++++++++++++++++++ module/pipeline_template.go | 15 ++++-- 6 files changed, 140 insertions(+), 9 deletions(-) diff --git a/DOCUMENTATION.md b/DOCUMENTATION.md index 71d70aa7..945a58b7 100644 --- a/DOCUMENTATION.md +++ b/DOCUMENTATION.md @@ -912,7 +912,8 @@ triggers: **Merge rules:** - **Modules:** All modules from all files are included. Main file's modules appear first. - **Pipelines, triggers, workflows, platform:** Main file's definitions take precedence. Imported values only fill in keys not already defined. -- **Recursive imports:** Imported files can themselves use `imports`. Circular imports are detected and produce an error. +- **Recursive imports:** Imported files can themselves use `imports`. Circular imports are detected and produce an error. Diamond imports (multiple files importing a shared dependency) are allowed. +- **Import order:** Depth-first traversal — if main imports [A, B] and A imports C, module order is: main, A, C, B. - **Relative paths:** Import paths are resolved relative to the importing file's directory. **Comparison with ApplicationConfig:** diff --git a/cmd/wfctl/template_validate.go b/cmd/wfctl/template_validate.go index c76f6af9..e01efe6c 100644 --- a/cmd/wfctl/template_validate.go +++ b/cmd/wfctl/template_validate.go @@ -545,11 +545,13 @@ var stepRefDotRe = regexp.MustCompile(`\.steps\.([a-zA-Z_][a-zA-Z0-9_-]*)`) // stepRefIndexRe matches index .steps "STEP_NAME" patterns. var stepRefIndexRe = regexp.MustCompile(`index\s+\.steps\s+"([^"]+)"`) -// stepRefFuncRe matches step "STEP_NAME" function calls. -var stepRefFuncRe = regexp.MustCompile(`\bstep\s+"([^"]+)"`) +// stepRefFuncRe matches step "STEP_NAME" function calls at the start of an +// action, after a pipe, or after an opening parenthesis. +var stepRefFuncRe = regexp.MustCompile(`(?:^|\||\()\s*step\s+"([^"]+)"`) -// hyphenDotRe matches dot-access chains with hyphens (e.g., .steps.my-step.field). -var hyphenDotRe = regexp.MustCompile(`\.[a-zA-Z_][a-zA-Z0-9_]*-[a-zA-Z0-9_-]*`) +// hyphenDotRe matches dot-access chains with hyphens (e.g., .steps.my-step.field), +// including continuation segments after the hyphenated part. +var hyphenDotRe = regexp.MustCompile(`\.[a-zA-Z_][a-zA-Z0-9_]*-[a-zA-Z0-9_-]*(?:\.[a-zA-Z_][a-zA-Z0-9_-]*)*`) // validatePipelineTemplates checks template expressions in pipeline step configs for // references to nonexistent or forward-declared steps and common template pitfalls. @@ -634,13 +636,18 @@ func validateStepRef(pipelineName, currentStep, refName string, currentIdx int, if !exists { result.Warnings = append(result.Warnings, fmt.Sprintf("pipeline %q step %q: references step %q which does not exist in this pipeline", pipelineName, currentStep, refName)) - } else if refIdx >= currentIdx { + } else if refIdx == currentIdx { + result.Warnings = append(result.Warnings, + fmt.Sprintf("pipeline %q step %q: references itself; a step cannot use its own outputs because they are not available until after execution", pipelineName, currentStep)) + } else if refIdx > currentIdx { result.Warnings = append(result.Warnings, fmt.Sprintf("pipeline %q step %q: references step %q which has not executed yet (appears later in pipeline)", pipelineName, currentStep, refName)) } } // collectTemplateStrings recursively finds all strings containing {{ in a value tree. +// This intentionally scans all fields (not just "config") because template expressions +// can appear in conditions, names, and other step fields. func collectTemplateStrings(data any) []string { var results []string switch v := data.(type) { diff --git a/cmd/wfctl/template_validate_test.go b/cmd/wfctl/template_validate_test.go index 904d23f1..8c4c6413 100644 --- a/cmd/wfctl/template_validate_test.go +++ b/cmd/wfctl/template_validate_test.go @@ -393,6 +393,42 @@ func TestValidateConfigWithStepFunction(t *testing.T) { } } +func TestValidateConfigWithSelfReference(t *testing.T) { + cfg := &config.WorkflowConfig{ + Pipelines: map[string]any{ + "api": map[string]any{ + "steps": []any{ + map[string]any{ + "name": "do-thing", + "type": "step.set", + "config": map[string]any{ + "values": map[string]any{ + "x": "{{ .steps.do-thing.output }}", + }, + }, + }, + }, + }, + }, + } + knownModules := KnownModuleTypes() + knownSteps := KnownStepTypes() + knownTriggers := KnownTriggerTypes() + + result := validateWorkflowConfig("test", cfg, knownModules, knownSteps, knownTriggers) + + found := false + for _, w := range result.Warnings { + if strings.Contains(w, "references itself") { + found = true + break + } + } + if !found { + t.Errorf("expected self-reference warning, got warnings: %v", result.Warnings) + } +} + func TestValidateConfigWithHyphenDotAccess(t *testing.T) { cfg := &config.WorkflowConfig{ Pipelines: map[string]any{ diff --git a/config/config.go b/config/config.go index b298d965..264e1458 100644 --- a/config/config.go +++ b/config/config.go @@ -137,6 +137,7 @@ func loadFromFileWithImports(filepath string, seen map[string]bool) (*WorkflowCo return nil, fmt.Errorf("circular import detected: %s", filepath) } seen[absPath] = true + defer delete(seen, absPath) data, err := os.ReadFile(absPath) if err != nil { @@ -164,6 +165,12 @@ func loadFromFileWithImports(filepath string, seen map[string]bool) (*WorkflowCo // Imported definitions provide defaults — the importing file's own definitions take // precedence for map-based fields (workflows, triggers, pipelines, platform). // Modules are appended after the main file's modules. +// +// Import order follows depth-first traversal: if main.yaml imports [A, B] and A +// imports C, the final module order is: main's modules, A's modules, C's modules, +// B's modules. Diamond imports (multiple files importing a shared dependency) are +// allowed — the shared file is loaded each time but only non-duplicate map keys +// are added (main-file-wins semantics). func (cfg *WorkflowConfig) processImports(seen map[string]bool) error { for _, imp := range cfg.Imports { impPath := imp diff --git a/config/config_import_test.go b/config/config_import_test.go index d45bdf83..fbd92b98 100644 --- a/config/config_import_test.go +++ b/config/config_import_test.go @@ -563,3 +563,74 @@ platform: } } } + +func TestLoadFromFile_DiamondImports(t *testing.T) { + dir := t.TempDir() + + // shared.yaml (D) - common dependency imported by both B and C + sharedYAML := ` +modules: + - name: shared-db + type: state.connector +` + if err := os.WriteFile(filepath.Join(dir, "shared.yaml"), []byte(sharedYAML), 0644); err != nil { + t.Fatal(err) + } + + // service-b.yaml (B) imports shared.yaml + serviceBYAML := ` +imports: + - shared.yaml + +modules: + - name: service-b + type: http.handler +` + if err := os.WriteFile(filepath.Join(dir, "service-b.yaml"), []byte(serviceBYAML), 0644); err != nil { + t.Fatal(err) + } + + // service-c.yaml (C) imports shared.yaml + serviceCYAML := ` +imports: + - shared.yaml + +modules: + - name: service-c + type: http.handler +` + if err := os.WriteFile(filepath.Join(dir, "service-c.yaml"), []byte(serviceCYAML), 0644); err != nil { + t.Fatal(err) + } + + // main.yaml (A) imports both B and C + mainYAML := ` +imports: + - service-b.yaml + - service-c.yaml + +modules: + - name: main-gateway + type: http.server +` + mainPath := filepath.Join(dir, "main.yaml") + if err := os.WriteFile(mainPath, []byte(mainYAML), 0644); err != nil { + t.Fatal(err) + } + + cfg, err := LoadFromFile(mainPath) + if err != nil { + t.Fatalf("unexpected error loading diamond imports: %v", err) + } + + gotModules := make(map[string]bool) + for _, m := range cfg.Modules { + gotModules[m.Name] = true + } + + for _, name := range []string{"main-gateway", "service-b", "service-c", "shared-db"} { + if !gotModules[name] { + t.Errorf("expected module %q to be loaded in diamond import scenario", name) + } + } +} diff --git a/module/pipeline_template.go b/module/pipeline_template.go index 6883a603..70aa9a98 100644 --- a/module/pipeline_template.go +++ b/module/pipeline_template.go @@ -41,10 +41,17 @@ func (te *TemplateEngine) templateData(pc *PipelineContext) map[string]any { return data } -// dotChainRe matches dot-access chains like .steps.my-step.field +// dotChainRe matches dot-access chains like .steps.my-step.field. +// Hyphens are intentionally allowed within identifier segments so that +// hyphenated step names and fields (e.g. .steps.my-step.field) are +// treated as a single chain. This means ambiguous cases like ".x-1" +// are interpreted as a hyphenated identifier ("x-1") rather than as +// subtraction ".x - 1" when applying the auto-fix rewrite. var dotChainRe = regexp.MustCompile(`\.[a-zA-Z_][a-zA-Z0-9_-]*(?:\.[a-zA-Z_][a-zA-Z0-9_-]*)*`) // stringLiteralRe matches double-quoted and backtick-quoted string literals. +// Go templates only support double-quoted and backtick strings (not single-quoted), +// so single quotes are intentionally not handled here. var stringLiteralRe = regexp.MustCompile(`"(?:[^"\\]|\\.)*"` + "|`[^`]*`") // preprocessTemplate rewrites dot-access chains containing hyphens into index @@ -89,9 +96,10 @@ func preprocessTemplate(tmplStr string) string { // Strip string literals to avoid false matches on quoted hyphens. var placeholders []string + const placeholderSentinel = "\x00" stripped := stringLiteralRe.ReplaceAllStringFunc(action, func(m string) string { placeholders = append(placeholders, m) - return "\x00" + return placeholderSentinel }) // Rewrite hyphenated dot-chains in the stripped action. @@ -139,9 +147,10 @@ func preprocessTemplate(tmplStr string) string { phIdx := 0 var final strings.Builder for i := 0; i < len(rewritten); i++ { - if rewritten[i] == '\x00' && phIdx < len(placeholders) { + if strings.HasPrefix(rewritten[i:], placeholderSentinel) && phIdx < len(placeholders) { final.WriteString(placeholders[phIdx]) phIdx++ + i += len(placeholderSentinel) - 1 // skip rest of sentinel } else { final.WriteByte(rewritten[i]) } From 2820930fd9bcb66e01a5efd81b7034a157bc453d Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Fri, 27 Feb 2026 09:18:26 -0500 Subject: [PATCH 3/4] fix: rewrite if-else chain to switch to satisfy gocritic linter Co-Authored-By: Claude Opus 4.6 --- cmd/wfctl/template_validate.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/cmd/wfctl/template_validate.go b/cmd/wfctl/template_validate.go index e01efe6c..9d36ff28 100644 --- a/cmd/wfctl/template_validate.go +++ b/cmd/wfctl/template_validate.go @@ -633,13 +633,14 @@ func validatePipelineTemplates(pipelineName string, stepsRaw []any, result *temp // current step in the pipeline execution order. func validateStepRef(pipelineName, currentStep, refName string, currentIdx int, stepNames map[string]int, result *templateValidationResult) { refIdx, exists := stepNames[refName] - if !exists { + switch { + case !exists: result.Warnings = append(result.Warnings, fmt.Sprintf("pipeline %q step %q: references step %q which does not exist in this pipeline", pipelineName, currentStep, refName)) - } else if refIdx == currentIdx { + case refIdx == currentIdx: result.Warnings = append(result.Warnings, fmt.Sprintf("pipeline %q step %q: references itself; a step cannot use its own outputs because they are not available until after execution", pipelineName, currentStep)) - } else if refIdx > currentIdx { + case refIdx > currentIdx: result.Warnings = append(result.Warnings, fmt.Sprintf("pipeline %q step %q: references step %q which has not executed yet (appears later in pipeline)", pipelineName, currentStep, refName)) } From ca970455f7edc0d28d0f10d8b048b0f638f7e7a4 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Fri, 27 Feb 2026 09:33:01 -0500 Subject: [PATCH 4/4] fix: address second round of PR #202 review comments - Fix validation step numbering to match execution order (1-5) - Add RE2 note to stringLiteralRe (Go regex has no backtracking/ReDoS) - Optimize placeholder restoration: use strings.Index instead of O(n*m) scan - Deduplicate modules in diamond imports (first definition wins) - Clarify comment detection: only pure block comments are skipped - Document step/trigger function nil-return semantics - Fix DOCUMENTATION.md escaped pipes in trimPrefix/trimSuffix examples - Add tests: unclosed template action, escaped quotes in string literals - Diamond import test now verifies no duplicate modules Co-Authored-By: Claude Opus 4.6 --- DOCUMENTATION.md | 4 ++-- cmd/wfctl/template_validate.go | 12 ++++++------ config/config.go | 14 ++++++++++++-- config/config_import_test.go | 7 +++++++ module/pipeline_template.go | 25 ++++++++++++++++++------- module/pipeline_template_test.go | 16 ++++++++++++++++ 6 files changed, 61 insertions(+), 17 deletions(-) diff --git a/DOCUMENTATION.md b/DOCUMENTATION.md index 945a58b7..2b7f384d 100644 --- a/DOCUMENTATION.md +++ b/DOCUMENTATION.md @@ -128,8 +128,8 @@ Pipeline steps support Go template syntax with these built-in functions: | `lower` | Lowercase string | `{{ lower .name }}` | | `default` | Default value when empty | `{{ default "pending" .status }}` | | `json` | Marshal value to JSON string | `{{ json .data }}` | -| `trimPrefix` | Remove prefix from string | `{{ .phone \| trimPrefix "+" }}` | -| `trimSuffix` | Remove suffix from string | `{{ .file \| trimSuffix ".txt" }}` | +| `trimPrefix` | Remove prefix from string | `{{ .phone | trimPrefix "+" }}` | +| `trimSuffix` | Remove suffix from string | `{{ .file | trimSuffix ".txt" }}` | | `step` | Access step outputs by name and keys | `{{ step "parse-request" "path_params" "id" }}` | | `trigger` | Access trigger data by keys | `{{ trigger "path_params" "id" }}` | diff --git a/cmd/wfctl/template_validate.go b/cmd/wfctl/template_validate.go index 9d36ff28..ee58e761 100644 --- a/cmd/wfctl/template_validate.go +++ b/cmd/wfctl/template_validate.go @@ -341,7 +341,7 @@ func validateWorkflowConfig(name string, cfg *config.WorkflowConfig, knownModule moduleNames[mod.Name] = true } - // 1. Validate module types + // 1. Validate module types and config fields result.ModuleCount = len(cfg.Modules) for _, mod := range cfg.Modules { if mod.Type == "" { @@ -353,7 +353,7 @@ func validateWorkflowConfig(name string, cfg *config.WorkflowConfig, knownModule result.Errors = append(result.Errors, fmt.Sprintf("module %q uses unknown type %q", mod.Name, mod.Type)) } else { result.ModuleValid++ - // 5. Warn on unknown config fields + // Warn on unknown config fields if mod.Config != nil && len(info.ConfigKeys) > 0 { knownKeys := make(map[string]bool) for _, k := range info.ConfigKeys { @@ -367,7 +367,7 @@ func validateWorkflowConfig(name string, cfg *config.WorkflowConfig, knownModule } } - // 3. Validate dependencies + // 2. Validate dependencies for _, dep := range mod.DependsOn { result.DepCount++ if !moduleNames[dep] { @@ -378,7 +378,7 @@ func validateWorkflowConfig(name string, cfg *config.WorkflowConfig, knownModule } } - // 2. Validate step types in pipelines + // 3. Validate step types in pipelines for pipelineName, pipelineRaw := range cfg.Pipelines { pipelineMap, ok := pipelineRaw.(map[string]any) if !ok { @@ -416,10 +416,10 @@ func validateWorkflowConfig(name string, cfg *config.WorkflowConfig, knownModule } } - // 6. Validate template expressions in pipeline steps + // 4. Validate template expressions in pipeline steps validatePipelineTemplates(pipelineName, stepsRaw, &result) - // 4. Validate trigger types + // 5. Validate trigger types if triggerRaw, ok := pipelineMap["trigger"]; ok { if triggerMap, ok := triggerRaw.(map[string]any); ok { triggerType, _ := triggerMap["type"].(string) diff --git a/config/config.go b/config/config.go index 264e1458..57445f12 100644 --- a/config/config.go +++ b/config/config.go @@ -183,8 +183,18 @@ func (cfg *WorkflowConfig) processImports(seen map[string]bool) error { return fmt.Errorf("import %q: %w", imp, err) } - // Merge imported modules (append after main file's modules) - cfg.Modules = append(cfg.Modules, impCfg.Modules...) + // Merge imported modules — deduplicate by name (first definition wins) + existingModules := make(map[string]struct{}, len(cfg.Modules)) + for _, m := range cfg.Modules { + existingModules[m.Name] = struct{}{} + } + for _, m := range impCfg.Modules { + if _, exists := existingModules[m.Name]; exists { + continue + } + cfg.Modules = append(cfg.Modules, m) + existingModules[m.Name] = struct{}{} + } // Merge maps — imported values only added if not already defined in main file if cfg.Workflows == nil { diff --git a/config/config_import_test.go b/config/config_import_test.go index fbd92b98..0c142fb6 100644 --- a/config/config_import_test.go +++ b/config/config_import_test.go @@ -625,6 +625,9 @@ modules: gotModules := make(map[string]bool) for _, m := range cfg.Modules { + if gotModules[m.Name] { + t.Errorf("duplicate module %q found in diamond import scenario", m.Name) + } gotModules[m.Name] = true } @@ -633,4 +636,8 @@ modules: t.Errorf("expected module %q to be loaded in diamond import scenario", name) } } + + if len(cfg.Modules) != 4 { + t.Errorf("expected exactly 4 modules (no duplicates), got %d", len(cfg.Modules)) + } } diff --git a/module/pipeline_template.go b/module/pipeline_template.go index 70aa9a98..51870a61 100644 --- a/module/pipeline_template.go +++ b/module/pipeline_template.go @@ -52,6 +52,8 @@ var dotChainRe = regexp.MustCompile(`\.[a-zA-Z_][a-zA-Z0-9_-]*(?:\.[a-zA-Z_][a-z // stringLiteralRe matches double-quoted and backtick-quoted string literals. // Go templates only support double-quoted and backtick strings (not single-quoted), // so single quotes are intentionally not handled here. +// Note: Go's regexp package uses RE2 (linear-time matching), so there is no risk +// of catastrophic backtracking / ReDoS with this pattern. var stringLiteralRe = regexp.MustCompile(`"(?:[^"\\]|\\.)*"` + "|`[^`]*`") // preprocessTemplate rewrites dot-access chains containing hyphens into index @@ -84,7 +86,9 @@ func preprocessTemplate(tmplStr string) string { action := rest[openIdx+2 : closeIdx] // content between {{ and }} - // Skip template comments {{/* ... */}}. + // Skip pure template comments {{/* ... */}}. Only actions whose entire + // content (after trimming) is a block comment are skipped. Mixed actions + // like {{ x /* comment */ y }} are not skipped since they contain code. trimmed := strings.TrimSpace(action) if strings.HasPrefix(trimmed, "/*") && strings.HasSuffix(trimmed, "*/") { out.WriteString("{{") @@ -141,19 +145,24 @@ func preprocessTemplate(tmplStr string) string { return "(index " + prefix + " " + strings.Join(quoted, " ") + ")" }) - // Restore string literals from placeholders. + // Restore string literals from placeholders using strings.Index for O(n) scanning. var restored string if len(placeholders) > 0 { phIdx := 0 var final strings.Builder - for i := 0; i < len(rewritten); i++ { - if strings.HasPrefix(rewritten[i:], placeholderSentinel) && phIdx < len(placeholders) { + remaining := rewritten + for { + idx := strings.Index(remaining, placeholderSentinel) + if idx < 0 { + final.WriteString(remaining) + break + } + final.WriteString(remaining[:idx]) + if phIdx < len(placeholders) { final.WriteString(placeholders[phIdx]) phIdx++ - i += len(placeholderSentinel) - 1 // skip rest of sentinel - } else { - final.WriteByte(rewritten[i]) } + remaining = remaining[idx+len(placeholderSentinel):] } restored = final.String() } else { @@ -176,6 +185,8 @@ func (te *TemplateEngine) funcMapWithContext(pc *PipelineContext) template.FuncM // step accesses step outputs by name and optional nested keys. // Usage: {{ step "parse-request" "path_params" "id" }} + // Returns nil if the step doesn't exist, a key is missing, or an + // intermediate value is not a map (consistent with missingkey=zero). fm["step"] = func(name string, keys ...string) any { stepMap, ok := pc.StepOutputs[name] if !ok || stepMap == nil { diff --git a/module/pipeline_template_test.go b/module/pipeline_template_test.go index fa2b75d6..ff6be420 100644 --- a/module/pipeline_template_test.go +++ b/module/pipeline_template_test.go @@ -538,6 +538,22 @@ func TestTemplateEngine_TriggerFunction(t *testing.T) { } } +func TestPreprocessTemplate_UnclosedAction(t *testing.T) { + input := "{{ .steps.my-step.field" + result := preprocessTemplate(input) + if result != input { + t.Errorf("unclosed action should be returned unchanged, got %q", result) + } +} + +func TestPreprocessTemplate_EscapedQuotesPreserved(t *testing.T) { + input := `{{ index .steps "my\"step" "field" }}` + result := preprocessTemplate(input) + if !strings.Contains(result, `"my\"step"`) { + t.Errorf("escaped quotes should be preserved, got %q", result) + } +} + func TestTemplateEngine_HyphenatedResolveEndToEnd(t *testing.T) { te := NewTemplateEngine() pc := NewPipelineContext(nil, nil)