fix(pipeline): fail closed on guard template errors; use reserved metadata keys for skipped output#333
Merged
fix(pipeline): fail closed on guard template errors; use reserved metadata keys for skipped output#333
Conversation
Adds --url flag to wfctl plugin install that downloads a tar.gz archive from a direct URL, extracts plugin.json to identify the plugin name, installs to the plugin directory, and records the SHA-256 checksum in the lockfile. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Extends wfctl plugin init to generate cmd/workflow-plugin-<name>/main.go, internal/provider.go, internal/steps.go, go.mod, .goreleaser.yml, CI/release GitHub Actions workflows, Makefile, and README.md. Adds --module flag for custom Go module paths. Preserves existing plugin.json and .go skeleton for backward compatibility. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add AutoFetchPlugin and AutoFetchDeclaredPlugins to plugin/autofetch.go, which shell out to wfctl to download plugins not found locally. Extend WorkflowConfig with a new PluginsConfig / ExternalPluginDecl type so configs can declare plugins with autoFetch: true and an optional version constraint. StdEngine gains SetExternalPluginDir and calls AutoFetchDeclaredPlugins in BuildFromConfig before module loading. The server's buildEngine registers the plugin dir so auto-fetch is active at runtime. If wfctl is absent, a warning is logged and startup continues. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Use struct conversion for staticIndexEntry → PluginSummary (staticcheck S1016) - Remove unused updateLockfile and writePluginJSON functions - Add nilerr annotations for intentional nil returns in integrity.go - Add gosec annotation for exec.Command in autofetch.go - Fix TestLoadRegistryConfigDefault to use DefaultRegistryConfig() directly Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- integrity.go: fail closed when lockfile exists but is unreadable or unparseable, preventing integrity enforcement bypass - autofetch.go: extract stripVersionConstraint helper; detect compound version constraints and fall back to latest; check both pluginName and workflow-plugin-<name> alternate form for installed-check; log restart warning when new plugins are downloaded (they require a server restart) - autofetch_test.go: test stripVersionConstraint directly instead of duplicating the logic inline; add compound-constraint cases - engine.go: clarify comment that auto-fetched plugins need a restart Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- generator.go: use plugin/external/sdk imports and types (PluginProvider, StepInstance, StepResult, StepTypes/CreateStep) instead of plugin/sdk - PLUGIN_AUTHORING.md: update examples to match external SDK interfaces - plugin_install.go: hash installed binary (not archive) for lockfile, add hashFileSHA256 helper, add install mode mutual exclusivity check, update installFromLocal to write lockfile, normalize plugin names - plugin_lockfile.go: add registry param to updateLockfileWithChecksum, pass version/registry in installFromLockfile, remove dir on mismatch - registry_source.go: validate URL in NewStaticRegistrySource - config.go: clarify Version field forwarding semantics Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- registry_source.go: use explicit field assignment for PluginSummary instead of struct type conversion (clearer, avoids tag confusion) - plugin_lockfile.go: don't pass @Version in installFromLockfile to prevent lockfile overwrite before checksum verification - plugin_install.go: add verifyInstalledPlugin() call in installFromURL for parity with registry installs - engine.go: add TODO to move auto-fetch before plugin discovery so newly fetched plugins are available without restart - integrity_test.go: add tests for unreadable and malformed lockfile to verify fail-closed behavior Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- registry_source.go: add nolint:gosimple for S1016 — explicit field assignment preferred for clarity across different struct tags - generator_test.go: add TestGenerateProjectStructure verifying all generated files exist and use correct external SDK imports/types Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…immediately Auto-fetch was running inside BuildFromConfig, which executes after external plugins are already discovered and loaded. Plugins downloaded by auto-fetch required a server restart to take effect. Move auto-fetch to buildEngine in cmd/server/main.go, before DiscoverPlugins/LoadPlugin. Remove the now-unused externalPluginDir field and SetExternalPluginDir from the engine. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Enables pipelines to call other pipelines by name with full context forwarding. Supports template-resolved workflow names and stop_pipeline option. Required for WebSocket message routing to game-specific pipelines. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- registry_source.go: switch GitHubRegistrySource to use registryHTTPClient (timeout-configured) for both ListPlugins and FetchManifest; update comment - autofetch.go: scan for AutoFetch=true entries before exec.LookPath to avoid misleading startup warning when no plugins need auto-fetch - autofetch.go: add internal autoFetchPlugin helper that accepts *slog.Logger and emits structured log entries when a logger is available; public AutoFetchPlugin delegates to it; AutoFetchDeclaredPlugins passes its logger - autofetch_test.go: rename TestAutoFetchPlugin_CorrectArgs to TestAutoFetchPlugin_SkipsWhenExists to match what the test actually asserts - integrity.go: replace os.ReadFile + sha256.Sum256 with streaming os.Open + io.Copy into sha256.New() to keep memory bounded for large binaries - integrity_test.go: add t.Skip guard in UnreadableLockfile test when the file is actually readable (Windows / root environments) - pipeline_step_workflow_call.go: use resolved workflowName (not s.workflow) in async return payload and sync error message for consistency - docs/PLUGIN_AUTHORING.md: clarify the distinction between plugin.json name (short, used by engine) and the registry/provider manifest name (workflow-plugin- prefixed), and which is used for dependency resolution - config/config.go: MergeApplicationConfig now merges Plugins.External from each referenced workflow file into the combined config, deduplicated by name Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- docs/PLUGIN_AUTHORING.md: close unclosed code fence after StepProvider example - cmd/wfctl/plugin_install.go: streaming hashFileSHA256 via io.Copy + sha256.New() - cmd/wfctl/plugin_install.go: warn on hash failure instead of silent empty checksum - config/merge_test.go: add TestMergeApplicationConfig_PluginDedup covering first-definition-wins dedup Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- pipeline_step_workflow_call.go: propagate stop_pipeline in async mode - integrity.go: "open plugin binary" instead of "read" for os.Open error - plugin_install.go: lowercase "warning:" for consistency - merge_test.go: use filepath.Join and check writeFileContent errors Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Steps now support optional `skip_if` and `if` Go template guards.
When `skip_if` evaluates to a truthy value the step is skipped and
the pipeline continues; `if` is the logical inverse. Skipped steps
produce `{"skipped": true, "reason": "..."}` output so downstream
steps can inspect the result.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
9 tasks
- Return errors (fail closed) instead of swallowing skip_if/if template failures - Use _skipped/_error keys in skipped step output (align with ErrorStrategySkip convention) - Fix misleading comment for If field in config/pipeline.go - Update tests to reflect new key names; add 2 tests for template error behavior Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Add skip_if and if fields to pipeline step execution
fix(pipeline): fail closed on guard template errors; use reserved metadata keys for skipped output
Mar 15, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens pipeline guard behavior (skip_if / if) by failing closed on template resolution errors and by emitting reserved underscore-prefixed metadata keys for skipped-step output to avoid collisions when step outputs merge into pc.Current.
Changes:
- Add
SkippableStepwrapper that evaluatesskip_if/if, returns errors on template parse/exec failures, and emits"_skipped"/"_error"metadata on guarded skips. - Extend pipeline step config (
config.PipelineStepConfig) and route inline pipeline parsing to carryskip_if/if, and wrap steps at build time when either guard is present. - Add unit tests validating skip/execute behavior, reserved metadata output keys, and fail-closed behavior on guard template errors.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
module/pipeline_step_skip.go |
Introduces the SkippableStep wrapper, truthiness rules, and reserved metadata output for guarded skips. |
engine.go |
Plumbs skip_if / if from inline route step YAML into PipelineStepConfig and wraps steps during pipeline build. |
config/pipeline.go |
Adds SkipIf / If fields and clarifies guard semantics in comments. |
module/pipeline_step_skip_test.go |
Adds test coverage for guard execution, metadata output keys, and template-error fail-closed behavior. |
Comment on lines
+969
to
+976
| skipIf, _ := stepMap["skip_if"].(string) | ||
| ifExpr, _ := stepMap["if"].(string) | ||
| cfgs = append(cfgs, config.PipelineStepConfig{ | ||
| Name: name, | ||
| Type: stepType, | ||
| Config: stepConfig, | ||
| SkipIf: skipIf, | ||
| If: ifExpr, |
Comment on lines
+21
to
+26
| type SkippableStep struct { | ||
| inner interfaces.PipelineStep | ||
| skipIf string // Go template; truthy result → skip | ||
| ifExpr string // Go template; falsy result → skip | ||
| tmpl *TemplateEngine | ||
| } |
Comment on lines
+89
to
+95
| func skippedResult(reason string) *interfaces.StepResult { | ||
| return &interfaces.StepResult{ | ||
| Output: map[string]any{ | ||
| "_skipped": true, | ||
| "_error": reason, | ||
| }, | ||
| } |
⏱ Benchmark Results✅ No significant performance regressions detected. benchstat comparison (baseline → PR)
|
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.
Three issues in
SkippableStepidentified during review of theskip_if/ifguard implementation:skip_ifwere treated as falsy (execute), and inifas falsy (skip). Both silently masked misconfigured templates.skipped/reasonat the top level of step output, which merges intopc.Currentand can overwrite business fields.Iffield implied absent and falsy behave identically; a presentifresolving to falsy should (and does) skip.Changes
module/pipeline_step_skip.goskip_ifandifnow return a wrappederrorfromExecute(fail closed), consistent with how other step/template failures are handled._skipped: true/_error: <reason>— the same reserved underscore-prefixed metadata keys used byErrorStrategySkipinpipeline_executor.go.config/pipeline.goIffield comment to clearly state that a presentifresolving to a falsy value skips the step.module/pipeline_step_skip_test.go"skipped"/"reason"→"_skipped"/"_error".TestStepSkipIf_TemplateError_ReturnsErrorandTestStepIf_TemplateError_ReturnsErrorto verify fail-closed behavior.🔒 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.