From 40d526002fd7b40a564dc6106536dc30944629b4 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 24 Mar 2026 17:59:22 +0000 Subject: [PATCH 1/4] Initial plan From 8a2d142f6028494569bf31edbbb04cc97adde688 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 24 Mar 2026 18:22:36 +0000 Subject: [PATCH 2/4] feat: implement pipeline output schema and step output field static analysis - Add PipelineOutputDef and Outputs field to PipelineConfig (Phase 2) - Add static output field validation to wfctl template validate (Phase 1) - Add ResponseSchema to EndpointContract with breaking change detection (Phase 4) - Tests for all new functionality Co-authored-by: intel352 <77607+intel352@users.noreply.github.com> Agent-Logs-Url: https://github.com/GoCodeAlone/workflow/sessions/ef399305-04d6-4981-ba4c-4fd338147b57 --- cmd/wfctl/contract.go | 88 +++++++-- cmd/wfctl/contract_test.go | 232 ++++++++++++++++++++++ cmd/wfctl/infra_state.go | 20 +- cmd/wfctl/main.go | 48 ++--- cmd/wfctl/plugin_install_new_test.go | 8 +- cmd/wfctl/template_validate.go | 82 +++++++- cmd/wfctl/template_validate_test.go | 284 +++++++++++++++++++++++++-- cmd/wfctl/test.go | 12 +- config/pipeline.go | 16 ++ 9 files changed, 714 insertions(+), 76 deletions(-) diff --git a/cmd/wfctl/contract.go b/cmd/wfctl/contract.go index dc059d8a..51a200bf 100644 --- a/cmd/wfctl/contract.go +++ b/cmd/wfctl/contract.go @@ -26,10 +26,11 @@ type Contract struct { // EndpointContract describes an HTTP endpoint in the contract. type EndpointContract struct { - Method string `json:"method"` - Path string `json:"path"` - AuthRequired bool `json:"authRequired"` - Pipeline string `json:"pipeline"` + Method string `json:"method"` + Path string `json:"path"` + AuthRequired bool `json:"authRequired"` + Pipeline string `json:"pipeline"` + ResponseSchema map[string]string `json:"responseSchema,omitempty"` // field name → type } // ModuleContract describes a module in the contract. @@ -268,6 +269,8 @@ func generateContract(cfg *config.WorkflowConfig) *Contract { } } } + // Populate response schema from pipeline outputs declaration + ep.ResponseSchema = extractPipelineOutputSchema(pipelineMap) contract.Endpoints = append(contract.Endpoints, ep) } } @@ -343,6 +346,31 @@ func generateContract(cfg *config.WorkflowConfig) *Contract { return contract } +// extractPipelineOutputSchema reads the optional "outputs" block from a raw +// pipeline map and returns a field→type map for use in EndpointContract. +// Returns nil when the pipeline has no outputs declaration. +func extractPipelineOutputSchema(pipelineMap map[string]any) map[string]string { + outputsRaw, ok := pipelineMap["outputs"] + if !ok || outputsRaw == nil { + return nil + } + outputsMap, ok := outputsRaw.(map[string]any) + if !ok || len(outputsMap) == 0 { + return nil + } + schema := make(map[string]string, len(outputsMap)) + for field, defRaw := range outputsMap { + fieldType := "any" + if defMap, ok := defRaw.(map[string]any); ok { + if t, ok := defMap["type"].(string); ok && t != "" { + fieldType = t + } + } + schema[field] = fieldType + } + return schema +} + // compareContracts compares a baseline contract to the current one. func compareContracts(base, current *Contract) *contractComparison { comp := &contractComparison{ @@ -365,18 +393,37 @@ func compareContracts(base, current *Contract) *contractComparison { // Check base endpoints for key, baseEP := range baseEPs { if currentEP, exists := currentEPs[key]; exists { - // Check for breaking changes + // Collect all breaking changes for this endpoint + var breakingDetails []string + + // Auth was added to a public endpoint if baseEP.AuthRequired != currentEP.AuthRequired && !baseEP.AuthRequired { - // Auth was added to a public endpoint - comp.Endpoints = append(comp.Endpoints, endpointChange{ - Method: baseEP.Method, - Path: baseEP.Path, - Pipeline: currentEP.Pipeline, - Change: changeChanged, - Detail: "auth requirement added (clients without tokens will get 401)", - IsBreaking: true, - }) - comp.BreakingCount++ + breakingDetails = append(breakingDetails, "auth requirement added (clients without tokens will get 401)") + } + + // Response schema fields were removed or changed type + for field, baseType := range baseEP.ResponseSchema { + if currentType, exists := currentEP.ResponseSchema[field]; !exists { + breakingDetails = append(breakingDetails, + fmt.Sprintf("response field %q removed (was %s)", field, baseType)) + } else if baseType != currentType && baseType != "any" && currentType != "any" { + breakingDetails = append(breakingDetails, + fmt.Sprintf("response field %q changed type from %s to %s", field, baseType, currentType)) + } + } + + if len(breakingDetails) > 0 { + for _, detail := range breakingDetails { + comp.Endpoints = append(comp.Endpoints, endpointChange{ + Method: baseEP.Method, + Path: baseEP.Path, + Pipeline: currentEP.Pipeline, + Change: changeChanged, + Detail: detail, + IsBreaking: true, + }) + comp.BreakingCount++ + } } else { comp.Endpoints = append(comp.Endpoints, endpointChange{ Method: baseEP.Method, @@ -511,6 +558,17 @@ func printContract(c *Contract) { auth = " [auth]" } fmt.Printf(" %-7s %s%s (pipeline: %s)\n", ep.Method, ep.Path, auth, ep.Pipeline) + if len(ep.ResponseSchema) > 0 { + // Print response schema fields sorted for stable output + fields := make([]string, 0, len(ep.ResponseSchema)) + for f := range ep.ResponseSchema { + fields = append(fields, f) + } + sort.Strings(fields) + for _, f := range fields { + fmt.Printf(" response.%s: %s\n", f, ep.ResponseSchema[f]) + } + } } fmt.Printf("\nModules (%d):\n", len(c.Modules)) diff --git a/cmd/wfctl/contract_test.go b/cmd/wfctl/contract_test.go index 61e81706..0c272bb6 100644 --- a/cmd/wfctl/contract_test.go +++ b/cmd/wfctl/contract_test.go @@ -560,3 +560,235 @@ pipelines: t.Errorf("expected 'breaking' in error message, got: %v", err) } } + +// TestGenerateContractResponseSchema checks that the response schema is extracted +// from the pipeline's optional outputs declaration. +func TestGenerateContractResponseSchema(t *testing.T) { + cfg := &config.WorkflowConfig{ + Pipelines: map[string]any{ + "get-item": map[string]any{ + "trigger": map[string]any{ + "type": "http", + "config": map[string]any{ + "path": "/api/items/:id", + "method": "GET", + }, + }, + "outputs": map[string]any{ + "id": map[string]any{"type": "string"}, + "name": map[string]any{"type": "string"}, + "found": map[string]any{"type": "boolean"}, + }, + "steps": []any{}, + }, + }, + } + + contract := generateContract(cfg) + + if len(contract.Endpoints) != 1 { + t.Fatalf("expected 1 endpoint, got %d", len(contract.Endpoints)) + } + ep := contract.Endpoints[0] + if len(ep.ResponseSchema) != 3 { + t.Fatalf("expected 3 response schema fields, got %d: %v", len(ep.ResponseSchema), ep.ResponseSchema) + } + if ep.ResponseSchema["id"] != "string" { + t.Errorf("expected id=string, got %q", ep.ResponseSchema["id"]) + } + if ep.ResponseSchema["found"] != "boolean" { + t.Errorf("expected found=boolean, got %q", ep.ResponseSchema["found"]) + } +} + +// TestGenerateContractNoResponseSchema checks that endpoints without an outputs +// block have a nil ResponseSchema (backwards-compatible). +func TestGenerateContractNoResponseSchema(t *testing.T) { + cfg := &config.WorkflowConfig{ + Pipelines: map[string]any{ + "get-item": map[string]any{ + "trigger": map[string]any{ + "type": "http", + "config": map[string]any{ + "path": "/api/items", + "method": "GET", + }, + }, + "steps": []any{}, + }, + }, + } + + contract := generateContract(cfg) + if len(contract.Endpoints) != 1 { + t.Fatalf("expected 1 endpoint, got %d", len(contract.Endpoints)) + } + if contract.Endpoints[0].ResponseSchema != nil { + t.Errorf("expected nil ResponseSchema for pipeline without outputs, got %v", contract.Endpoints[0].ResponseSchema) + } +} + +// TestCompareContracts_ResponseSchemaFieldRemoved checks that removing a +// declared response schema field is detected as a breaking change. +func TestCompareContracts_ResponseSchemaFieldRemoved(t *testing.T) { + base := &Contract{ + Version: "1.0", + Endpoints: []EndpointContract{ + { + Method: "GET", + Path: "/api/items", + Pipeline: "get-items", + ResponseSchema: map[string]string{ + "id": "string", + "name": "string", + "found": "boolean", + }, + }, + }, + } + current := &Contract{ + Version: "1.0", + Endpoints: []EndpointContract{ + { + Method: "GET", + Path: "/api/items", + Pipeline: "get-items", + ResponseSchema: map[string]string{ + "id": "string", + "name": "string", + // "found" has been removed + }, + }, + }, + } + + comp := compareContracts(base, current) + + if comp.BreakingCount == 0 { + t.Error("expected breaking change when response field is removed") + } + found := false + for _, ec := range comp.Endpoints { + if ec.IsBreaking && strings.Contains(ec.Detail, "found") && strings.Contains(ec.Detail, "removed") { + found = true + break + } + } + if !found { + t.Errorf("expected breaking change about removed field 'found', got: %v", comp.Endpoints) + } +} + +// TestCompareContracts_ResponseSchemaTypeChanged checks that changing a +// declared response schema field type is detected as a breaking change. +func TestCompareContracts_ResponseSchemaTypeChanged(t *testing.T) { + base := &Contract{ + Version: "1.0", + Endpoints: []EndpointContract{ + { + Method: "GET", + Path: "/api/items", + Pipeline: "get-items", + ResponseSchema: map[string]string{ + "count": "integer", + }, + }, + }, + } + current := &Contract{ + Version: "1.0", + Endpoints: []EndpointContract{ + { + Method: "GET", + Path: "/api/items", + Pipeline: "get-items", + ResponseSchema: map[string]string{ + "count": "string", // changed from integer to string + }, + }, + }, + } + + comp := compareContracts(base, current) + + if comp.BreakingCount == 0 { + t.Error("expected breaking change when response field type changes") + } + found := false + for _, ec := range comp.Endpoints { + if ec.IsBreaking && strings.Contains(ec.Detail, "count") && strings.Contains(ec.Detail, "changed type") { + found = true + break + } + } + if !found { + t.Errorf("expected breaking change about type change for 'count', got: %v", comp.Endpoints) + } +} + +// TestCompareContracts_ResponseSchemaFieldAdded checks that adding a new +// response schema field is not a breaking change. +func TestCompareContracts_ResponseSchemaFieldAdded(t *testing.T) { + base := &Contract{ + Version: "1.0", + Endpoints: []EndpointContract{ + { + Method: "GET", + Path: "/api/items", + Pipeline: "get-items", + ResponseSchema: map[string]string{"id": "string"}, + }, + }, + } + current := &Contract{ + Version: "1.0", + Endpoints: []EndpointContract{ + { + Method: "GET", + Path: "/api/items", + Pipeline: "get-items", + ResponseSchema: map[string]string{"id": "string", "name": "string"}, + }, + }, + } + + comp := compareContracts(base, current) + + if comp.BreakingCount != 0 { + t.Errorf("expected no breaking changes when a new field is added, got %d: %v", comp.BreakingCount, comp.Endpoints) + } +} + +// TestExtractPipelineOutputSchema tests the helper that reads the outputs block. +func TestExtractPipelineOutputSchema(t *testing.T) { + t.Run("with outputs", func(t *testing.T) { + pipelineMap := map[string]any{ + "outputs": map[string]any{ + "id": map[string]any{"type": "string", "description": "Item ID"}, + "found": map[string]any{"type": "boolean"}, + "score": map[string]any{}, // missing type → defaults to "any" + }, + } + got := extractPipelineOutputSchema(pipelineMap) + if got == nil { + t.Fatal("expected non-nil schema") + } + if got["id"] != "string" { + t.Errorf("id: want string, got %q", got["id"]) + } + if got["found"] != "boolean" { + t.Errorf("found: want boolean, got %q", got["found"]) + } + if got["score"] != "any" { + t.Errorf("score: want any, got %q", got["score"]) + } + }) + + t.Run("without outputs", func(t *testing.T) { + pipelineMap := map[string]any{"steps": []any{}} + got := extractPipelineOutputSchema(pipelineMap) + if got != nil { + t.Errorf("expected nil for pipeline without outputs, got %v", got) + } + }) +} diff --git a/cmd/wfctl/infra_state.go b/cmd/wfctl/infra_state.go index 15bb28e4..16a0ee61 100644 --- a/cmd/wfctl/infra_state.go +++ b/cmd/wfctl/infra_state.go @@ -181,11 +181,11 @@ func runInfraStateImport(args []string) error { // --- tfstate export --- type tfState struct { - Version int `json:"version"` - TerraformVersion string `json:"terraform_version"` - Serial int `json:"serial"` - Lineage string `json:"lineage"` - Outputs map[string]any `json:"outputs"` + Version int `json:"version"` + TerraformVersion string `json:"terraform_version"` + Serial int `json:"serial"` + Lineage string `json:"lineage"` + Outputs map[string]any `json:"outputs"` Resources []tfStateResource `json:"resources"` } @@ -304,11 +304,11 @@ func importFromPulumi(srcFile, stateDir string) error { var checkpoint struct { Latest struct { Resources []struct { - URN string `json:"urn"` - Type string `json:"type"` - ID string `json:"id"` - Inputs map[string]any `json:"inputs"` - Outputs map[string]any `json:"outputs"` + URN string `json:"urn"` + Type string `json:"type"` + ID string `json:"id"` + Inputs map[string]any `json:"inputs"` + Outputs map[string]any `json:"outputs"` } `json:"resources"` } `json:"latest"` } diff --git a/cmd/wfctl/main.go b/cmd/wfctl/main.go index c572987b..08442460 100644 --- a/cmd/wfctl/main.go +++ b/cmd/wfctl/main.go @@ -53,30 +53,30 @@ func isHelpRequested(err error) bool { // the runtime functions that are registered in the CLICommandRegistry service // and invoked by step.cli_invoke from within each command's pipeline. var commands = map[string]func([]string) error{ - "init": runInit, - "validate": runValidate, - "inspect": runInspect, - "run": runRun, - "plugin": runPlugin, - "pipeline": runPipeline, - "schema": runSchema, - "snippets": runSnippets, - "manifest": runManifest, - "migrate": runMigrate, - "build-ui": runBuildUI, - "ui": runUI, - "publish": runPublish, - "deploy": runDeploy, - "api": runAPI, - "diff": runDiff, - "template": runTemplate, - "contract": runContract, - "compat": runCompat, - "generate": runGenerate, - "git": runGit, - "registry": runRegistry, - "update": runUpdate, - "mcp": runMCP, + "init": runInit, + "validate": runValidate, + "inspect": runInspect, + "run": runRun, + "plugin": runPlugin, + "pipeline": runPipeline, + "schema": runSchema, + "snippets": runSnippets, + "manifest": runManifest, + "migrate": runMigrate, + "build-ui": runBuildUI, + "ui": runUI, + "publish": runPublish, + "deploy": runDeploy, + "api": runAPI, + "diff": runDiff, + "template": runTemplate, + "contract": runContract, + "compat": runCompat, + "generate": runGenerate, + "git": runGit, + "registry": runRegistry, + "update": runUpdate, + "mcp": runMCP, "modernize": runModernize, "infra": runInfra, "docs": runDocs, diff --git a/cmd/wfctl/plugin_install_new_test.go b/cmd/wfctl/plugin_install_new_test.go index f291f580..cf9d4da0 100644 --- a/cmd/wfctl/plugin_install_new_test.go +++ b/cmd/wfctl/plugin_install_new_test.go @@ -24,8 +24,8 @@ func buildPluginTarGz(t *testing.T, pluginName string, binaryContent []byte, pjC t.Helper() topDir := pluginName + "-" + runtime.GOOS + "-" + runtime.GOARCH entries := map[string][]byte{ - topDir + "/" + pluginName: binaryContent, - topDir + "/plugin.json": pjContent, + topDir + "/" + pluginName: binaryContent, + topDir + "/plugin.json": pjContent, } return buildTarGz(t, entries, 0755) } @@ -166,8 +166,8 @@ func TestInstallFromURL_NameNormalization(t *testing.T) { pjContent := minimalPluginJSON(fullName, "0.1.0") entries := map[string][]byte{ - "top/" + fullName: []byte("#!/bin/sh\n"), - "top/plugin.json": pjContent, + "top/" + fullName: []byte("#!/bin/sh\n"), + "top/plugin.json": pjContent, } tarball := buildTarGz(t, entries, 0755) diff --git a/cmd/wfctl/template_validate.go b/cmd/wfctl/template_validate.go index 97bf929d..defe6899 100644 --- a/cmd/wfctl/template_validate.go +++ b/cmd/wfctl/template_validate.go @@ -568,6 +568,9 @@ var templateExprRe = regexp.MustCompile(`\{\{(.*?)\}\}`) // stepRefDotRe matches .steps.STEP_NAME patterns (dot access). var stepRefDotRe = regexp.MustCompile(`\.steps\.([a-zA-Z_][a-zA-Z0-9_-]*)`) +// stepFieldDotRe matches .steps.STEP_NAME.FIELD_NAME (captures step and first field). +var stepFieldDotRe = regexp.MustCompile(`\.steps\.([a-zA-Z_][a-zA-Z0-9_-]*)\.([a-zA-Z_][a-zA-Z0-9_-]*)`) + // stepRefIndexRe matches index .steps "STEP_NAME" patterns. var stepRefIndexRe = regexp.MustCompile(`index\s+\.steps\s+"([^"]+)"`) @@ -575,26 +578,45 @@ var stepRefIndexRe = regexp.MustCompile(`index\s+\.steps\s+"([^"]+)"`) // action, after a pipe, or after an opening parenthesis. var stepRefFuncRe = regexp.MustCompile(`(?:^|\||\()\s*step\s+"([^"]+)"`) +// stepFuncFieldRe matches step "STEP_NAME" "FIELD_NAME" capturing both arguments. +var stepFuncFieldRe = regexp.MustCompile(`step\s+"([^"]+)"\s+"([^"]+)"`) + // 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_-]*)*`) +// pipelineStepMeta holds the type and config of a pipeline step for static analysis. +type pipelineStepMeta struct { + typ string + config map[string]any +} + // validatePipelineTemplates checks template expressions in pipeline step configs for // references to nonexistent or forward-declared steps and common template pitfalls. +// It also warns when a template references a field that is not in the step type's +// declared output schema (Phase 1 static analysis). func validatePipelineTemplates(pipelineName string, stepsRaw []any, result *templateValidationResult) { - // Build ordered step name list - stepNames := make(map[string]int) // step name -> index in pipeline + // Build ordered step name list and step metadata for schema validation. + stepNames := make(map[string]int) // step name -> index in pipeline + stepMeta := make(map[string]pipelineStepMeta) // step name -> type+config + for i, stepRaw := range stepsRaw { stepMap, ok := stepRaw.(map[string]any) if !ok { continue } name, _ := stepMap["name"].(string) - if name != "" { - stepNames[name] = i + if name == "" { + continue } + stepNames[name] = i + typ, _ := stepMap["type"].(string) + cfg, _ := stepMap["config"].(map[string]any) + stepMeta[name] = pipelineStepMeta{typ: typ, config: cfg} } + reg := schema.NewStepSchemaRegistry() + // Check each step's config for template expressions for i, stepRaw := range stepsRaw { stepMap, ok := stepRaw.(map[string]any) @@ -631,6 +653,13 @@ func validatePipelineTemplates(pipelineName string, stepsRaw []any, result *temp validateStepRef(pipelineName, stepName, refName, i, stepNames, result) } + // Check for step output field references via dot-access (.steps.NAME.FIELD) + fieldDotMatches := stepFieldDotRe.FindAllStringSubmatch(actionContent, -1) + for _, m := range fieldDotMatches { + refStepName, refField := m[1], m[2] + validateStepOutputField(pipelineName, stepName, refStepName, refField, stepMeta, reg, result) + } + // Check for step name references via index indexMatches := stepRefIndexRe.FindAllStringSubmatch(actionContent, -1) for _, m := range indexMatches { @@ -645,6 +674,13 @@ func validatePipelineTemplates(pipelineName string, stepsRaw []any, result *temp validateStepRef(pipelineName, stepName, refName, i, stepNames, result) } + // Check for step output field references via step function (step "NAME" "FIELD") + funcFieldMatches := stepFuncFieldRe.FindAllStringSubmatch(actionContent, -1) + for _, m := range funcFieldMatches { + refStepName, refField := m[1], m[2] + validateStepOutputField(pipelineName, stepName, refStepName, refField, stepMeta, reg, result) + } + // Warn on hyphenated dot-access (auto-fixed but suggest preferred syntax) if hyphenDotRe.MatchString(actionContent) { result.Warnings = append(result.Warnings, @@ -655,6 +691,44 @@ func validatePipelineTemplates(pipelineName string, stepsRaw []any, result *temp } } +// validateStepOutputField checks that a referenced output field exists in the +// step type's declared output schema. It emits a warning when the step type is +// known, has declared outputs, and none of them match the referenced field. +func validateStepOutputField(pipelineName, currentStep, refStepName, refField string, stepMeta map[string]pipelineStepMeta, reg *schema.StepSchemaRegistry, result *templateValidationResult) { + meta, ok := stepMeta[refStepName] + if !ok || meta.typ == "" { + return // step name unknown or no type — already caught by validateStepRef + } + + outputs := reg.InferStepOutputs(meta.typ, meta.config) + if len(outputs) == 0 { + return // no declared outputs for this step type — nothing to check + } + + // If any output key is a placeholder (wrapped in parentheses), the step + // has dynamic/config-dependent outputs and we cannot validate statically. + for _, o := range outputs { + if len(o.Key) > 1 && o.Key[0] == '(' && o.Key[len(o.Key)-1] == ')' { + return + } + } + + for _, o := range outputs { + if o.Key == refField { + return // field found — all good + } + } + + // Build a suggestion list from declared outputs + keys := make([]string, 0, len(outputs)) + for _, o := range outputs { + keys = append(keys, o.Key) + } + result.Warnings = append(result.Warnings, + fmt.Sprintf("pipeline %q step %q: references %s.%s but step %q (%s) declares outputs: %s", + pipelineName, currentStep, refStepName, refField, refStepName, meta.typ, strings.Join(keys, ", "))) +} + // 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) { diff --git a/cmd/wfctl/template_validate_test.go b/cmd/wfctl/template_validate_test.go index 049c5dff..85241751 100644 --- a/cmd/wfctl/template_validate_test.go +++ b/cmd/wfctl/template_validate_test.go @@ -532,27 +532,32 @@ func TestValidateConfigWithSelfReference(t *testing.T) { } } -func TestValidateConfigWithHyphenDotAccess(t *testing.T) { +// TestValidateStepOutputField_UndeclaredField_Warning checks that the validator +// warns when a template references a field that is not in the step type's +// declared output schema (Phase 1 static analysis). +func TestValidateStepOutputField_UndeclaredField_Warning(t *testing.T) { + // step.db_query with mode:single declares outputs: row, found + // Referencing .steps.query.rows (plural) should warn cfg := &config.WorkflowConfig{ Pipelines: map[string]any{ "api": map[string]any{ "steps": []any{ map[string]any{ - "name": "my-step", - "type": "step.set", + "name": "query", + "type": "step.db_query", "config": map[string]any{ - "values": map[string]any{ - "x": "hello", - }, + "db": "mydb", + "query": "SELECT * FROM items WHERE id = 1", + "mode": "single", }, }, map[string]any{ - "name": "consumer", - "type": "step.set", + "name": "respond", + "type": "step.json_response", "config": map[string]any{ - "values": map[string]any{ - "y": "{{ .steps.my-step.field }}", - }, + "status": 200, + // "rows" is the list-mode output; single-mode declares "row" + "body": `{{ json .steps.query.rows }}`, }, }, }, @@ -567,12 +572,265 @@ func TestValidateConfigWithHyphenDotAccess(t *testing.T) { found := false for _, w := range result.Warnings { - if strings.Contains(w, "hyphenated dot-access") && strings.Contains(w, "prefer") { + if strings.Contains(w, "rows") && strings.Contains(w, "query") { found = true break } } if !found { - t.Errorf("expected informational warning about hyphenated dot-access, got warnings: %v", result.Warnings) + t.Errorf("expected warning for undeclared field 'rows' on step.db_query (single mode), got warnings: %v", result.Warnings) + } +} + +// TestValidateStepOutputField_DeclaredField_NoWarning checks that referencing a +// declared output field does not produce a warning. +func TestValidateStepOutputField_DeclaredField_NoWarning(t *testing.T) { + // step.db_query with mode:single declares outputs: row, found + // Referencing .steps.query.row should NOT warn + cfg := &config.WorkflowConfig{ + Pipelines: map[string]any{ + "api": map[string]any{ + "steps": []any{ + map[string]any{ + "name": "query", + "type": "step.db_query", + "config": map[string]any{ + "db": "mydb", + "query": "SELECT * FROM items WHERE id = 1", + "mode": "single", + }, + }, + map[string]any{ + "name": "respond", + "type": "step.json_response", + "config": map[string]any{ + "status": 200, + "body": `{{ json .steps.query.row }}`, + }, + }, + }, + }, + }, + } + knownModules := KnownModuleTypes() + knownSteps := KnownStepTypes() + knownTriggers := KnownTriggerTypes() + + result := validateWorkflowConfig("test", cfg, knownModules, knownSteps, knownTriggers) + + for _, w := range result.Warnings { + if strings.Contains(w, "query.row") && strings.Contains(w, "declares outputs") { + t.Errorf("unexpected warning for declared field 'row': %s", w) + } + } +} + +// TestValidateStepOutputField_StepFuncSyntax_Warning verifies that the validator +// also checks field names in the step "NAME" "FIELD" function call syntax. +func TestValidateStepOutputField_StepFuncSyntax_Warning(t *testing.T) { + // step.db_query with mode:list declares outputs: rows, count + // Using step "query" "row" (singular) should warn + cfg := &config.WorkflowConfig{ + Pipelines: map[string]any{ + "api": map[string]any{ + "steps": []any{ + map[string]any{ + "name": "query", + "type": "step.db_query", + "config": map[string]any{ + "mode": "list", + }, + }, + map[string]any{ + "name": "respond", + "type": "step.json_response", + "config": map[string]any{ + "status": 200, + // list mode declares "rows"/"count", not "row" + "body": `{{ step "query" "row" }}`, + }, + }, + }, + }, + }, + } + knownModules := KnownModuleTypes() + knownSteps := KnownStepTypes() + knownTriggers := KnownTriggerTypes() + + result := validateWorkflowConfig("test", cfg, knownModules, knownSteps, knownTriggers) + + found := false + for _, w := range result.Warnings { + if strings.Contains(w, "row") && strings.Contains(w, "query") { + found = true + break + } + } + if !found { + t.Errorf("expected warning for undeclared field 'row' on step.db_query (list mode) via step func, got warnings: %v", result.Warnings) + } +} + +// TestValidateStepOutputField_SetStep_NoWarning checks that step.set outputs +// inferred from config are validated correctly (declared key should not warn). +func TestValidateStepOutputField_SetStep_NoWarning(t *testing.T) { + cfg := &config.WorkflowConfig{ + Pipelines: map[string]any{ + "api": map[string]any{ + "steps": []any{ + map[string]any{ + "name": "setter", + "type": "step.set", + "config": map[string]any{ + "values": map[string]any{ + "user_id": "123", + }, + }, + }, + map[string]any{ + "name": "respond", + "type": "step.json_response", + "config": map[string]any{ + "status": 200, + "body": `{{ .steps.setter.user_id }}`, + }, + }, + }, + }, + }, + } + knownModules := KnownModuleTypes() + knownSteps := KnownStepTypes() + knownTriggers := KnownTriggerTypes() + + result := validateWorkflowConfig("test", cfg, knownModules, knownSteps, knownTriggers) + + for _, w := range result.Warnings { + if strings.Contains(w, "declares outputs") && strings.Contains(w, "setter") { + t.Errorf("unexpected step output warning for step.set with declared key: %s", w) + } + } +} + +// TestValidateStepOutputField_NoOutputSchema_NoWarning checks that steps with +// no declared outputs do not produce false-positive field warnings. +func TestValidateStepOutputField_NoOutputSchema_NoWarning(t *testing.T) { + // An unknown/external step type has no schema — any field reference should be silently ignored + cfg := &config.WorkflowConfig{ + Pipelines: map[string]any{ + "api": map[string]any{ + "steps": []any{ + map[string]any{ + "name": "external", + "type": "step.external_custom_step", // not in registry + }, + map[string]any{ + "name": "respond", + "type": "step.json_response", + "config": map[string]any{ + "status": 200, + "body": `{{ .steps.external.anything }}`, + }, + }, + }, + }, + }, + } + knownModules := KnownModuleTypes() + knownSteps := KnownStepTypes() + knownTriggers := KnownTriggerTypes() + + result := validateWorkflowConfig("test", cfg, knownModules, knownSteps, knownTriggers) + + for _, w := range result.Warnings { + if strings.Contains(w, "declares outputs") && strings.Contains(w, "external") { + t.Errorf("unexpected output field warning for step with no declared outputs: %s", w) + } + } +} + +// TestValidateStepOutputFieldRegistry tests validateStepOutputField directly. +func TestValidateStepOutputFieldRegistry(t *testing.T) { + reg := schema.NewStepSchemaRegistry() + + tests := []struct { + name string + stepName string + stepType string + stepConfig map[string]any + refField string + expectWarn bool + }{ + { + name: "db_query single mode: valid field row", + stepName: "q", + stepType: "step.db_query", + stepConfig: map[string]any{"mode": "single"}, + refField: "row", + expectWarn: false, + }, + { + name: "db_query single mode: invalid field rows", + stepName: "q", + stepType: "step.db_query", + stepConfig: map[string]any{"mode": "single"}, + refField: "rows", + expectWarn: true, + }, + { + name: "db_query list mode: valid field rows", + stepName: "q", + stepType: "step.db_query", + stepConfig: map[string]any{"mode": "list"}, + refField: "rows", + expectWarn: false, + }, + { + name: "db_query list mode: invalid field row", + stepName: "q", + stepType: "step.db_query", + stepConfig: map[string]any{"mode": "list"}, + refField: "row", + expectWarn: true, + }, + { + name: "step.set: declared key is valid", + stepName: "s", + stepType: "step.set", + stepConfig: map[string]any{"values": map[string]any{"my_field": "v"}}, + refField: "my_field", + expectWarn: false, + }, + { + name: "step.set: undeclared key warns", + stepName: "s", + stepType: "step.set", + stepConfig: map[string]any{"values": map[string]any{"my_field": "v"}}, + refField: "other_field", + expectWarn: true, + }, + { + name: "unknown step type: no warning", + stepName: "s", + stepType: "step.nonexistent", + stepConfig: nil, + refField: "anything", + expectWarn: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + result := &templateValidationResult{} + stepMeta := map[string]pipelineStepMeta{ + tc.stepName: {typ: tc.stepType, config: tc.stepConfig}, + } + validateStepOutputField("pipeline", "current-step", tc.stepName, tc.refField, stepMeta, reg, result) + hasWarn := len(result.Warnings) > 0 + if hasWarn != tc.expectWarn { + t.Errorf("expectWarn=%v but warnings=%v", tc.expectWarn, result.Warnings) + } + }) } } diff --git a/cmd/wfctl/test.go b/cmd/wfctl/test.go index 6be1ed6f..3128d071 100644 --- a/cmd/wfctl/test.go +++ b/cmd/wfctl/test.go @@ -245,9 +245,9 @@ type testMockConfig struct { } type testCase struct { - Description string `yaml:"description"` - Trigger testTriggerDef `yaml:"trigger"` - StopAfter string `yaml:"stop_after"` + Description string `yaml:"description"` + Trigger testTriggerDef `yaml:"trigger"` + StopAfter string `yaml:"stop_after"` Mocks *testMockConfig `yaml:"mocks"` Assertions []testAssertion `yaml:"assertions"` } @@ -262,9 +262,9 @@ type testTriggerDef struct { } type testAssertion struct { - Step string `yaml:"step"` - Output map[string]any `yaml:"output"` - Executed *bool `yaml:"executed"` + Step string `yaml:"step"` + Output map[string]any `yaml:"output"` + Executed *bool `yaml:"executed"` Response *testResponseAssert `yaml:"response"` } diff --git a/config/pipeline.go b/config/pipeline.go index c014bfc5..8b4b3c17 100644 --- a/config/pipeline.go +++ b/config/pipeline.go @@ -1,5 +1,14 @@ package config +// PipelineOutputDef describes a single declared output field of a pipeline. +// When a pipeline declares its outputs, callers (HTTP triggers, +// step.workflow_call, wfctl contract test) can validate or document the +// expected response shape. +type PipelineOutputDef struct { + Type string `json:"type" yaml:"type"` + Description string `json:"description,omitempty" yaml:"description,omitempty"` +} + // PipelineConfig represents a single composable pipeline definition. type PipelineConfig struct { Trigger PipelineTriggerConfig `json:"trigger" yaml:"trigger"` @@ -7,6 +16,13 @@ type PipelineConfig struct { OnError string `json:"on_error,omitempty" yaml:"on_error,omitempty"` Timeout string `json:"timeout,omitempty" yaml:"timeout,omitempty"` Compensation []PipelineStepConfig `json:"compensation,omitempty" yaml:"compensation,omitempty"` + // Outputs declares the named output fields that this pipeline is expected + // to produce. This is an optional, backwards-compatible declaration — + // existing pipelines without an Outputs block continue to work unchanged. + // When present, it enables: + // - wfctl contract test to include response schemas in endpoint contracts + // - wfctl template validate to validate output_mapping in step.workflow_call + Outputs map[string]PipelineOutputDef `json:"outputs,omitempty" yaml:"outputs,omitempty"` } // PipelineTriggerConfig defines what starts a pipeline. From adb2e2797363eaf81d499473da548c6e9553d604 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 24 Mar 2026 21:36:25 +0000 Subject: [PATCH 3/4] fix: address PR review comments on template_validate and pipeline.go - Tighten stepFuncFieldRe to require (^|pipe|paren) prefix, avoiding false matches - Skip stepFieldDotRe field validation when hyphenDotRe fires (prevents double/misleading warnings on hyphenated dot-access) - Update PipelineConfig.Outputs comment to accurately reflect current behavior - Restore TestValidateConfigWithHyphenDotAccess test that was accidentally removed Co-authored-by: intel352 <77607+intel352@users.noreply.github.com> Agent-Logs-Url: https://github.com/GoCodeAlone/workflow/sessions/2e2b3ec2-d5d6-4637-a546-08f6a7dbd72e --- cmd/wfctl/template_validate.go | 18 ++++++++---- cmd/wfctl/template_validate_test.go | 45 +++++++++++++++++++++++++++++ config/pipeline.go | 3 +- 3 files changed, 59 insertions(+), 7 deletions(-) diff --git a/cmd/wfctl/template_validate.go b/cmd/wfctl/template_validate.go index defe6899..4341b988 100644 --- a/cmd/wfctl/template_validate.go +++ b/cmd/wfctl/template_validate.go @@ -578,8 +578,10 @@ var stepRefIndexRe = regexp.MustCompile(`index\s+\.steps\s+"([^"]+)"`) // action, after a pipe, or after an opening parenthesis. var stepRefFuncRe = regexp.MustCompile(`(?:^|\||\()\s*step\s+"([^"]+)"`) -// stepFuncFieldRe matches step "STEP_NAME" "FIELD_NAME" capturing both arguments. -var stepFuncFieldRe = regexp.MustCompile(`step\s+"([^"]+)"\s+"([^"]+)"`) +// stepFuncFieldRe matches step "STEP_NAME" "FIELD_NAME" capturing both arguments, +// when used as a function call at the start of an action, after a pipe, or after +// an opening parenthesis. +var stepFuncFieldRe = regexp.MustCompile(`(?:^|\||\()\s*step\s+"([^"]+)"\s+"([^"]+)"`) // hyphenDotRe matches dot-access chains with hyphens (e.g., .steps.my-step.field), // including continuation segments after the hyphenated part. @@ -654,10 +656,14 @@ func validatePipelineTemplates(pipelineName string, stepsRaw []any, result *temp } // Check for step output field references via dot-access (.steps.NAME.FIELD) - fieldDotMatches := stepFieldDotRe.FindAllStringSubmatch(actionContent, -1) - for _, m := range fieldDotMatches { - refStepName, refField := m[1], m[2] - validateStepOutputField(pipelineName, stepName, refStepName, refField, stepMeta, reg, result) + // Skip when the action contains hyphenated dot-access, which is not valid + // Go-template syntax and is already flagged separately by hyphenDotRe. + if !hyphenDotRe.MatchString(actionContent) { + fieldDotMatches := stepFieldDotRe.FindAllStringSubmatch(actionContent, -1) + for _, m := range fieldDotMatches { + refStepName, refField := m[1], m[2] + validateStepOutputField(pipelineName, stepName, refStepName, refField, stepMeta, reg, result) + } } // Check for step name references via index diff --git a/cmd/wfctl/template_validate_test.go b/cmd/wfctl/template_validate_test.go index 85241751..32cbc841 100644 --- a/cmd/wfctl/template_validate_test.go +++ b/cmd/wfctl/template_validate_test.go @@ -532,6 +532,51 @@ func TestValidateConfigWithSelfReference(t *testing.T) { } } +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) + } +} + // TestValidateStepOutputField_UndeclaredField_Warning checks that the validator // warns when a template references a field that is not in the step type's // declared output schema (Phase 1 static analysis). diff --git a/config/pipeline.go b/config/pipeline.go index 8b4b3c17..20044fd8 100644 --- a/config/pipeline.go +++ b/config/pipeline.go @@ -21,7 +21,8 @@ type PipelineConfig struct { // existing pipelines without an Outputs block continue to work unchanged. // When present, it enables: // - wfctl contract test to include response schemas in endpoint contracts - // - wfctl template validate to validate output_mapping in step.workflow_call + // - tools that understand pipeline outputs (for example, step.workflow_call) + // to validate or document output_mapping and other response shapes Outputs map[string]PipelineOutputDef `json:"outputs,omitempty" yaml:"outputs,omitempty"` } From 6ed8e86cb60836d45f69ba3ab62ecc0d6d2652e1 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 24 Mar 2026 22:17:28 +0000 Subject: [PATCH 4/4] fix: remove duplicate reg declaration and fix mixed indentation in validatePipelineTemplates The previous commit introduced a duplicate `reg := schema.NewStepSchemaRegistry()` declaration causing a compile error ("no new variables on left side of :="). Also fixes mixed spaces/tabs indentation in the same function block. Co-authored-by: intel352 <77607+intel352@users.noreply.github.com> Agent-Logs-Url: https://github.com/GoCodeAlone/workflow/sessions/d45a738a-055c-418e-825c-fce5c1de3dc5 --- cmd/wfctl/template_validate.go | 19 ++++++++----------- cmd/wfctl/template_validate_test.go | 1 - module/pipeline_step_deploy_rolling.go | 1 - 3 files changed, 8 insertions(+), 13 deletions(-) diff --git a/cmd/wfctl/template_validate.go b/cmd/wfctl/template_validate.go index cb20b943..18f0a997 100644 --- a/cmd/wfctl/template_validate.go +++ b/cmd/wfctl/template_validate.go @@ -653,8 +653,8 @@ func hasDynamicOutputs(outputs []schema.InferredOutput) bool { func validatePipelineTemplates(pipelineName string, stepsRaw []any, result *templateValidationResult) { // Build ordered step name list and step metadata for schema validation. stepNames := make(map[string]int) // step name -> index in pipeline - stepMeta := make(map[string]pipelineStepMeta) // step name -> type+config - stepInfos := make(map[string]stepBuildInfo) // step name -> type and config + stepMeta := make(map[string]pipelineStepMeta) // step name -> type+config (used by validateStepOutputField) + stepInfos := make(map[string]stepBuildInfo) // step name -> type and config (used by validateStepRef) reg := schema.NewStepSchemaRegistry() @@ -666,22 +666,19 @@ func validatePipelineTemplates(pipelineName string, stepsRaw []any, result *temp name, _ := stepMap["name"].(string) if name == "" { continue - } - + } + stepNames[name] = i typ, _ := stepMap["type"].(string) cfg, _ := stepMap["config"].(map[string]any) + if cfg == nil { + cfg = map[string]any{} + } - if cfg == nil { - cfg = map[string]any{} - } - - stepInfos[name] = stepBuildInfo{stepType: typ, stepConfig: cfg} + stepInfos[name] = stepBuildInfo{stepType: typ, stepConfig: cfg} stepMeta[name] = pipelineStepMeta{typ: typ, config: cfg} } - reg := schema.NewStepSchemaRegistry() - // Check each step's config for template expressions for i, stepRaw := range stepsRaw { stepMap, ok := stepRaw.(map[string]any) diff --git a/cmd/wfctl/template_validate_test.go b/cmd/wfctl/template_validate_test.go index 30d00856..56a817d9 100644 --- a/cmd/wfctl/template_validate_test.go +++ b/cmd/wfctl/template_validate_test.go @@ -880,7 +880,6 @@ func TestValidateStepOutputFieldRegistry(t *testing.T) { } } - // --- Output field name validation tests --- // TestValidateStepOutputField_KnownField checks that a reference to a known output diff --git a/module/pipeline_step_deploy_rolling.go b/module/pipeline_step_deploy_rolling.go index f7ae122d..8d889819 100644 --- a/module/pipeline_step_deploy_rolling.go +++ b/module/pipeline_step_deploy_rolling.go @@ -242,4 +242,3 @@ func (s *DeployRollingStep) Execute(ctx context.Context, _ *PipelineContext) (*S "batches": totalBatches, }}, nil } -