Conversation
…nalysis - 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
There was a problem hiding this comment.
Pull request overview
Adds opt-in pipeline output schema declarations and extends wfctl tooling to (a) statically warn when templates reference undeclared step output fields and (b) enrich/compare HTTP endpoint contracts with declared response schemas.
Changes:
- Add
outputs:declarations toPipelineConfigviaPipelineOutputDeffor pipeline response contracts. - Enhance
wfctl template validatewith step-output-field static analysis usingschema.StepSchemaRegistry.InferStepOutputs. - Extend
wfctl contractto recordresponseSchemaand treat response field removals/type changes as breaking.
Reviewed changes
Copilot reviewed 5 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
config/pipeline.go |
Adds opt-in pipeline Outputs contract metadata (PipelineOutputDef). |
cmd/wfctl/template_validate.go |
Adds regex-based extraction of referenced step output fields and warns when not declared by the step schema. |
cmd/wfctl/template_validate_test.go |
Adds tests for the new step output field warnings/no-warnings behavior. |
cmd/wfctl/contract.go |
Adds ResponseSchema to endpoint contracts, populates from pipeline outputs, compares schema for breaking changes, prints schema. |
cmd/wfctl/contract_test.go |
Adds tests for response schema extraction and breaking-change detection. |
cmd/wfctl/test.go |
Formatting alignment only. |
cmd/wfctl/plugin_install_new_test.go |
Formatting alignment only. |
cmd/wfctl/main.go |
Formatting alignment only. |
cmd/wfctl/infra_state.go |
Formatting alignment only. |
| // 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) | ||
| } |
There was a problem hiding this comment.
Field validation via stepFieldDotRe runs even when the template uses hyphenated dot-access (e.g., .steps.my-step.field), which is not valid Go-template field access and is already flagged by hyphenDotRe. This can lead to extra, misleading output-field warnings on top of the hyphenated dot-access warning. Consider skipping stepFieldDotRe validation when hyphenDotRe matches the same actionContent, or otherwise ensuring field validation only runs on syntactically valid access forms.
| // 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{ |
There was a problem hiding this comment.
This change replaces the previous hyphenated dot-access validation test with step-output-field tests, leaving hyphenated dot-access behavior untested. Since hyphenDotRe warnings are still part of validatePipelineTemplates, consider restoring a dedicated test that asserts the hyphenated dot-access warning to prevent regressions.
|
@copilot apply changes based on the comments in this thread |
- 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
⏱ Benchmark Results✅ No significant performance regressions detected. benchstat comparison (baseline → PR)
|
All four review points addressed in adb2e27:
|
|
@copilot Address the failing test, linter, and build, regardless of pre-existing issues or not. |
…lidatePipelineTemplates
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
Fixed in 6ed8e86. The previous commit accidentally introduced a duplicate |
182 built-in steps declare output schemas that are never validated — not at config load, not at runtime, not by
wfctl validate. Pipelines also have no mechanism to declare their output contract, makingstep.workflow_callcallers and HTTP consumers blind to what they'll receive.Changes
config/pipeline.go— Pipeline output declarations (opt-in)PipelineOutputDef(type,description) andOutputs map[string]PipelineOutputDeftoPipelineConfigoutputs:are unchangedcmd/wfctl/template_validate.go— Phase 1 static analysisstepFieldDotRe/stepFuncFieldReregexes to extract the field name after a step reference (.steps.NAME.FIELDandstep "NAME" "FIELD")validatePipelineTemplatesnow tracks step type+config metadata for each pipeline stepvalidateStepOutputFieldusesschema.InferStepOutputsto warn when a referenced field isn't in the step type's declared output schema(key),(claims)) and steps with no declared outputs are silently skippedcmd/wfctl/contract.go— Contract enrichmentResponseSchema map[string]string(field → type) toEndpointContractgenerateContractpopulatesResponseSchemafrom the pipeline'soutputs:declaration when presentcompareContractsnow flags as breaking: response field removal, response field type changeprintContractrenders schema fields indented under each endpointOriginal prompt
This section details on the original issue you should resolve
<issue_title>Pipeline and step output type schemas are metadata-only — no runtime or static enforcement</issue_title>
<issue_description>## Summary
The engine has comprehensive type metadata for step outputs (182 built-in steps declare their output keys, types, and descriptions via
schema.StepOutputDef), but this metadata is never enforced — not at config load time, not at runtime, and not bywfctl validate. The schemas serve documentation purposes only.This is a separate concern from #367 (missingkey=zero for template resolution). Even if #367 is fixed, there is no mechanism to declare or validate the shape of data flowing between steps or returned from pipelines.
What exists today (metadata, not enforcement)
Every built-in step declares its outputs in
schema/step_schema_builtins.go:These are exposed via
wfctl get_step_schemaand MCP tooling. But at runtime,StepResult.Outputis alwaysmap[string]any— the engine never checks that the actual output matches the declared schema.What's missing
1. Pipeline-level output contract
PipelineConfighas nooutputs:field:A pipeline can return anything. Callers (HTTP triggers,
step.workflow_call, eventbus consumers) have no declared contract for what they'll receive.2. Step output validation
When a step completes, the engine merges its output into
PipelineContext.Currentwithout checking that the output matches the step type's declared schema:3. Cross-step type compatibility in wfctl
The validator checks that step names exist but never checks that referenced fields exist in the step type's declared output schema. The schema metadata is already there — it just isn't used.
Example:
{{.steps.query.row.slug}}— the validator could check:querystep exists ✅ (already done)queryisstep.db_query→ declared outputs includerow(type:map) — could do, not donerowcontainsslug— can't know (depends on SQL), but could warn it's unverifiableEven checking (2) would catch bugs like referencing
steps.query.rowswhen the step usesmode: single(which only outputsrow+found, notrows+count).4.
step.workflow_calloutput is(dynamic): anyCallers get no type contract from the called pipeline. If the called pipeline declared its output schema,
step.workflow_callcould validate thatoutput_mappingreferences valid fields.5.
wfctl contract testhas no response schemasEndpointContractcaptures method, path, pipeline name — but no request/response schema. Output field removals or type changes are invisible to contract diffing.Suggested implementation (backwards-compatible)
All proposals are opt-in — existing configs continue to work unchanged.
Phase 1: Static analysis with existing metadata (no config changes)
Use the existing
StepSchema.Outputsinwfctl template validateto warn when a template references a field not in the step type's declared output. This requires zero config changes — the metadata already exists for all 182 built-in steps.Phase 2: Optional pipeline output declarations
Add an optional
outputs:block to pipeline config. When absent, behavior is unchanged (fully backwards-compatible). When present, enables downstream validation.Benefits:
step.workflow_callcallers can validateoutput_mappingagainst the called pipeline's declared outputswfctl contract testcan include response schemas in endpoint contracts🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.