Conversation
Co-authored-by: intel352 <77607+intel352@users.noreply.github.com> Agent-Logs-Url: https://github.com/GoCodeAlone/workflow/sessions/793db56c-189a-4e30-b1c0-47f9fcc38e6a
Co-authored-by: intel352 <77607+intel352@users.noreply.github.com> Agent-Logs-Url: https://github.com/GoCodeAlone/workflow/sessions/793db56c-189a-4e30-b1c0-47f9fcc38e6a
Copilot
AI
changed the title
[WIP] Implement engine runtime validation for wfctl cross-reference checks
Engine runtime validation: reuse wfctl cross-reference checks at startup
Mar 24, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR factors pipeline template cross-reference validation out of wfctl into a shared validation package, and runs that same validation during engine startup (BuildFromConfig) with configurable strictness via engine.validation.templateRefs.
Changes:
- Added
validation/package for pipeline template reference checks and SQL column extraction. - Added
engine.validation.templateRefsconfig and wired startup validation intoStdEngine.BuildFromConfig(). - Updated
wfctlto delegate to the shared validation package; added/updated tests and documentation.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
validation/pipeline_refs.go |
New shared implementation of template cross-reference validation + SQL column extraction. |
validation/pipeline_refs_test.go |
Unit tests covering reference checks and SQL column extraction. |
engine.go |
Runs template ref validation at startup with off/warn/error modes. |
config/config.go |
Adds EngineConfig / EngineValidationConfig to WorkflowConfig. |
engine_validation_test.go |
Tests engine startup behavior across validation modes. |
cmd/wfctl/template_validate.go |
Replaces in-file validation logic with calls into validation package. |
cmd/wfctl/template_validate_test.go |
Updates tests to validate via the shared validation package. |
cmd/wfctl/api_extract.go |
Uses validation.ExtractSQLColumns instead of a local duplicate. |
DOCUMENTATION.md |
Documents the new engine.validation.templateRefs setting. |
| mode := "warn" // default | ||
| if cfg.Engine != nil && cfg.Engine.Validation != nil && cfg.Engine.Validation.TemplateRefs != "" { | ||
| mode = cfg.Engine.Validation.TemplateRefs | ||
| } |
There was a problem hiding this comment.
engine.validation.templateRefs accepts only "off" | "warn" | "error", but BuildFromConfig treats any other value as "warn" (since only "off" and "error" are handled specially). This makes typos silently change behavior. Consider validating the value explicitly and either returning a config error (preferred) or logging a warning and falling back to the default.
Suggested change
| } | |
| } | |
| switch mode { | |
| case "off", "warn", "error": | |
| // valid | |
| default: | |
| return fmt.Errorf("invalid engine.validation.templateRefs value %q, must be one of: off, warn, error", mode) | |
| } |
Comment on lines
+109
to
+113
| stepInfos := make(map[string]stepBuildInfo) // step name -> type and config (used by validateStepRef) | ||
|
|
||
| reg := schema.NewStepSchemaRegistry() | ||
|
|
||
| for i, stepRaw := range stepsRaw { |
There was a problem hiding this comment.
validatePipelineTemplateRefs creates a fresh schema.NewStepSchemaRegistry() for every pipeline. This both rebuilds built-in schemas repeatedly and (more importantly) ignores any step schemas registered by loaded plugins (the engine’s PluginLoader().StepSchemaRegistry()), so output-field validation won’t run for plugin step types even when schemas are available. Consider accepting a *schema.StepSchemaRegistry as an input (or otherwise wiring the engine/plugin loader registry through) and reusing a single instance across all pipelines.
Comment on lines
+163
to
+172
| // Check for step name references via dot-access (captures optional field path) | ||
| dotMatches := stepRefDotRe.FindAllStringSubmatch(actionContent, -1) | ||
| for _, m := range dotMatches { | ||
| refName := m[1] | ||
| fieldPath := "" | ||
| if len(m) > 2 { | ||
| fieldPath = m[2] | ||
| } | ||
| validateStepRef(pipelineName, stepName, refName, fieldPath, i, stepNames, stepInfos, reg, result) | ||
| } |
There was a problem hiding this comment.
When actionContent contains a hyphenated dot-access segment (matched by hyphenDotRe), stepRefDotRe can still partially match and pass a truncated fieldPath into validateStepRef, which can produce misleading extra warnings (e.g. spurious unknown output / SQL-column warnings) in addition to the intended “hyphenated dot-access” warning. Consider skipping field-path validation for dot-matches when hyphenDotRe matches the action (still validating that the referenced step exists/ordering), so the linter emits only the relevant hyphen guidance.
⏱ Benchmark Results✅ No significant performance regressions detected. benchstat comparison (baseline → PR)
|
Contributor
|
@copilot apply changes based on the comments in this thread |
…cture, accept registry param, fix hyphen spurious warnings Co-authored-by: intel352 <77607+intel352@users.noreply.github.com> Agent-Logs-Url: https://github.com/GoCodeAlone/workflow/sessions/b13a8553-fb55-4892-a6e4-21e323b5cb20
Contributor
Author
Applied all five review comments in commit
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Not every team runs
wfctl validatein CI — the engine should protect itself. This extracts the pipeline template cross-reference logic fromwfctlinto a sharedvalidationpackage and wires it intoBuildFromConfig()with configurable strictness.Changes
New
validation/packageValidatePipelineTemplateRefs(pipelines map[string]any) *RefValidationResult— shared validation callable by bothwfctland the enginestep.db_queryExtractSQLColumns()moved here fromcmd/wfctl/api_extract.go(was duplicated in two places)config/config.goNew
Engine *EngineConfigfield onWorkflowConfig:engine.go— startup validation inBuildFromConfig()warn(default): logs warnings, engine starts — backwards compatible, no functional change for existing configserror: returns error and refuses to start if validation finds issuesoff: skips entirely, exact prior behaviourcmd/wfctl/template_validate.go+api_extract.govalidationpackageExample
Original prompt
This section details on the original issue you should resolve
<issue_title>Engine runtime validation: reuse wfctl cross-reference checks at startup and execution time</issue_title>
<issue_description>## Summary
Follow-up to #368 (wfctl static cross-reference validation) and #374 (output type enforcement).
#368 proposes deep template reference validation in
wfctl validate— checking field-level references against step output schemas, SQL alias extraction, and cross-file import validation. That same validation logic should also be available inside the engine itself, so that problems are caught automatically at startup or first execution without requiring a separatewfctl validateCI step.Motivation
wfctl validatein CI — the engine should protect itselfwfctl validatecould still fail at runtime due to environment-specific differences (env var expansion, imported file resolution)Proposed behavior
Phase 1: Startup validation (config load time)
When the engine loads config via
BuildFromConfig(), run the same checks thatwfctl template validateperforms:steps.query.rowreferences a field declared instep.db_query's output schema (proposed in wfctl validate: no cross-reference checking for DB columns, step output fields, or imported configs #368)steps.query.rowson amode: singledb_queryThis should be warnings by default to maintain backwards compatibility:
With
warn(default): log warnings at startup for any suspicious references, but allow the engine to start. Witherror: refuse to start if validation fails. Withoff: skip entirely (current behavior).Phase 2: First-execution validation (runtime)
Some references can only be validated when a pipeline actually runs — for example, when step outputs are known. On first execution of each pipeline, the engine could:
StepSchema.OutputsThis provides a runtime safety net that catches issues static analysis cannot:
Implementation: share validation logic
The key principle is one validation library, two consumers:
The validation logic proposed in #368 (deep template ref checking, output schema lookup, SQL alias extraction) should live in a shared package (e.g.,
validation/or withinconfig/) that bothwfctland the engine import. This avoids duplicating logic and ensures consistency.Backwards compatibility
warn) only adds log output — no functional changeoffpreserves exact current behaviorerroris opt-in for teams that want strict validationReferences
💬 Send tasks to Copilot coding agent from Slack and Teams to turn conversations into code. Copilot posts an update in your thread when it's finished.