diff --git a/DOCUMENTATION.md b/DOCUMENTATION.md index 6afc055d..7a2bf857 100644 --- a/DOCUMENTATION.md +++ b/DOCUMENTATION.md @@ -429,8 +429,36 @@ value: '{{ step "parse-request" "path_params" "id" }}' 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. +#### Missing Key Behaviour +By default, template expressions that reference a missing map key (e.g. a typo in a step field name) resolve to the zero value silently — but the engine now logs a **WARN** message to make the problem visible: + +``` +WARN template resolved missing key to zero value pipeline=my-pipeline error="..." +``` + +To turn the warning into a hard error, set `strict_templates: true` on the pipeline: + +```yaml +pipelines: + my-pipeline: + strict_templates: true # any missing key access fails the pipeline step + steps: + - name: process + type: step.set + config: + values: + tenant: "{{ .steps.auth.affilate_id }}" # typo: affilate_id instead of affiliate_id → step fails immediately +``` + +| Mode | `strict_templates` | Missing key result | +|------|-------------------|--------------------| +| Default | `false` | Zero value (``) + WARN log | +| Strict | `true` | Step returns an error | + +Strict mode applies to **both** direct dot-access (`{{ .steps.auth.field }}`) and the `step`/`trigger` helper functions (`{{ step "auth" "field" }}`). A missing key via either syntax will fail the step when `strict_templates: true` is set. + +`wfctl template validate --config workflow.yaml` lints template expressions and warns on undefined step references and forward references. Use `strict_templates: true` in the pipeline config to catch field-level typos at runtime. ### Infrastructure | Type | Description | Plugin | |------|-------------|--------| 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/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..bf9d770e 100644 --- a/config/pipeline.go +++ b/config/pipeline.go @@ -7,6 +7,11 @@ 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"` + // StrictTemplates causes the pipeline to return an error when any template + // expression references a missing map key, instead of silently using the zero + // value. Useful for catching typos in step field references at runtime. + // Default is false (missing keys produce a warning log and resolve to zero). + StrictTemplates bool `json:"strict_templates,omitempty" yaml:"strict_templates,omitempty"` } // PipelineTriggerConfig defines what starts a pipeline. diff --git a/engine.go b/engine.go index 1b799934..c55f2a54 100644 --- a/engine.go +++ b/engine.go @@ -93,7 +93,6 @@ type StdEngine struct { // configHash is the SHA-256 hash of the last config built via BuildFromConfig. // Format: "sha256:". Empty until BuildFromConfig is called. configHash string - } // App returns the underlying modular.Application. @@ -142,7 +141,6 @@ func (e *StdEngine) SetPluginInstaller(installer *plugin.PluginInstaller) { e.pluginInstaller = installer } - // NewStdEngine creates a new workflow engine func NewStdEngine(app modular.Application, logger modular.Logger) *StdEngine { e := &StdEngine{ @@ -854,11 +852,12 @@ func (e *StdEngine) configurePipelines(pipelineCfg map[string]any) error { } pipeline := &module.Pipeline{ - Name: pipelineName, - Steps: steps, - OnError: onError, - Timeout: timeout, - Compensation: compSteps, + Name: pipelineName, + Steps: steps, + OnError: onError, + Timeout: timeout, + Compensation: compSteps, + StrictTemplates: pipeCfg.StrictTemplates, } // Propagate the engine's logger to the pipeline so that execution logs diff --git a/interfaces/iac_test.go b/interfaces/iac_test.go index 4c51e754..b1cc15d3 100644 --- a/interfaces/iac_test.go +++ b/interfaces/iac_test.go @@ -44,7 +44,7 @@ func (m *mockProvider) ResolveSizing(_ string, _ interfaces.Size, _ *interfaces. return nil, nil } func (m *mockProvider) ResourceDriver(_ string) (interfaces.ResourceDriver, error) { return nil, nil } -func (m *mockProvider) Close() error { return nil } +func (m *mockProvider) Close() error { return nil } // mockDriver implements ResourceDriver type mockDriver struct{} @@ -84,7 +84,7 @@ func (s *mockState) GetResource(_ context.Context, _ string) (*interfaces.Resour func (s *mockState) ListResources(_ context.Context) ([]interfaces.ResourceState, error) { return nil, nil } -func (s *mockState) DeleteResource(_ context.Context, _ string) error { return nil } +func (s *mockState) DeleteResource(_ context.Context, _ string) error { return nil } func (s *mockState) SavePlan(_ context.Context, _ interfaces.IaCPlan) error { return nil } func (s *mockState) GetPlan(_ context.Context, _ string) (*interfaces.IaCPlan, error) { return nil, nil diff --git a/interfaces/pipeline.go b/interfaces/pipeline.go index ac99312d..c3dabe52 100644 --- a/interfaces/pipeline.go +++ b/interfaces/pipeline.go @@ -57,6 +57,17 @@ type PipelineContext struct { // Metadata holds execution metadata (pipeline name, trace ID, etc.) Metadata map[string]any + + // StrictTemplates causes template execution to return an error instead of + // the zero value when a template expression references a missing map key. + // When false (the default), missing keys resolve to the zero value with a + // warning logged via Logger. Enable via pipeline config: strict_templates: true. + StrictTemplates bool + + // Logger is used to emit warnings when a template expression resolves a + // missing key to the zero value (non-strict mode). When nil, slog.Default() + // is used. + Logger *slog.Logger } // NewPipelineContext creates a PipelineContext initialized with trigger data. diff --git a/module/iac_state_azure_test.go b/module/iac_state_azure_test.go index 2236502e..0ed76190 100644 --- a/module/iac_state_azure_test.go +++ b/module/iac_state_azure_test.go @@ -13,9 +13,9 @@ import ( // mockAzureClient is an in-memory implementation of AzureBlobClient for testing. type mockAzureClient struct { - mu sync.Mutex - blobs map[string][]byte // name -> body - leases map[string]string // name -> leaseID (empty = not leased) + mu sync.Mutex + blobs map[string][]byte // name -> body + leases map[string]string // name -> leaseID (empty = not leased) } func newMockAzureClient() *mockAzureClient { diff --git a/module/iac_state_gcs_test.go b/module/iac_state_gcs_test.go index bc4c6265..5f9e1caa 100644 --- a/module/iac_state_gcs_test.go +++ b/module/iac_state_gcs_test.go @@ -14,9 +14,9 @@ import ( // mockGCSClient is an in-memory implementation of GCSObjectClient for testing. type mockGCSClient struct { mu sync.Mutex - objects map[string][]byte // key -> body - generation map[string]int64 // key -> current generation - errOnPut map[string]error // key -> error to return on conditional Put + objects map[string][]byte // key -> body + generation map[string]int64 // key -> current generation + errOnPut map[string]error // key -> error to return on conditional Put } func newMockGCSClient() *mockGCSClient { diff --git a/module/infra_module_deploy_bridge_test.go b/module/infra_module_deploy_bridge_test.go index e7ada5f4..11a2d378 100644 --- a/module/infra_module_deploy_bridge_test.go +++ b/module/infra_module_deploy_bridge_test.go @@ -46,7 +46,7 @@ func (d *deployCapableDriver) Update(_ context.Context, image string) error { d.updateCalled = true return nil } -func (d *deployCapableDriver) HealthCheck(_ context.Context, _ string) error { return d.healthErr } +func (d *deployCapableDriver) HealthCheck(_ context.Context, _ string) error { return d.healthErr } func (d *deployCapableDriver) CurrentImage(_ context.Context) (string, error) { return d.image, nil } func (d *deployCapableDriver) ReplicaCount(_ context.Context) (int, error) { return d.replicas, nil } @@ -93,9 +93,9 @@ func (d *deployCapableDriver) DestroyCanary(_ context.Context) error { type deployProviderMock struct { *infraMockProvider - deployDriver *deployCapableDriver - bgDriver *deployCapableDriver - canaryDriver *deployCapableDriver + deployDriver *deployCapableDriver + bgDriver *deployCapableDriver + canaryDriver *deployCapableDriver } func (p *deployProviderMock) ProvideDeployDriver(_ string) DeployDriver { diff --git a/module/infra_module_test.go b/module/infra_module_test.go index 0979e33b..12232fed 100644 --- a/module/infra_module_test.go +++ b/module/infra_module_test.go @@ -20,16 +20,16 @@ func newInfraMockApp() *infraMockApp { return &infraMockApp{services: make(map[string]any)} } -func (a *infraMockApp) RegisterConfigSection(string, modular.ConfigProvider) {} +func (a *infraMockApp) RegisterConfigSection(string, modular.ConfigProvider) {} func (a *infraMockApp) GetConfigSection(string) (modular.ConfigProvider, error) { return nil, nil } func (a *infraMockApp) ConfigSections() map[string]modular.ConfigProvider { return nil } -func (a *infraMockApp) Logger() modular.Logger { return &noopLogger{} } -func (a *infraMockApp) SetLogger(modular.Logger) {} -func (a *infraMockApp) ConfigProvider() modular.ConfigProvider { return nil } -func (a *infraMockApp) SvcRegistry() modular.ServiceRegistry { return a.services } -func (a *infraMockApp) RegisterModule(modular.Module) {} +func (a *infraMockApp) Logger() modular.Logger { return &noopLogger{} } +func (a *infraMockApp) SetLogger(modular.Logger) {} +func (a *infraMockApp) ConfigProvider() modular.ConfigProvider { return nil } +func (a *infraMockApp) SvcRegistry() modular.ServiceRegistry { return a.services } +func (a *infraMockApp) RegisterModule(modular.Module) {} func (a *infraMockApp) RegisterService(name string, svc any) error { a.services[name] = svc return nil @@ -46,23 +46,23 @@ func (a *infraMockApp) GetService(name string, target any) error { } return nil } -func (a *infraMockApp) Init() error { return nil } -func (a *infraMockApp) Start() error { return nil } -func (a *infraMockApp) Stop() error { return nil } -func (a *infraMockApp) Run() error { return nil } -func (a *infraMockApp) IsVerboseConfig() bool { return false } -func (a *infraMockApp) SetVerboseConfig(bool) {} -func (a *infraMockApp) Context() context.Context { return context.Background() } -func (a *infraMockApp) GetServicesByModule(string) []string { return nil } +func (a *infraMockApp) Init() error { return nil } +func (a *infraMockApp) Start() error { return nil } +func (a *infraMockApp) Stop() error { return nil } +func (a *infraMockApp) Run() error { return nil } +func (a *infraMockApp) IsVerboseConfig() bool { return false } +func (a *infraMockApp) SetVerboseConfig(bool) {} +func (a *infraMockApp) Context() context.Context { return context.Background() } +func (a *infraMockApp) GetServicesByModule(string) []string { return nil } func (a *infraMockApp) GetServiceEntry(string) (*modular.ServiceRegistryEntry, bool) { return nil, false } func (a *infraMockApp) GetServicesByInterface(reflect.Type) []*modular.ServiceRegistryEntry { return nil } -func (a *infraMockApp) GetModule(string) modular.Module { return nil } -func (a *infraMockApp) GetAllModules() map[string]modular.Module { return nil } -func (a *infraMockApp) StartTime() time.Time { return time.Time{} } +func (a *infraMockApp) GetModule(string) modular.Module { return nil } +func (a *infraMockApp) GetAllModules() map[string]modular.Module { return nil } +func (a *infraMockApp) StartTime() time.Time { return time.Time{} } func (a *infraMockApp) OnConfigLoaded(func(modular.Application) error) {} // infraMockProvider implements interfaces.IaCProvider for tests. @@ -408,11 +408,11 @@ func TestInfraModule_ProvidesServices(t *testing.T) { func TestInfraModule_ResourceConfig_StripsStandardKeys(t *testing.T) { m := NewInfraModule("db", "infra.database", map[string]any{ - "provider": "aws", - "size": "m", + "provider": "aws", + "size": "m", "resources": map[string]any{"cpu": "2"}, - "engine": "postgres", - "version": "16", + "engine": "postgres", + "version": "16", }) cfg := m.ResourceConfig() if _, ok := cfg["provider"]; ok { diff --git a/module/pipeline_executor.go b/module/pipeline_executor.go index 57fbec50..1c9fa4ed 100644 --- a/module/pipeline_executor.go +++ b/module/pipeline_executor.go @@ -40,6 +40,12 @@ type Pipeline struct { // used by step.request_parse for path parameter extraction. RoutePattern string + // StrictTemplates enables strict template key resolution: when true, any + // template expression that references a missing map key returns an error + // instead of silently resolving to the zero value. Corresponds to the + // pipeline config field strict_templates. + StrictTemplates bool + // EventRecorder is an optional recorder for execution events. // When nil (the default), no events are recorded. Events are best-effort: // recording failures are logged but never fail the pipeline. @@ -121,11 +127,13 @@ func (p *Pipeline) Execute(ctx context.Context, triggerData map[string]any) (*Pi } } pc := NewPipelineContext(triggerData, md) + pc.StrictTemplates = p.StrictTemplates logger := p.Logger if logger == nil { logger = slog.Default() } + pc.Logger = logger logger.Info("Pipeline started", "pipeline", p.Name, "steps", len(p.Steps)) diff --git a/module/pipeline_step_ai_classify_test.go b/module/pipeline_step_ai_classify_test.go index 62f38cea..c8c834bf 100644 --- a/module/pipeline_step_ai_classify_test.go +++ b/module/pipeline_step_ai_classify_test.go @@ -60,9 +60,9 @@ func TestAIClassifyStep_NonStringCategoryIgnored(t *testing.T) { func TestAIClassifyStep_WithValidStringCategories(t *testing.T) { registry := ai.NewAIModelRegistry() step, err := NewAIClassifyStepFactory(registry)("classify", map[string]any{ - "categories": []any{"spam", "not_spam"}, - "provider": "anthropic", - "input_from": ".body.text", + "categories": []any{"spam", "not_spam"}, + "provider": "anthropic", + "input_from": ".body.text", "temperature": 0.2, }, nil) if err != nil { diff --git a/module/pipeline_step_ai_extract_test.go b/module/pipeline_step_ai_extract_test.go index 18d81093..233e532a 100644 --- a/module/pipeline_step_ai_extract_test.go +++ b/module/pipeline_step_ai_extract_test.go @@ -43,10 +43,10 @@ func TestAIExtractStep_WithAllOptions(t *testing.T) { "schema": map[string]any{ "type": "object", }, - "provider": "anthropic", - "model": "claude-3-haiku", - "input_from": ".body.text", - "max_tokens": 256, + "provider": "anthropic", + "model": "claude-3-haiku", + "input_from": ".body.text", + "max_tokens": 256, "temperature": 0.0, }, nil) if err != nil { diff --git a/module/pipeline_step_container_build_test.go b/module/pipeline_step_container_build_test.go index 32127d4b..ae667f42 100644 --- a/module/pipeline_step_container_build_test.go +++ b/module/pipeline_step_container_build_test.go @@ -12,9 +12,9 @@ import ( // ─── Mock ContainerRegistry ─────────────────────────────────────────────────── type mockContainerRegistry struct { - registryURL string + registryURL string pushedImages []string - pushErr error + pushErr error } func (m *mockContainerRegistry) PushImage(_ context.Context, imageRef string) (string, error) { diff --git a/module/pipeline_step_deploy_blue_green_test.go b/module/pipeline_step_deploy_blue_green_test.go index 449761ae..0623a0e9 100644 --- a/module/pipeline_step_deploy_blue_green_test.go +++ b/module/pipeline_step_deploy_blue_green_test.go @@ -12,11 +12,11 @@ import ( type mockBlueGreenDriver struct { mockDeployDriver - createGreenErr error - switchTrafficErr error - destroyBlueErr error - greenEndpoint string - greenEndpointErr error + createGreenErr error + switchTrafficErr error + destroyBlueErr error + greenEndpoint string + greenEndpointErr error createGreenCalled bool switchTrafficCalled bool destroyBlueCalled bool diff --git a/module/pipeline_step_deploy_canary_test.go b/module/pipeline_step_deploy_canary_test.go index 95c0389d..38bb2bdf 100644 --- a/module/pipeline_step_deploy_canary_test.go +++ b/module/pipeline_step_deploy_canary_test.go @@ -12,11 +12,11 @@ import ( type mockCanaryDriver struct { mockDeployDriver - createCanaryErr error - routePercentErr error - metricGateErr error - promoteCanaryErr error - destroyCanaryErr error + createCanaryErr error + routePercentErr error + metricGateErr error + promoteCanaryErr error + destroyCanaryErr error createCanaryCalled bool promoteCanaryCalled bool diff --git a/module/pipeline_step_deploy_rollback.go b/module/pipeline_step_deploy_rollback.go index 51280aa2..410a3438 100644 --- a/module/pipeline_step_deploy_rollback.go +++ b/module/pipeline_step_deploy_rollback.go @@ -22,9 +22,9 @@ type DeployHistoryStore interface { // DeployHistoryEntry describes a single deployment event. type DeployHistoryEntry struct { - Service string `json:"service"` - Image string `json:"image"` - Version string `json:"version"` + Service string `json:"service"` + Image string `json:"image"` + Version string `json:"version"` DeployedAt time.Time `json:"deployed_at"` } @@ -181,10 +181,10 @@ func (s *DeployRollbackStep) Execute(ctx context.Context, _ *PipelineContext) (* } return &StepResult{Output: map[string]any{ - "success": true, - "service": s.service, - "rolled_back_to": entry.Version, - "image": entry.Image, + "success": true, + "service": s.service, + "rolled_back_to": entry.Version, + "image": entry.Image, "originally_deployed_at": entry.DeployedAt.Format(time.RFC3339), }}, nil } diff --git a/module/pipeline_step_deploy_rollback_test.go b/module/pipeline_step_deploy_rollback_test.go index 6fc32b69..a7240b68 100644 --- a/module/pipeline_step_deploy_rollback_test.go +++ b/module/pipeline_step_deploy_rollback_test.go @@ -35,8 +35,8 @@ func setupRollbackApp(t *testing.T) (*module.MockApplication, *mockDeployDriver, func baseRollbackCfg() map[string]any { return map[string]any{ - "service": "rb-svc", - "history_store": "deploy-history", + "service": "rb-svc", + "history_store": "deploy-history", "target_version": "previous", "health_check": map[string]any{ "path": "/health", diff --git a/module/pipeline_step_deploy_rolling.go b/module/pipeline_step_deploy_rolling.go index 6c42e888..f7ae122d 100644 --- a/module/pipeline_step_deploy_rolling.go +++ b/module/pipeline_step_deploy_rolling.go @@ -41,12 +41,12 @@ func resolveDeployDriver(app modular.Application, serviceName, stepName string) // HTTPDeployDriver is a simple DeployDriver backed by a mock or HTTP-reachable service. // It is provided for testing and simple HTTP-based deployments. type HTTPDeployDriver struct { - BaseURL string - CurrentImg string - ReplicaCnt int - UpdateFn func(ctx context.Context, image string) error - HealthFn func(ctx context.Context, path string) error - httpClient *http.Client + BaseURL string + CurrentImg string + ReplicaCnt int + UpdateFn func(ctx context.Context, image string) error + HealthFn func(ctx context.Context, path string) error + httpClient *http.Client } // Update calls the user-provided UpdateFn or sets CurrentImg directly. diff --git a/module/pipeline_step_deploy_rolling_test.go b/module/pipeline_step_deploy_rolling_test.go index fc3ead5d..6e4dfe66 100644 --- a/module/pipeline_step_deploy_rolling_test.go +++ b/module/pipeline_step_deploy_rolling_test.go @@ -55,9 +55,9 @@ func setupRollingApp(t *testing.T) (*module.MockApplication, *mockDeployDriver) func baseRollingCfg() map[string]any { return map[string]any{ - "service": "my-svc", - "image": "myapp:v2", - "max_surge": 1, + "service": "my-svc", + "image": "myapp:v2", + "max_surge": 1, "max_unavailable": 1, "health_check": map[string]any{ "path": "/health", diff --git a/module/pipeline_template.go b/module/pipeline_template.go index 7b808e45..6e8f643e 100644 --- a/module/pipeline_template.go +++ b/module/pipeline_template.go @@ -4,6 +4,7 @@ import ( "bytes" "encoding/json" "fmt" + "log/slog" "net/url" "reflect" "regexp" @@ -305,46 +306,98 @@ 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 { + // In strict mode (pc.StrictTemplates true), returns an error when the step + // doesn't exist or a key is missing. In default mode, returns nil (consistent + // with missingkey=zero behaviour) and a WARN is emitted by Resolve via the + // missingkey=error detection pass. + fm["step"] = func(name string, keys ...string) (any, error) { stepMap, ok := pc.StepOutputs[name] if !ok || stepMap == nil { - return nil + if pc.StrictTemplates { + return nil, fmt.Errorf("step %q not found in pipeline context", name) + } + return nil, nil //nolint:nilnil } var val any = stepMap for _, key := range keys { m, ok := val.(map[string]any) if !ok { - return nil + if pc.StrictTemplates { + return nil, fmt.Errorf("step %q: value at key path is not a map (cannot access key %q)", name, key) + } + return nil, nil //nolint:nilnil } - val = m[key] + v, exists := m[key] + if !exists { + if pc.StrictTemplates { + return nil, fmt.Errorf("step %q: key %q not found", name, key) + } + return nil, nil //nolint:nilnil + } + val = v } - return val + return val, nil } // trigger accesses trigger data by nested keys. // Usage: {{ trigger "path_params" "id" }} - fm["trigger"] = func(keys ...string) any { + // In strict mode (pc.StrictTemplates true), returns an error when a key is + // missing. In default mode, returns nil (consistent with missingkey=zero). + fm["trigger"] = func(keys ...string) (any, error) { if pc.TriggerData == nil { - return nil + if pc.StrictTemplates && len(keys) > 0 { + return nil, fmt.Errorf("trigger data is nil; cannot access key %q", keys[0]) + } + return nil, nil //nolint:nilnil } var val any = map[string]any(pc.TriggerData) for _, key := range keys { m, ok := val.(map[string]any) if !ok { - return nil + if pc.StrictTemplates { + return nil, fmt.Errorf("trigger: value at key path is not a map (cannot access key %q)", key) + } + return nil, nil //nolint:nilnil } - val = m[key] + v, exists := m[key] + if !exists { + if pc.StrictTemplates { + return nil, fmt.Errorf("trigger: key %q not found", key) + } + return nil, nil //nolint:nilnil + } + val = v } - return val + return val, nil } return fm } +// isMissingKeyError reports whether err is a text/template "map has no entry +// for key" error produced when missingkey=error is set. Checking the error +// message string is the standard approach because text/template does not +// export a typed sentinel for this condition. +func isMissingKeyError(err error) bool { + return strings.Contains(err.Error(), "map has no entry for key") +} + // Resolve evaluates a template string against a PipelineContext. // If the string does not contain {{ }}, it is returned as-is. +// +// Missing key behaviour (direct map access via {{ .steps.foo.bar }}): +// - When pc.StrictTemplates is true (Option A), any reference to a missing +// map key causes an immediate error via missingkey=error, surfacing typos +// as failures. +// - When pc.StrictTemplates is false (the default, Option C), a missing key +// resolves to the zero value AND a WARN log is emitted via pc.Logger (or +// slog.Default() when no logger is set) so that the silent failure is +// visible without breaking existing pipelines. +// +// NOTE: Strict template mode applies to both direct map key resolution +// (missingkey=error) and the step/trigger helper functions. Missing keys +// accessed via {{ step "name" "field" }} or {{ trigger "key" }} also return +// an error in strict mode. func (te *TemplateEngine) Resolve(tmplStr string, pc *PipelineContext) (string, error) { if !strings.Contains(tmplStr, "{{") { return tmplStr, nil @@ -352,14 +405,55 @@ func (te *TemplateEngine) Resolve(tmplStr string, pc *PipelineContext) (string, tmplStr = preprocessTemplate(tmplStr) - t, err := template.New("").Funcs(te.funcMapWithContext(pc)).Option("missingkey=zero").Parse(tmplStr) + // Parse once; we may execute with different missingkey options below. + t, err := template.New("").Funcs(te.funcMapWithContext(pc)).Parse(tmplStr) if err != nil { return "", fmt.Errorf("template parse error: %w", err) } + data := te.templateData(pc) + + // Strict mode (Option A): error immediately on missing keys. + if pc != nil && pc.StrictTemplates { + var buf bytes.Buffer + if err := t.Option("missingkey=error").Execute(&buf, data); err != nil { + return "", fmt.Errorf("template exec error: %w", err) + } + return buf.String(), nil + } + + // Default mode (Option C): try with missingkey=error to detect missing + // keys, log a warning (without template contents to avoid leaking + // secrets/PII), then fall back to missingkey=zero so the pipeline + // continues with the zero value (preserving backward compatibility). var buf bytes.Buffer - if err := t.Execute(&buf, te.templateData(pc)); err != nil { - return "", fmt.Errorf("template exec error: %w", err) + if execErr := t.Option("missingkey=error").Execute(&buf, data); execErr != nil { + if !isMissingKeyError(execErr) { + return "", fmt.Errorf("template exec error: %w", execErr) + } + + // Log a warning about the missing key so developers can spot typos, + // without including the full template text (which may contain secrets). + logger := slog.Default() + if pc != nil && pc.Logger != nil { + logger = pc.Logger + } + pipelineName := "" + if pc != nil && pc.Metadata != nil { + if v, ok := pc.Metadata["pipeline"]; ok { + pipelineName = fmt.Sprint(v) + } + } + logger.Warn("template resolved missing key to zero value", + "pipeline", pipelineName, + "error", execErr, + ) + + // Re-execute with zero mode to preserve backward-compatible output. + buf.Reset() + if err := t.Option("missingkey=zero").Execute(&buf, data); err != nil { + return "", fmt.Errorf("template exec error: %w", err) + } } return buf.String(), nil } diff --git a/module/pipeline_template_test.go b/module/pipeline_template_test.go index 6350a589..453512a7 100644 --- a/module/pipeline_template_test.go +++ b/module/pipeline_template_test.go @@ -2,6 +2,7 @@ package module import ( "fmt" + "log/slog" "strings" "testing" ) @@ -86,18 +87,159 @@ func TestTemplateEngine_EmptyStringPassthrough(t *testing.T) { } } -func TestTemplateEngine_MissingKeyReturnsZeroValue(t *testing.T) { +func TestTemplateEngine_MissingKeyLogsWarning(t *testing.T) { te := NewTemplateEngine() - pc := NewPipelineContext(nil, nil) + pc := NewPipelineContext(nil, map[string]any{"pipeline": "test-pipeline"}) + + // Capture log output to verify the warning is emitted. + var logBuf strings.Builder + handler := slog.NewTextHandler(&logBuf, &slog.HandlerOptions{Level: slog.LevelWarn}) + pc.Logger = slog.New(handler) - // missingkey=zero means missing keys produce zero values, not errors + // Non-strict mode: missing key resolves to zero value but logs a warning. result, err := te.Resolve("{{ .nonexistent }}", pc) if err != nil { - t.Fatalf("unexpected error: %v", err) + t.Fatalf("unexpected error in non-strict mode: %v", err) + } + // The zero value for a missing key in a map renders as "". + _ = result + + // A warning should have been logged with the pipeline name, not the full template. + logOutput := logBuf.String() + if !strings.Contains(logOutput, "template resolved missing key to zero value") { + t.Errorf("expected missing-key warning in log, got: %q", logOutput) + } + if !strings.Contains(logOutput, "test-pipeline") { + t.Errorf("expected pipeline name in log, got: %q", logOutput) + } + // The raw template attribute must NOT appear in logs (security: may contain secrets/PII). + // The key name may appear in the error message from text/template, which is acceptable. + if strings.Contains(logOutput, "template={{ .nonexistent }}") { + t.Errorf("log should not contain raw template= attribute (security), got: %q", logOutput) + } +} + +func TestTemplateEngine_StrictModeReturnsError(t *testing.T) { + te := NewTemplateEngine() + pc := NewPipelineContext(nil, nil) + pc.StrictTemplates = true + + _, err := te.Resolve("{{ .nonexistent }}", pc) + if err == nil { + t.Fatal("expected error in strict mode for missing key") + } + if !strings.Contains(err.Error(), "template exec error") { + t.Errorf("expected 'template exec error' in message, got: %v", err) + } +} + +func TestTemplateEngine_StrictModePassesForPresentKey(t *testing.T) { + te := NewTemplateEngine() + pc := NewPipelineContext(map[string]any{"name": "Alice"}, nil) + pc.StrictTemplates = true + + result, err := te.Resolve("{{ .name }}", pc) + if err != nil { + t.Fatalf("unexpected error in strict mode for present key: %v", err) + } + if result != "Alice" { + t.Errorf("expected 'Alice', got %q", result) + } +} + +func TestTemplateEngine_StrictModeStepFieldTypoReturnsError(t *testing.T) { + te := NewTemplateEngine() + pc := NewPipelineContext(nil, nil) + pc.MergeStepOutput("auth", map[string]any{"affiliate_id": "tenant123"}) + pc.StrictTemplates = true + + // Correct access should succeed. + result, err := te.Resolve("{{ .steps.auth.affiliate_id }}", pc) + if err != nil { + t.Fatalf("unexpected error for correct field in strict mode: %v", err) + } + if result != "tenant123" { + t.Errorf("expected 'tenant123', got %q", result) + } + + // Typo in field name should fail in strict mode. + _, err = te.Resolve("{{ .steps.auth.affilate_id }}", pc) + if err == nil { + t.Fatal("expected error for typo in field name in strict mode") + } +} + +func TestTemplateEngine_NonStrictModeStepFieldTypoLogsWarning(t *testing.T) { + te := NewTemplateEngine() + pc := NewPipelineContext(nil, nil) + pc.MergeStepOutput("auth", map[string]any{"affiliate_id": "tenant123"}) + + var logBuf strings.Builder + handler := slog.NewTextHandler(&logBuf, &slog.HandlerOptions{Level: slog.LevelWarn}) + pc.Logger = slog.New(handler) + + // Typo in field name: should resolve to zero value with a warning. + result, err := te.Resolve("{{ .steps.auth.affilate_id }}", pc) + if err != nil { + t.Fatalf("unexpected error in non-strict mode for field typo: %v", err) + } + _ = result // zero/empty value + + if !strings.Contains(logBuf.String(), "template resolved missing key to zero value") { + t.Errorf("expected missing-key warning in log, got: %q", logBuf.String()) + } +} + +func TestTemplateEngine_StrictModeStepHelperMissingStepReturnsError(t *testing.T) { + te := NewTemplateEngine() + pc := NewPipelineContext(nil, nil) + pc.StrictTemplates = true + + // step helper for nonexistent step should fail in strict mode. + _, err := te.Resolve(`{{ step "nonexistent" "field" }}`, pc) + if err == nil { + t.Fatal("expected error for missing step in strict mode via step helper") + } +} + +func TestTemplateEngine_StrictModeStepHelperMissingFieldReturnsError(t *testing.T) { + te := NewTemplateEngine() + pc := NewPipelineContext(nil, nil) + pc.MergeStepOutput("auth", map[string]any{"affiliate_id": "tenant123"}) + pc.StrictTemplates = true + + // step helper for existing step but missing field should fail in strict mode. + _, err := te.Resolve(`{{ step "auth" "affilate_id" }}`, pc) + if err == nil { + t.Fatal("expected error for missing field in strict mode via step helper") + } +} + +func TestTemplateEngine_StrictModeStepHelperSucceeds(t *testing.T) { + te := NewTemplateEngine() + pc := NewPipelineContext(nil, nil) + pc.MergeStepOutput("auth", map[string]any{"affiliate_id": "tenant123"}) + pc.StrictTemplates = true + + result, err := te.Resolve(`{{ step "auth" "affiliate_id" }}`, pc) + if err != nil { + t.Fatalf("unexpected error in strict mode for correct step helper access: %v", err) + } + if result != "tenant123" { + t.Errorf("expected 'tenant123', got %q", result) + } +} + +func TestTemplateEngine_StrictModeTriggerHelperMissingKeyReturnsError(t *testing.T) { + te := NewTemplateEngine() + pc := NewPipelineContext(map[string]any{"source": "webhook"}, nil) + pc.StrictTemplates = true + + // trigger helper for missing key should fail in strict mode. + _, err := te.Resolve(`{{ trigger "nonexistent_key" }}`, pc) + if err == nil { + t.Fatal("expected error for missing trigger key in strict mode via trigger helper") } - // The zero value for a missing key in a map renders as "" or empty - // With missingkey=zero it should render as the zero value string representation - _ = result // Just verify no error } func TestTemplateEngine_InvalidTemplateReturnsError(t *testing.T) { diff --git a/module/step_output_redactor_test.go b/module/step_output_redactor_test.go index b686c182..1e2d3894 100644 --- a/module/step_output_redactor_test.go +++ b/module/step_output_redactor_test.go @@ -6,7 +6,7 @@ import ( func TestRedactStepOutput_SensitiveFields(t *testing.T) { cases := []struct { - key string + key string wantRedacted bool }{ {"password", true}, diff --git a/plugin/autofetch_test.go b/plugin/autofetch_test.go index aa9d4d4e..c49c1b64 100644 --- a/plugin/autofetch_test.go +++ b/plugin/autofetch_test.go @@ -56,9 +56,9 @@ func TestAutoFetchPlugin_WfctlNotFound(t *testing.T) { // compound constraints are detected as invalid. func TestStripVersionConstraint(t *testing.T) { cases := []struct { - input string - want string - wantOK bool + input string + want string + wantOK bool }{ {">=0.1.0", "0.1.0", true}, {"<=0.1.0", "0.1.0", true}, @@ -66,9 +66,9 @@ func TestStripVersionConstraint(t *testing.T) { {"~1.0.0", "1.0.0", true}, {"0.3.0", "0.3.0", true}, {"", "", true}, - {">=0.1.0,<0.2.0", "", false}, // compound — not supported - {">=0.1.0 <0.2.0", "", false}, // compound with space - {"0.1.0 0.2.0", "", false}, // two bare versions separated by space + {">=0.1.0,<0.2.0", "", false}, // compound — not supported + {">=0.1.0 <0.2.0", "", false}, // compound with space + {"0.1.0 0.2.0", "", false}, // two bare versions separated by space } for _, tc := range cases { got, ok := stripVersionConstraint(tc.input) diff --git a/schema/coercion.go b/schema/coercion.go index 64d2d2ff..1703e2b6 100644 --- a/schema/coercion.go +++ b/schema/coercion.go @@ -41,28 +41,28 @@ func NewTypeCoercionRegistry() *TypeCoercionRegistry { // Pipeline types "PipelineContext": {"any", "StepResult", "PipelineContext"}, - "StepResult": {"any", "PipelineContext", "StepResult"}, + "StepResult": {"any", "PipelineContext", "StepResult"}, // Service/provider types "prometheus.Metrics": {"any"}, - "net.Listener": {"any"}, - "Scheduler": {"any"}, - "AuthService": {"any"}, - "EventBus": {"any"}, - "Cache": {"any"}, - "http.Client": {"any"}, - "sql.DB": {"any"}, - "SchemaValidator": {"any"}, - "StorageProvider": {"any"}, - "SecretProvider": {"any"}, - "PersistenceStore": {"any"}, - "WorkflowRegistry": {"any"}, - "ExternalAPIClient": {"any"}, - "FileStore": {"any", "StorageProvider"}, - "ObjectStore": {"any", "StorageProvider"}, - "UserStore": {"any"}, - "trace.Span": {"any"}, - "trace.Tracer": {"any"}, + "net.Listener": {"any"}, + "Scheduler": {"any"}, + "AuthService": {"any"}, + "EventBus": {"any"}, + "Cache": {"any"}, + "http.Client": {"any"}, + "sql.DB": {"any"}, + "SchemaValidator": {"any"}, + "StorageProvider": {"any"}, + "SecretProvider": {"any"}, + "PersistenceStore": {"any"}, + "WorkflowRegistry": {"any"}, + "ExternalAPIClient": {"any"}, + "FileStore": {"any", "StorageProvider"}, + "ObjectStore": {"any", "StorageProvider"}, + "UserStore": {"any"}, + "trace.Span": {"any"}, + "trace.Tracer": {"any"}, }} return r } diff --git a/schema/step_schema_builtins.go b/schema/step_schema_builtins.go index e56846ba..fd20c754 100644 --- a/schema/step_schema_builtins.go +++ b/schema/step_schema_builtins.go @@ -2360,9 +2360,9 @@ func (r *StepSchemaRegistry) registerBuiltins() { // ---- Marketplace Installed ---- r.Register(&StepSchema{ - Type: "step.marketplace_installed", - Plugin: "marketplace", - Description: "Lists installed marketplace plugins.", + Type: "step.marketplace_installed", + Plugin: "marketplace", + Description: "Lists installed marketplace plugins.", ConfigFields: []ConfigFieldDef{}, Outputs: []StepOutputDef{ {Key: "plugins", Type: "[]any", Description: "List of installed plugin metadata"},