feat: add skip_if and if fields to pipeline step execution#332
Merged
feat: add skip_if and if fields to pipeline step execution#332
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>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds conditional execution controls for pipeline steps via skip_if / if guard templates, and wires them into both named pipelines and inline route pipelines. The PR also includes several plugin/registry-related robustness improvements (integrity hashing, auto-fetch logging/behavior, lockfile handling, and registry HTTP timeouts) plus plugin declaration deduplication during application config merge.
Changes:
- Add
skip_if/iffields toPipelineStepConfigand wrap steps with a newSkippableStepguard. - Extend
workflow_callto support templated workflow names and an optionalstop_pipelinebehavior. - Improve plugin tooling reliability (streaming SHA-256 hashing, registry HTTP client timeouts, auto-fetch behavior) and deduplicate merged external plugin declarations.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
engine.go |
Parses skip_if/if on inline route steps and wraps built steps with SkippableStep. |
config/pipeline.go |
Adds SkipIf/If fields to pipeline step config schema. |
module/pipeline_step_skip.go |
Introduces SkippableStep wrapper implementing guard evaluation and skip output. |
module/pipeline_step_skip_test.go |
Adds unit tests covering guard combinations and template access. |
module/pipeline_step_workflow_call.go |
Adds stop_pipeline, supports templated workflow lookup, and returns Stop in results. |
config/config.go |
Merges/deduplicates external plugin declarations when merging application workflow files. |
config/merge_test.go |
Tests plugin deduplication behavior during application config merge. |
plugin/autofetch.go |
Refactors auto-fetch to support optional slog logging and avoids wfctl warnings when unused. |
plugin/autofetch_test.go |
Renames/retargets test to validate skip-when-installed behavior. |
plugin/integrity.go |
Switches integrity hashing to streaming I/O for plugin binaries. |
plugin/integrity_test.go |
Makes unreadable-lockfile test resilient across platforms/root execution. |
cmd/wfctl/registry_source.go |
Routes registry HTTP through a shared client with a timeout; minor struct init tweak. |
cmd/wfctl/plugin_install.go |
Uses streaming SHA-256 hashing; tweaks lockfile update behavior/messages. |
cmd/wfctl/plugin_lockfile.go |
Improves checksum-mismatch handling by warning on removal failures. |
cmd/wfctl/multi_registry.go |
Small variable rename/cleanup in static registry construction. |
docs/PLUGIN_AUTHORING.md |
Clarifies plugin naming expectations and short vs canonical naming. |
Comment on lines
+48
to
+66
| val, err := s.tmpl.Resolve(s.skipIf, pc) | ||
| if err != nil { | ||
| // Template resolution errors are non-fatal: treat as falsy (execute). | ||
| val = "" | ||
| } | ||
| if isTruthy(val) { | ||
| return skippedResult("skip_if evaluated to true"), nil | ||
| } | ||
| } | ||
|
|
||
| // Evaluate if (inverse logic: falsy → skip) | ||
| if s.ifExpr != "" { | ||
| val, err := s.tmpl.Resolve(s.ifExpr, pc) | ||
| if err != nil { | ||
| // Template resolution errors are non-fatal: treat as falsy (skip). | ||
| val = "" | ||
| } | ||
| if !isTruthy(val) { | ||
| return skippedResult("if evaluated to false"), nil |
Comment on lines
+91
to
+92
| "skipped": true, | ||
| "reason": reason, |
Comment on lines
+30
to
+31
| // template evaluates to truthy. Falsy or absent with no SkipIf → execute. | ||
| // When both SkipIf and If are set, SkipIf takes precedence. |
⏱ Benchmark Results✅ No significant performance regressions detected. benchstat comparison (baseline → PR)
|
Contributor
Author
|
@copilot open a new pull request to apply changes based on the comments in this thread |
Contributor
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.
Summary
skip_ifandifoptional Go template fields toPipelineStepConfig(YAML:skip_if/if)SkippableStepwrapper inmodule/pipeline_step_skip.go— evaluated beforeExecute, zero overhead when neither field is setskip_if: truthy result (non-empty, not"false", not"0") → step is skippedif: inverse — falsy result → step is skipped; when both are set,skip_iftakes precedence{"skipped": true, "reason": "..."}so downstream steps can inspect the resultbuildPipelineSteps(named pipelines) andparseRoutePipelineSteps(inline route pipelines) apply the wrapperUsage
Test plan
go test ./... -timeout 120s— all packages pass (no regressions)TestStepSkipIf_SkipsWhenTrue— step not executed when skip_if is truthyTestStepSkipIf_ExecutesWhenFalse— step executes when skip_if is falsyTestStepSkipIf_OutputContainsSkippedFlag— skipped output includesskipped: trueandreasonTestStepIf_ExecutesWhenTrue— step executes whenifis truthyTestStepIf_SkipsWhenFalse— step skipped whenifis falsyTestStepSkipIf_TemplateResolution— templates read from pipeline context (current, step outputs)TestStepSkipIf_AbsentMeansExecute— backward compatible, always executes when neither field is setTestStepSkipIf_FalseStringIsFalsy/TestStepSkipIf_ZeroStringIsFalsy— falsy value handling🤖 Generated with Claude Code