feat: plugin-aware modernize rules for wfctl modernize and MCP#304
Merged
feat: plugin-aware modernize rules for wfctl modernize and MCP#304
Conversation
Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Add support for external plugins in wfctl modernize command
feat: plugin-aware modernize rules for wfctl modernize and MCP
Mar 12, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Adds plugin-declared migration rules to the modernize ecosystem so wfctl modernize and the MCP modernize tool can detect/fix plugin-specific breaking config changes (type/key renames) when a plugin directory is provided.
Changes:
- Introduces
modernize.ManifestRule(JSON-serializable) plusmodernize.LoadRulesFromDir()to compile plugin.json rule descriptors into executable modernize rules. - Extends
wfctl modernizewith--plugin-dirand updates MCPmodernizeto include external plugin rules when the server started with a plugin directory. - Updates plugin manifest structs/docs and adds test coverage for rule parsing/loading and CLI integration.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| plugin/manifest.go | Adds ModernizeRules []modernize.ManifestRule to the Go manifest struct to mirror plugin.json. |
| plugin/engine_plugin.go | Introduces optional ModernizeRulesProvider interface for in-process plugins. |
| modernize/manifest_rule.go | Implements ManifestRule, conversion to executable Rule, and LoadRulesFromDir() for external plugins. |
| modernize/manifest_rule_test.go | Adds unit tests for rule validation, rule behavior, and loading rules from plugin.json files. |
| mcp/wfctl_tools.go | Updates MCP modernize tool to append plugin-provided rules when s.pluginDir is set. |
| docs/WFCTL.md | Documents wfctl modernize --plugin-dir and how plugin rules are discovered/used. |
| docs/PLUGIN_DEVELOPMENT_GUIDE.md | Documents modernizeRules in external plugin.json manifests and ecosystem guidance. |
| docs/PLUGIN_DEVELOPMENT.md | Documents in-process plugin modernize rules and the updated manifest struct shape. |
| cmd/wfctl/modernize.go | Adds --plugin-dir flag and loads plugin rules into the modernize rule set. |
| cmd/wfctl/modernize_test.go | Adds CLI tests covering --plugin-dir (empty, missing, list-rules, apply, multiple plugins). |
| cmd/wfctl/main.go | Re-aligns the commands map formatting (no behavior change). |
Comment on lines
+347
to
+355
| keyNode := findMapKeyNode(cfg, oldKey) | ||
| if keyNode != nil { | ||
| changes = append(changes, Change{ | ||
| RuleID: id, | ||
| Line: keyNode.Line, | ||
| Description: fmt.Sprintf("Renamed config key %q -> %q in %q step", oldKey, newKey, stepType), | ||
| }) | ||
| keyNode.Value = newKey | ||
| } |
There was a problem hiding this comment.
The step config key rename fix has the same key-collision issue as the module variant: renaming oldKey to newKey can create duplicate keys if newKey already exists in the step config mapping. Please handle the collision case explicitly (e.g., remove oldKey instead of renaming, or skip/mark not-fixable) to avoid producing ambiguous YAML.
Comment on lines
+1709
to
+1713
| if s.pluginDir != "" { | ||
| pluginRules, err := modernize.LoadRulesFromDir(s.pluginDir) | ||
| if err == nil { | ||
| rules = append(rules, pluginRules...) | ||
| } |
There was a problem hiding this comment.
handleModernize silently ignores errors when loading plugin rules (it only appends rules when err==nil). This can make plugin rule loading failures invisible to clients even though the tool description says plugin rules are included when started with a plugin directory. Consider returning a tool error (or at least including a warning field in the tool result) when LoadRulesFromDir fails so users can diagnose misconfigured/unreadable plugin directories.
| } | ||
| if kinds == 0 { | ||
| return fmt.Errorf("modernize rule %q: must specify at least one of: oldModuleType, oldStepType, or moduleType/stepType + oldKey", mr.ID) | ||
| } |
There was a problem hiding this comment.
ManifestRule.Validate() says exactly one rule kind must be configured, but it only errors when kinds==0. If a rule descriptor sets multiple kind fields (e.g., oldModuleType plus moduleType/oldKey), Validate() will pass and ToRule() will silently pick the first match, dropping the others. Please enforce kinds==1 (error when kinds>1) so misconfigured manifests fail fast and deterministically.
Suggested change
| } | |
| } | |
| if kinds > 1 { | |
| return fmt.Errorf("modernize rule %q: exactly one rule kind must be configured; found %d kinds", mr.ID, kinds) | |
| } |
Comment on lines
+123
to
+126
| sev := mr.Severity | ||
| if sev == "" { | ||
| sev = "warning" | ||
| } |
There was a problem hiding this comment.
Severity is documented/assumed to be "error" or "warning", but ManifestRule.ToRule() passes through any non-empty value without validation. This can lead to inconsistent output or downstream handling if a plugin emits an unexpected severity string. Consider validating mr.Severity in Validate() (allow empty, "warning", "error") and returning a clear error for unsupported values.
Comment on lines
+291
to
+299
| keyNode := findMapKeyNode(cfg, oldKey) | ||
| if keyNode != nil { | ||
| changes = append(changes, Change{ | ||
| RuleID: id, | ||
| Line: keyNode.Line, | ||
| Description: fmt.Sprintf("Renamed config key %q -> %q in %q module", oldKey, newKey, moduleType), | ||
| }) | ||
| keyNode.Value = newKey | ||
| } |
There was a problem hiding this comment.
The module config key rename fix renames the YAML key node in-place without checking whether newKey already exists in the same mapping. If both oldKey and newKey are present, this will create duplicate keys in the YAML mapping, which can change semantics or make the output confusing. Please detect collisions (newKey already present) and either delete the oldKey entry, merge safely, or mark the finding as not fixable in that scenario.
⏱ Benchmark Results✅ No significant performance regressions detected. benchstat comparison (baseline → PR)
|
Contributor
|
@copilot apply changes based on the comments in this thread |
…tection, MCP error handling, and interface comments Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>
Contributor
Author
Applied all review feedback in 7948c76:
|
Contributor
|
@copilot Make sure linter is passing |
…ocritic linter Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>
Contributor
Author
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.
wfctl modernizeonly knew about core engine rules. External plugins had no way to declare migration rules for their own type renames and config key changes, leaving plugin users without automated migration tooling.New:
modernize.ManifestRule— JSON-serializable rule descriptorsFour rule kinds, declared in
plugin.jsonundermodernizeRules:{ "modernizeRules": [ { "id": "my-rename-type", "description": "...", "oldModuleType": "my.v1", "newModuleType": "my.v2" }, { "id": "my-rename-step", "description": "...", "oldStepType": "step.old", "newStepType": "step.new" }, { "id": "my-rename-modkey", "description": "...", "moduleType": "my.v2", "oldKey": "apiEndpoint", "newKey": "endpoint" }, { "id": "my-rename-stepkey","description": "...", "stepType": "step.new", "oldKey": "timeout_ms", "newKey": "timeoutMs" } ] }modernize.LoadRulesFromDir(pluginDir)scans*/plugin.jsonfiles and compiles them into[]modernize.Rulewith full Check/Fix AST functions.wfctl modernize --plugin-dir(mirrorsvalidate --plugin-dir)In-process Go plugin interface
Optional
plugin.ModernizeRulesProviderinterface for function-based rules that go beyond pattern matching:plugin.PluginManifestgains aModernizeRules []modernize.ManifestRulefield so the Go struct mirrors the JSON format.MCP server
handleModernizeautomatically includes plugin rules when the server was started with--plugin-dir. No client-side changes needed.What plugin authors and
workflow-registryneed to domodernizeRulestoplugin.jsonon breaking releases; keep historical rules until the old types can be considered fully retired.GoCodeAlone/workflow-registry: extendRegistryCapabilitieswith amodernizeRulessummary; updatewfctl publishto include the rules in registry manifests for discovery.Documented in
docs/PLUGIN_DEVELOPMENT_GUIDE.md(external) anddocs/PLUGIN_DEVELOPMENT.md(in-process), with a full field reference table and ecosystem guidance.💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.