diff --git a/cmd/wfctl/main.go b/cmd/wfctl/main.go index 2c31bff4..f0ee6a21 100644 --- a/cmd/wfctl/main.go +++ b/cmd/wfctl/main.go @@ -32,29 +32,29 @@ var version = "dev" // 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, + "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, diff --git a/cmd/wfctl/modernize.go b/cmd/wfctl/modernize.go index 9ff2e117..442006dc 100644 --- a/cmd/wfctl/modernize.go +++ b/cmd/wfctl/modernize.go @@ -17,6 +17,7 @@ func runModernize(args []string) error { excludeFlag := fs.String("exclude-rules", "", "Comma-separated list of rule IDs to skip") format := fs.String("format", "text", "Output format: text or json") dir := fs.String("dir", "", "Scan all YAML files in a directory (recursive)") + pluginDir := fs.String("plugin-dir", "", "Directory of installed external plugins; their modernize rules are loaded") fs.Usage = func() { fmt.Fprintf(fs.Output(), `Usage: wfctl modernize [options] [config2.yaml ...] @@ -30,6 +31,7 @@ Examples: wfctl modernize --dir ./config/ wfctl modernize --rules hyphen-steps,conditional-field config.yaml wfctl modernize --list-rules + wfctl modernize --plugin-dir data/plugins config.yaml Options: `) @@ -42,6 +44,15 @@ Options: rules := modernize.AllRules() + // Load additional modernize rules from installed external plugins. + if *pluginDir != "" { + pluginRules, err := modernize.LoadRulesFromDir(*pluginDir) + if err != nil { + return fmt.Errorf("failed to load plugin rules from %s: %w", *pluginDir, err) + } + rules = append(rules, pluginRules...) + } + if *listRules { fmt.Println("Available modernize rules:") fmt.Println() diff --git a/cmd/wfctl/modernize_test.go b/cmd/wfctl/modernize_test.go index cde75baa..7c5be023 100644 --- a/cmd/wfctl/modernize_test.go +++ b/cmd/wfctl/modernize_test.go @@ -1,7 +1,9 @@ package main import ( + "encoding/json" "os" + "path/filepath" "strings" "testing" @@ -591,3 +593,221 @@ pipelines: t.Error("conditional-field: template not converted") } } + +// --- Plugin directory (--plugin-dir) tests --- + +// writeTempYAMLFile writes YAML content to a temp file and returns the file path. +func writeTempYAMLFile(t *testing.T, content string) string { + t.Helper() + dir := t.TempDir() + f, err := os.CreateTemp(dir, "*.yaml") + if err != nil { + t.Fatalf("CreateTemp: %v", err) + } + if _, err := f.WriteString(content); err != nil { + t.Fatalf("WriteString: %v", err) + } + if err := f.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + return f.Name() +} + +// writeTestPluginManifest creates a plugin subdirectory with a plugin.json file. +func writeTestPluginManifest(t *testing.T, pluginsDir, pluginName string, manifest map[string]any) { + t.Helper() + dir := filepath.Join(pluginsDir, pluginName) + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + data, err := json.Marshal(manifest) + if err != nil { + t.Fatalf("json.Marshal: %v", err) + } + if err := os.WriteFile(filepath.Join(dir, "plugin.json"), data, 0o644); err != nil { + t.Fatalf("WriteFile: %v", err) + } +} + +// TestRunModernize_PluginDir_Empty tests --plugin-dir with an empty directory. +func TestRunModernize_PluginDir_Empty(t *testing.T) { + pluginDir := t.TempDir() + cfgFile := writeTempYAMLFile(t, ` +modules: + - name: my-server + type: http.server + config: + address: :8080 +`) + err := runModernize([]string{"--plugin-dir", pluginDir, cfgFile}) + if err != nil { + t.Fatalf("unexpected error with empty plugin dir: %v", err) + } +} + +// TestRunModernize_PluginDir_WithRules tests --plugin-dir with a plugin that +// declares a modernize rule. This simulates an external plugin migration +// scenario where the plugin author has renamed a module type in v2. +func TestRunModernize_PluginDir_WithRules(t *testing.T) { + pluginDir := t.TempDir() + writeTestPluginManifest(t, pluginDir, "test-ext-plugin", map[string]any{ + "name": "test-ext-plugin", + "version": "2.0.0", + "author": "Test", + "description": "External test plugin", + "modernizeRules": []map[string]any{ + { + "id": "ext-rename-type", + "description": "Rename ext.old_module to ext.new_module", + "severity": "error", + "oldModuleType": "ext.old_module", + "newModuleType": "ext.new_module", + }, + }, + }) + + // Write a config that uses the old (deprecated) module type. + cfgFile := writeTempYAMLFile(t, ` +modules: + - name: my-connector + type: ext.old_module + config: + endpoint: https://api.example.com +`) + // Dry-run should succeed and report findings. + err := runModernize([]string{"--plugin-dir", pluginDir, cfgFile}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } +} + +// TestRunModernize_PluginDir_NonexistentDir tests --plugin-dir with a missing +// directory, which should produce an error. +func TestRunModernize_PluginDir_NonexistentDir(t *testing.T) { + cfgFile := writeTempYAMLFile(t, "modules: []\n") + err := runModernize([]string{"--plugin-dir", "/nonexistent/dir/12345", cfgFile}) + if err == nil { + t.Fatal("expected error for nonexistent plugin directory") + } +} + +// TestRunModernize_PluginDir_ListRulesIncludesPluginRules tests that +// --list-rules works when a plugin directory is supplied. +func TestRunModernize_PluginDir_ListRulesIncludesPluginRules(t *testing.T) { + pluginDir := t.TempDir() + writeTestPluginManifest(t, pluginDir, "my-plugin", map[string]any{ + "name": "my-plugin", + "version": "1.0.0", + "author": "Dev", + "description": "My plugin", + "modernizeRules": []map[string]any{ + { + "id": "my-plugin-rename", + "description": "Rename my.old to my.new", + "oldModuleType": "my.old", + "newModuleType": "my.new", + }, + }, + }) + // --list-rules should succeed even when a plugin dir is supplied. + err := runModernize([]string{"--plugin-dir", pluginDir, "--list-rules"}) + if err != nil { + t.Fatalf("unexpected error with plugin dir + list-rules: %v", err) + } +} + +// TestRunModernize_PluginDir_Apply tests that --apply with a plugin rule +// actually fixes the config file in-place. +func TestRunModernize_PluginDir_Apply(t *testing.T) { + pluginDir := t.TempDir() + writeTestPluginManifest(t, pluginDir, "myplugin", map[string]any{ + "name": "myplugin", + "version": "1.0.0", + "author": "Dev", + "description": "Plugin", + "modernizeRules": []map[string]any{ + { + "id": "myplugin-rename", + "description": "Rename myplugin.old to myplugin.new", + "severity": "error", + "oldModuleType": "myplugin.old", + "newModuleType": "myplugin.new", + }, + }, + }) + + cfgFile := writeTempYAMLFile(t, `modules: + - name: x + type: myplugin.old + config: + key: val +`) + if err := runModernize([]string{"--apply", "--plugin-dir", pluginDir, cfgFile}); err != nil { + t.Fatalf("runModernize --apply: %v", err) + } + + // Verify the file was updated. + data, err := os.ReadFile(cfgFile) + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + result := string(data) + if strings.Contains(result, "myplugin.old") { + t.Error("old module type should have been renamed to myplugin.new") + } + if !strings.Contains(result, "myplugin.new") { + t.Error("new module type should appear after --apply") + } +} + +// TestRunModernize_PluginDir_MultiplePlugins tests that rules from multiple +// plugins are all loaded and applied. +func TestRunModernize_PluginDir_MultiplePlugins(t *testing.T) { + pluginDir := t.TempDir() + writeTestPluginManifest(t, pluginDir, "plugin-a", map[string]any{ + "name": "plugin-a", + "version": "1.0.0", + "author": "Dev", + "description": "Plugin A", + "modernizeRules": []map[string]any{ + { + "id": "plugin-a-rule", + "description": "Rename a.old to a.new", + "oldModuleType": "a.old", + "newModuleType": "a.new", + }, + }, + }) + writeTestPluginManifest(t, pluginDir, "plugin-b", map[string]any{ + "name": "plugin-b", + "version": "1.0.0", + "author": "Dev", + "description": "Plugin B", + "modernizeRules": []map[string]any{ + { + "id": "plugin-b-rule", + "description": "Rename step.b_old to step.b_new", + "oldStepType": "step.b_old", + "newStepType": "step.b_new", + }, + }, + }) + + cfgFile := writeTempYAMLFile(t, ` +modules: + - name: conn + type: a.old +pipelines: + main: + steps: + - name: run + type: step.b_old + config: + key: val +`) + // Should report 2 findings (one per plugin rule) without error. + err := runModernize([]string{"--plugin-dir", pluginDir, cfgFile}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } +} diff --git a/docs/PLUGIN_DEVELOPMENT.md b/docs/PLUGIN_DEVELOPMENT.md index 88263ca2..5ad5eaee 100644 --- a/docs/PLUGIN_DEVELOPMENT.md +++ b/docs/PLUGIN_DEVELOPMENT.md @@ -19,6 +19,7 @@ Each plugin implements the `plugin.EnginePlugin` interface and contributes: | Capabilities | `Capabilities()` | Capability contracts this plugin satisfies | | UI schemas | `ModuleSchemas()` | Schema definitions for the workflow builder UI | | Wiring hooks | `WiringHooks()` | Post-init cross-module integration logic | +| Modernize rules | `ModernizeRules()` _(optional)_ | Migration rules for `wfctl modernize` | ## The EnginePlugin Interface @@ -276,29 +277,132 @@ if err := engine.LoadPlugin(pluginmyplugin.New()); err != nil { Also add it to `testhelpers_test.go` → `allPlugins()` for test coverage. +### 11. Declare modernize rules (optional) + +If your plugin introduces or renames module/step types over time, you can declare **modernize rules** so that users can automatically detect and fix stale configs using `wfctl modernize`. + +#### In-process Go plugins + +Implement the optional `plugin.ModernizeRulesProvider` interface: + +```go +import "github.com/GoCodeAlone/workflow/modernize" + +// ModernizeRules returns migration rules for the wfctl modernize command. +func (p *Plugin) ModernizeRules() []modernize.Rule { + return []modernize.Rule{ + modernize.ManifestRule{ + ID: "myplugin-rename-v2", + Description: "Rename myplugin.old_worker to myplugin.worker (v2.0 migration)", + Severity: "error", + OldModuleType: "myplugin.old_worker", + NewModuleType: "myplugin.worker", + }.MustToRule(), + modernize.ManifestRule{ + ID: "myplugin-rename-endpoint", + Description: "Rename apiEndpoint to endpoint in myplugin.worker config", + ModuleType: "myplugin.worker", + OldKey: "apiEndpoint", + NewKey: "endpoint", + }.MustToRule(), + } +} +``` + +Support for consuming `ModernizeRules()` from in-process plugins is not yet wired into the engine; this interface is reserved for future use. You can still write arbitrary Check/Fix functions for complex migrations that go beyond type and key renaming in your rules. + +#### External plugins via `plugin.json` + +External (process-isolated) plugins declare rules in their `plugin.json` manifest under the `modernizeRules` key: + +```json +{ + "name": "my-vendor-plugin", + "version": "2.0.0", + "author": "Vendor Inc.", + "description": "Vendor plugin for workflow", + "moduleTypes": ["vendor.connector"], + "modernizeRules": [ + { + "id": "vendor-rename-type", + "description": "Rename vendor.old_connector to vendor.connector (v2 migration)", + "severity": "error", + "oldModuleType": "vendor.old_connector", + "newModuleType": "vendor.connector" + }, + { + "id": "vendor-rename-key", + "description": "Rename apiEndpoint to endpoint in vendor.connector config", + "moduleType": "vendor.connector", + "oldKey": "apiEndpoint", + "newKey": "endpoint" + }, + { + "id": "vendor-rename-step", + "description": "Rename step.vendor_fetch to step.vendor_get", + "oldStepType": "step.vendor_fetch", + "newStepType": "step.vendor_get" + } + ] +} +``` + +Users with the plugin installed run: + +```bash +wfctl modernize --plugin-dir data/plugins config.yaml +wfctl modernize --apply --plugin-dir data/plugins config.yaml +``` + +#### ManifestRule fields + +| Field | Type | Description | +|-------|------|-------------| +| `id` | string | Required. Unique kebab-case rule identifier | +| `description` | string | Required. Human-readable description | +| `severity` | string | `"error"` or `"warning"` (default: `"warning"`) | +| `message` | string | Override the auto-generated finding message | +| `oldModuleType` | string | Trigger a module type rename: old type to detect | +| `newModuleType` | string | Target module type after rename | +| `oldStepType` | string | Trigger a step type rename: old type to detect | +| `newStepType` | string | Target step type after rename | +| `moduleType` | string | Scope a config key rename to modules of this type | +| `stepType` | string | Scope a config key rename to steps of this type | +| `oldKey` | string | Config key to detect (used with `moduleType` or `stepType`) | +| `newKey` | string | Replacement config key | + +**Exactly one rule kind must be configured per rule:** +- Module type rename: `oldModuleType` + `newModuleType` +- Step type rename: `oldStepType` + `newStepType` +- Module config key rename: `moduleType` + `oldKey` + `newKey` +- Step config key rename: `stepType` + `oldKey` + `newKey` + ## Plugin Manifest The `PluginManifest` struct declares metadata used for discovery, dependency resolution, and the admin UI: ```go type PluginManifest struct { - Name string // unique plugin name (kebab-case) - Version string // semver, e.g. "1.0.0" - Author string // required - Description string // required - Tier PluginTier // TierCore, TierOfficial, TierCommunity - ModuleTypes []string // module types this plugin provides - StepTypes []string // step types this plugin provides - TriggerTypes []string // trigger types this plugin provides - WorkflowTypes []string // workflow handler types - WiringHooks []string // names of wiring hooks - Capabilities []CapabilityDecl // capability declarations - Dependencies []Dependency // plugin dependencies with version constraints + Name string // unique plugin name (kebab-case) + Version string // semver, e.g. "1.0.0" + Author string // required + Description string // required + Tier PluginTier // TierCore, TierOfficial, TierCommunity + ModuleTypes []string // module types this plugin provides + StepTypes []string // step types this plugin provides + TriggerTypes []string // trigger types this plugin provides + WorkflowTypes []string // workflow handler types + WiringHooks []string // names of wiring hooks + Capabilities []CapabilityDecl // capability declarations + Dependencies []Dependency // plugin dependencies with version constraints + ModernizeRules []modernize.ManifestRule // migration rules for wfctl modernize } ``` **All of `Name`, `Version`, `Author`, and `Description` are required** — the plugin loader validates these during `LoadPlugin()`. +The `ModernizeRules` field allows plugins to embed migration rules directly in the Go manifest struct. For external plugins (process-isolated), declare the equivalent rules in `plugin.json` — see [Declare modernize rules](#11-declare-modernize-rules-optional) above. + ## Workflow Dependency Validation Configs can declare required capabilities and plugins: diff --git a/docs/PLUGIN_DEVELOPMENT_GUIDE.md b/docs/PLUGIN_DEVELOPMENT_GUIDE.md index 8682ff6d..9a3a831c 100644 --- a/docs/PLUGIN_DEVELOPMENT_GUIDE.md +++ b/docs/PLUGIN_DEVELOPMENT_GUIDE.md @@ -88,9 +88,66 @@ Every external plugin must include a `plugin.json` file in its directory. This f | `tags` | No | Array of search/categorization tags. | | `repository` | No | URL to the source code repository. | | `dependencies` | No | Array of `{"name": "other-plugin", "constraint": ">=1.0.0"}` objects. | +| `modernizeRules` | No | Array of migration rules for `wfctl modernize` (see below). | The `name` field must match the directory name under `data/plugins/`. Names are validated against the pattern `^[a-z][a-z0-9-]*[a-z0-9]$` (minimum 2 characters) or a single lowercase letter. +### Declaring Modernize Rules + +When you rename module types, step types, or config keys in a new version of your plugin, declare migration rules in `plugin.json` so that users can detect and auto-fix stale configs with `wfctl modernize --plugin-dir data/plugins`. + +```json +{ + "name": "my-plugin", + "version": "2.0.0", + "author": "Your Name", + "description": "My plugin v2", + "moduleTypes": ["my.connector"], + "modernizeRules": [ + { + "id": "my-plugin-rename-type", + "description": "Rename my.old_connector to my.connector (v2 migration)", + "severity": "error", + "oldModuleType": "my.old_connector", + "newModuleType": "my.connector" + }, + { + "id": "my-plugin-rename-key", + "description": "Rename apiEndpoint to endpoint in my.connector config", + "moduleType": "my.connector", + "oldKey": "apiEndpoint", + "newKey": "endpoint" + } + ] +} +``` + +Each rule object supports these fields: + +| Field | Required | Description | +|-------|----------|-------------| +| `id` | Yes | Unique kebab-case rule identifier | +| `description` | Yes | Human-readable description of the migration | +| `severity` | No | `"error"` or `"warning"` (default: `"warning"`) | +| `message` | No | Custom finding message (auto-generated if omitted) | +| `oldModuleType` + `newModuleType` | — | Module type rename rule | +| `oldStepType` + `newStepType` | — | Step type rename rule | +| `moduleType` + `oldKey` + `newKey` | — | Config key rename in a specific module type | +| `stepType` + `oldKey` + `newKey` | — | Config key rename in a specific step type | + +Users run modernize with your plugin directory to apply these rules: + +```bash +# Detect issues (dry-run) +wfctl modernize --plugin-dir data/plugins config.yaml + +# Apply fixes +wfctl modernize --apply --plugin-dir data/plugins config.yaml + +# See all available rules (built-in + plugin) +wfctl modernize --plugin-dir data/plugins --list-rules +``` + ## Directory Layout External plugins live under the `data/plugins/` directory relative to the engine's working directory. Each plugin gets its own subdirectory. The binary inside must have the same name as the directory. @@ -661,3 +718,27 @@ Verify the directory layout matches the expected convention: - Directory name matches the `name` field in `plugin.json` - Binary inside has the same name as the directory - Binary has execute permissions (`chmod +x data/plugins/my-plugin/my-plugin`) + +## Ecosystem Modernization + +### What External Plugin Authors Need to Do + +To participate in the `wfctl modernize` ecosystem, external plugin authors must: + +1. **Add `modernizeRules` to `plugin.json`** whenever a new release renames module/step types or config keys (see [Declaring Modernize Rules](#declaring-modernize-rules) above). + +2. **Ship the updated `plugin.json`** alongside the plugin binary in each release. Users who run `wfctl modernize --plugin-dir data/plugins config.yaml` will automatically pick up the rules from the installed version. + +3. **Include rule history**: Rules should remain in `plugin.json` as long as users may still be running configs with the old names. Only remove a rule when you are confident that no users have configs referencing the old type/key. + +### What the GoCodeAlone/workflow-registry Needs + +The `GoCodeAlone/workflow-registry` repository stores registry manifests (`manifest.json`) for discovery purposes. To surface modernize rules in the registry: + +1. **Extend `RegistryCapabilities`** in the registry manifest format to include a `modernizeRules` summary (e.g., list of rule IDs) so that the registry can inform users "this plugin version includes N migration rules." + +2. **Update `wfctl publish`** to optionally include the `modernizeRules` from `plugin.json` in the registry manifest so that it is available for discovery via `wfctl registry search`. + +3. **Registry manifest convention**: Until the registry manifest format is extended, external plugins should embed their `modernizeRules` in `plugin.json` (the installed copy). The registry entry is informational; the rules are read from the local installed file at modernize time. + +> **Note**: The `wfctl modernize --plugin-dir` flag reads installed `plugin.json` files from disk. It does not fetch rules from the registry. This design keeps the command offline-capable and deterministic. diff --git a/docs/WFCTL.md b/docs/WFCTL.md index 65974d3c..fac6e728 100644 --- a/docs/WFCTL.md +++ b/docs/WFCTL.md @@ -1150,6 +1150,7 @@ wfctl modernize [options] | `--exclude-rules` | none | Comma-separated rule IDs to skip | | `--dir` | | Scan all YAML files recursively | | `--format` | `text` | Output format: text or json | +| `--plugin-dir` | _(none)_ | Directory of installed external plugins; their modernize rules are loaded | **Rules:** @@ -1164,6 +1165,22 @@ wfctl modernize [options] | `empty-routes` | error | no | Detect empty routes in step.conditional | | `camelcase-config` | warning | no | Detect snake_case config keys | +**Plugin-provided rules** are loaded when `--plugin-dir` is specified. Each installed plugin can declare its own migration rules in its `plugin.json` manifest under the `modernizeRules` key. Rules from all plugins in the directory are merged with the built-in rules. + +```bash +# Run built-in rules only +wfctl modernize config.yaml + +# Include migration rules from installed plugins +wfctl modernize --plugin-dir data/plugins config.yaml + +# Apply all fixes (built-in + plugin rules) +wfctl modernize --apply --plugin-dir data/plugins config.yaml + +# List all available rules including those from plugins +wfctl modernize --plugin-dir data/plugins --list-rules +``` + --- ## Project Config File (`.wfctl.yaml`) diff --git a/mcp/wfctl_tools.go b/mcp/wfctl_tools.go index 52d384f5..b7e0fb47 100644 --- a/mcp/wfctl_tools.go +++ b/mcp/wfctl_tools.go @@ -167,7 +167,9 @@ func (s *Server) registerWfctlTools() { mcp.WithDescription("Detect and optionally fix known YAML config anti-patterns. "+ "Reports issues like hyphenated step names, template syntax in conditional fields, "+ "missing db_query mode, dot-access patterns, absolute dbPaths, empty routes, "+ - "and snake_case config keys. Returns findings with line numbers and fix suggestions."), + "and snake_case config keys. When the server was started with a plugin directory, "+ + "plugin-declared modernize rules are automatically included. "+ + "Returns findings with line numbers and fix suggestions."), mcp.WithString("yaml_content", mcp.Required(), mcp.Description("The YAML content of the workflow configuration to analyze"), @@ -1701,6 +1703,24 @@ func (s *Server) handleModernize(_ context.Context, req mcp.CallToolRequest) (*m } rules := modernize.AllRules() + + // Include modernize rules from installed external plugins when a plugin + // directory was configured at server startup. + if s.pluginDir != "" { + pluginRules, err := modernize.LoadRulesFromDir(s.pluginDir) + if err != nil { + // Surface the error as a warning in the result so clients can diagnose + // misconfigured plugin directories rather than silently missing rules. + result := map[string]any{ + "findings": []modernize.Finding{}, + "count": 0, + "plugin_warning": fmt.Sprintf("failed to load plugin modernize rules from %q: %v", s.pluginDir, err), + } + return marshalToolResult(result) + } + rules = append(rules, pluginRules...) + } + rules = modernize.FilterRules(rules, rulesFilter, excludeFilter) // Check phase diff --git a/modernize/manifest_rule.go b/modernize/manifest_rule.go new file mode 100644 index 00000000..ee87c248 --- /dev/null +++ b/modernize/manifest_rule.go @@ -0,0 +1,446 @@ +package modernize + +import ( + "encoding/json" + "fmt" + "os" + "path/filepath" + + "gopkg.in/yaml.v3" +) + +// ManifestRule is a JSON-serializable modernize rule that external plugins can +// declare in their plugin.json manifest under the "modernizeRules" key. It +// supports the most common migration patterns: renaming module types, renaming +// step types, and renaming config keys within a specific module or step type. +// +// Example plugin.json snippet: +// +// { +// "modernizeRules": [ +// { +// "id": "myplugin-rename-type", +// "description": "Rename old.module.type to new.module.type", +// "severity": "error", +// "oldModuleType": "old.module.type", +// "newModuleType": "new.module.type" +// }, +// { +// "id": "myplugin-rename-key", +// "description": "Rename old_key to new_key in my.module config", +// "severity": "warning", +// "moduleType": "my.module", +// "oldKey": "old_key", +// "newKey": "new_key" +// } +// ] +// } +type ManifestRule struct { + // ID is a unique, kebab-case identifier for the rule (e.g., "myplugin-rename-type"). + ID string `json:"id"` + // Description is a human-readable summary of what the rule detects/fixes. + Description string `json:"description"` + // Severity is "error" or "warning" (default: "warning"). + Severity string `json:"severity,omitempty"` + // Message overrides the auto-generated finding message. + Message string `json:"message,omitempty"` + + // OldModuleType and NewModuleType trigger a module type rename rule. + // When set, the rule detects any module with type == OldModuleType + // and, when fixed, renames it to NewModuleType. + OldModuleType string `json:"oldModuleType,omitempty"` + NewModuleType string `json:"newModuleType,omitempty"` + + // OldStepType and NewStepType trigger a step type rename rule. + // When set, the rule detects any pipeline step with type == OldStepType + // and, when fixed, renames it to NewStepType. + OldStepType string `json:"oldStepType,omitempty"` + NewStepType string `json:"newStepType,omitempty"` + + // ModuleType and OldKey/NewKey trigger a module config key rename rule. + // Detects modules of the given type that have OldKey in their config + // and, when fixed, renames the key to NewKey. + ModuleType string `json:"moduleType,omitempty"` + + // StepType and OldKey/NewKey trigger a step config key rename rule. + // Detects steps of the given type that have OldKey in their config + // and, when fixed, renames the key to NewKey. + StepType string `json:"stepType,omitempty"` + + // OldKey is the config key to detect (used with ModuleType or StepType). + OldKey string `json:"oldKey,omitempty"` + // NewKey is the replacement config key (used with ModuleType or StepType). + NewKey string `json:"newKey,omitempty"` +} + +// Validate returns an error if the ManifestRule is misconfigured. +func (mr ManifestRule) Validate() error { + if mr.ID == "" { + return fmt.Errorf("modernize rule: id is required") + } + if mr.Description == "" { + return fmt.Errorf("modernize rule %q: description is required", mr.ID) + } + // Exactly one rule kind must be configured. + kinds := 0 + if mr.OldModuleType != "" { + kinds++ + if mr.NewModuleType == "" { + return fmt.Errorf("modernize rule %q: newModuleType is required when oldModuleType is set", mr.ID) + } + } + if mr.OldStepType != "" { + kinds++ + if mr.NewStepType == "" { + return fmt.Errorf("modernize rule %q: newStepType is required when oldStepType is set", mr.ID) + } + } + if mr.ModuleType != "" && mr.OldKey != "" { + kinds++ + if mr.NewKey == "" { + return fmt.Errorf("modernize rule %q: newKey is required when moduleType + oldKey is set", mr.ID) + } + } + if mr.StepType != "" && mr.OldKey != "" { + kinds++ + if mr.NewKey == "" { + return fmt.Errorf("modernize rule %q: newKey is required when stepType + oldKey is set", mr.ID) + } + } + if kinds == 0 { + return fmt.Errorf("modernize rule %q: must specify at least one of: oldModuleType, oldStepType, or moduleType/stepType + oldKey", mr.ID) + } + if kinds > 1 { + return fmt.Errorf("modernize rule %q: exactly one rule kind must be configured; found %d kinds set", mr.ID, kinds) + } + if mr.Severity != "" && mr.Severity != "error" && mr.Severity != "warning" { + return fmt.Errorf("modernize rule %q: severity must be \"error\" or \"warning\", got %q", mr.ID, mr.Severity) + } + return nil +} + +// ToRule converts a ManifestRule into a Rule with appropriate Check and Fix +// functions. Returns an error if the rule is misconfigured (see Validate). +func (mr ManifestRule) ToRule() (Rule, error) { + if err := mr.Validate(); err != nil { + return Rule{}, err + } + + sev := mr.Severity + if sev == "" { + sev = "warning" + } + + switch { + case mr.OldModuleType != "": + return mr.toModuleTypeRenameRule(sev), nil + case mr.OldStepType != "": + return mr.toStepTypeRenameRule(sev), nil + case mr.ModuleType != "" && mr.OldKey != "": + return mr.toModuleConfigKeyRenameRule(sev), nil + default: // StepType + OldKey + return mr.toStepConfigKeyRenameRule(sev), nil + } +} + +// MustToRule is like ToRule but panics on misconfiguration. It is intended for +// use in plugin initialisation code where a malformed rule is a programming error. +func (mr ManifestRule) MustToRule() Rule { + r, err := mr.ToRule() + if err != nil { + panic(fmt.Sprintf("modernize.ManifestRule.MustToRule: %v", err)) + } + return r +} + +// toModuleTypeRenameRule builds a Rule that detects and renames a module type. +func (mr ManifestRule) toModuleTypeRenameRule(sev string) Rule { + oldType := mr.OldModuleType + newType := mr.NewModuleType + id := mr.ID + msg := mr.Message + if msg == "" { + msg = fmt.Sprintf("Module type %q is deprecated; use %q instead", oldType, newType) + } + return Rule{ + ID: id, + Description: mr.Description, + Severity: sev, + Check: func(root *yaml.Node, _ []byte) []Finding { + var findings []Finding + forEachModule(root, func(mod *yaml.Node) { + typeNode := findMapValue(mod, "type") + if typeNode != nil && typeNode.Kind == yaml.ScalarNode && typeNode.Value == oldType { + findings = append(findings, Finding{ + RuleID: id, + Line: typeNode.Line, + Message: msg, + Fixable: true, + }) + } + }) + return findings + }, + Fix: func(root *yaml.Node) []Change { + var changes []Change + forEachModule(root, func(mod *yaml.Node) { + typeNode := findMapValue(mod, "type") + if typeNode != nil && typeNode.Kind == yaml.ScalarNode && typeNode.Value == oldType { + changes = append(changes, Change{ + RuleID: id, + Line: typeNode.Line, + Description: fmt.Sprintf("Renamed module type %q -> %q", oldType, newType), + }) + typeNode.Value = newType + } + }) + return changes + }, + } +} + +// toStepTypeRenameRule builds a Rule that detects and renames a step type. +func (mr ManifestRule) toStepTypeRenameRule(sev string) Rule { + oldType := mr.OldStepType + newType := mr.NewStepType + id := mr.ID + msg := mr.Message + if msg == "" { + msg = fmt.Sprintf("Step type %q is deprecated; use %q instead", oldType, newType) + } + return Rule{ + ID: id, + Description: mr.Description, + Severity: sev, + Check: func(root *yaml.Node, _ []byte) []Finding { + var findings []Finding + forEachStepOfType(root, oldType, func(step *yaml.Node) { + typeNode := findMapValue(step, "type") + if typeNode != nil { + findings = append(findings, Finding{ + RuleID: id, + Line: typeNode.Line, + Message: msg, + Fixable: true, + }) + } + }) + return findings + }, + Fix: func(root *yaml.Node) []Change { + var changes []Change + forEachStepOfType(root, oldType, func(step *yaml.Node) { + typeNode := findMapValue(step, "type") + if typeNode != nil && typeNode.Kind == yaml.ScalarNode { + changes = append(changes, Change{ + RuleID: id, + Line: typeNode.Line, + Description: fmt.Sprintf("Renamed step type %q -> %q", oldType, newType), + }) + typeNode.Value = newType + } + }) + return changes + }, + } +} + +// toModuleConfigKeyRenameRule builds a Rule that renames a config key in +// modules of a specific type. +func (mr ManifestRule) toModuleConfigKeyRenameRule(sev string) Rule { + moduleType := mr.ModuleType + oldKey := mr.OldKey + newKey := mr.NewKey + id := mr.ID + msg := mr.Message + if msg == "" { + msg = fmt.Sprintf("Config key %q is deprecated in %q modules; use %q instead", oldKey, moduleType, newKey) + } + msgCollision := fmt.Sprintf("Config key %q is deprecated in %q modules (cannot auto-rename: %q already exists)", oldKey, moduleType, newKey) + return Rule{ + ID: id, + Description: mr.Description, + Severity: sev, + Check: func(root *yaml.Node, _ []byte) []Finding { + var findings []Finding + forEachModule(root, func(mod *yaml.Node) { + typeNode := findMapValue(mod, "type") + if typeNode == nil || typeNode.Value != moduleType { + return + } + cfg := findMapValue(mod, "config") + if cfg == nil || cfg.Kind != yaml.MappingNode { + return + } + keyNode := findMapKeyNode(cfg, oldKey) + if keyNode != nil { + // If newKey already exists this is still a finding but not auto-fixable. + fixable := findMapKeyNode(cfg, newKey) == nil + findingMsg := msg + if !fixable { + findingMsg = msgCollision + } + findings = append(findings, Finding{ + RuleID: id, + Line: keyNode.Line, + Message: findingMsg, + Fixable: fixable, + }) + } + }) + return findings + }, + Fix: func(root *yaml.Node) []Change { + var changes []Change + forEachModule(root, func(mod *yaml.Node) { + typeNode := findMapValue(mod, "type") + if typeNode == nil || typeNode.Value != moduleType { + return + } + cfg := findMapValue(mod, "config") + if cfg == nil || cfg.Kind != yaml.MappingNode { + return + } + keyNode := findMapKeyNode(cfg, oldKey) + if keyNode == nil { + return + } + // Skip rename if newKey already exists to avoid duplicate keys. + if findMapKeyNode(cfg, newKey) != nil { + return + } + 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 + }) + return changes + }, + } +} + +// toStepConfigKeyRenameRule builds a Rule that renames a config key in +// steps of a specific type. +func (mr ManifestRule) toStepConfigKeyRenameRule(sev string) Rule { + stepType := mr.StepType + oldKey := mr.OldKey + newKey := mr.NewKey + id := mr.ID + msg := mr.Message + if msg == "" { + msg = fmt.Sprintf("Config key %q is deprecated in %q steps; use %q instead", oldKey, stepType, newKey) + } + msgCollision := fmt.Sprintf("Config key %q is deprecated in %q steps (cannot auto-rename: %q already exists)", oldKey, stepType, newKey) + return Rule{ + ID: id, + Description: mr.Description, + Severity: sev, + Check: func(root *yaml.Node, _ []byte) []Finding { + var findings []Finding + forEachStepOfType(root, stepType, func(step *yaml.Node) { + cfg := findMapValue(step, "config") + if cfg == nil || cfg.Kind != yaml.MappingNode { + return + } + keyNode := findMapKeyNode(cfg, oldKey) + if keyNode != nil { + // If newKey already exists this is still a finding but not auto-fixable. + fixable := findMapKeyNode(cfg, newKey) == nil + findingMsg := msg + if !fixable { + findingMsg = msgCollision + } + findings = append(findings, Finding{ + RuleID: id, + Line: keyNode.Line, + Message: findingMsg, + Fixable: fixable, + }) + } + }) + return findings + }, + Fix: func(root *yaml.Node) []Change { + var changes []Change + forEachStepOfType(root, stepType, func(step *yaml.Node) { + cfg := findMapValue(step, "config") + if cfg == nil || cfg.Kind != yaml.MappingNode { + return + } + keyNode := findMapKeyNode(cfg, oldKey) + if keyNode == nil { + return + } + // Skip rename if newKey already exists to avoid duplicate keys. + if findMapKeyNode(cfg, newKey) != nil { + return + } + 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 + }) + return changes + }, + } +} + +// findMapKeyNode returns the key node (not value node) for a given key in a +// mapping node. Used when the key itself needs to be renamed. +func findMapKeyNode(node *yaml.Node, key string) *yaml.Node { + if node.Kind != yaml.MappingNode { + return nil + } + for i := 0; i+1 < len(node.Content); i += 2 { + if node.Content[i].Value == key { + return node.Content[i] // return the key node, not the value node + } + } + return nil +} + +// pluginManifestModernize is the minimal subset of plugin.json used to load +// modernize rules. It avoids importing the plugin package to prevent cycles. +type pluginManifestModernize struct { + ModernizeRules []ManifestRule `json:"modernizeRules"` +} + +// LoadRulesFromDir scans pluginDir for subdirectories containing a plugin.json +// manifest with a "modernizeRules" field, converts each manifest rule to a Rule, +// and returns the combined slice. Subdirectories with missing or malformed +// manifests are silently skipped. Returns an error only if pluginDir cannot be +// read at all. +func LoadRulesFromDir(pluginDir string) ([]Rule, error) { + entries, err := os.ReadDir(pluginDir) + if err != nil { + return nil, fmt.Errorf("read plugin dir %q: %w", pluginDir, err) + } + + var rules []Rule + for _, e := range entries { + if !e.IsDir() { + continue + } + manifestPath := filepath.Join(pluginDir, e.Name(), "plugin.json") + data, err := os.ReadFile(manifestPath) //nolint:gosec // G304: path is within trusted plugin dir + if err != nil { + continue + } + var m pluginManifestModernize + if err := json.Unmarshal(data, &m); err != nil { + continue // malformed manifests are skipped + } + for i := range m.ModernizeRules { + r, err := m.ModernizeRules[i].ToRule() + if err != nil { + continue // invalid rule descriptors are skipped + } + rules = append(rules, r) + } + } + return rules, nil +} diff --git a/modernize/manifest_rule_test.go b/modernize/manifest_rule_test.go new file mode 100644 index 00000000..3025ca82 --- /dev/null +++ b/modernize/manifest_rule_test.go @@ -0,0 +1,876 @@ +package modernize + +import ( + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" + + "gopkg.in/yaml.v3" +) + +// parseYAMLNode is a test helper that parses YAML into a yaml.Node. +func parseYAMLNode(t *testing.T, input string) *yaml.Node { + t.Helper() + var doc yaml.Node + if err := yaml.Unmarshal([]byte(input), &doc); err != nil { + t.Fatalf("failed to parse YAML: %v", err) + } + return &doc +} + +// TestManifestRule_Validate covers rule validation. +func TestManifestRule_Validate(t *testing.T) { + tests := []struct { + name string + rule ManifestRule + wantErr bool + }{ + { + name: "missing ID", + rule: ManifestRule{Description: "desc", OldModuleType: "a", NewModuleType: "b"}, + wantErr: true, + }, + { + name: "missing Description", + rule: ManifestRule{ID: "x", OldModuleType: "a", NewModuleType: "b"}, + wantErr: true, + }, + { + name: "module type rename missing newModuleType", + rule: ManifestRule{ID: "x", Description: "d", OldModuleType: "a"}, + wantErr: true, + }, + { + name: "step type rename missing newStepType", + rule: ManifestRule{ID: "x", Description: "d", OldStepType: "a"}, + wantErr: true, + }, + { + name: "config key rename missing newKey", + rule: ManifestRule{ID: "x", Description: "d", ModuleType: "my.mod", OldKey: "old"}, + wantErr: true, + }, + { + name: "no rule kind configured", + rule: ManifestRule{ID: "x", Description: "d"}, + wantErr: true, + }, + { + name: "valid module type rename", + rule: ManifestRule{ID: "x", Description: "d", OldModuleType: "a", NewModuleType: "b"}, + wantErr: false, + }, + { + name: "valid step type rename", + rule: ManifestRule{ID: "x", Description: "d", OldStepType: "a", NewStepType: "b"}, + wantErr: false, + }, + { + name: "valid module config key rename", + rule: ManifestRule{ID: "x", Description: "d", ModuleType: "my.mod", OldKey: "old", NewKey: "new"}, + wantErr: false, + }, + { + name: "valid step config key rename", + rule: ManifestRule{ID: "x", Description: "d", StepType: "step.foo", OldKey: "old", NewKey: "new"}, + wantErr: false, + }, + { + name: "multiple rule kinds set (module type + module key rename)", + rule: ManifestRule{ + ID: "x", + Description: "d", + OldModuleType: "a", + NewModuleType: "b", + ModuleType: "a", + OldKey: "old", + NewKey: "new", + }, + wantErr: true, + }, + { + name: "multiple rule kinds set (step type + step key rename)", + rule: ManifestRule{ + ID: "x", + Description: "d", + OldStepType: "step.a", + NewStepType: "step.b", + StepType: "step.a", + OldKey: "old", + NewKey: "new", + }, + wantErr: true, + }, + { + name: "invalid severity value", + rule: ManifestRule{ID: "x", Description: "d", Severity: "critical", OldModuleType: "a", NewModuleType: "b"}, + wantErr: true, + }, + { + name: "valid severity error", + rule: ManifestRule{ID: "x", Description: "d", Severity: "error", OldModuleType: "a", NewModuleType: "b"}, + wantErr: false, + }, + { + name: "valid severity warning", + rule: ManifestRule{ID: "x", Description: "d", Severity: "warning", OldModuleType: "a", NewModuleType: "b"}, + wantErr: false, + }, + { + name: "empty severity (defaults to warning)", + rule: ManifestRule{ID: "x", Description: "d", OldModuleType: "a", NewModuleType: "b"}, + wantErr: false, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + err := tc.rule.Validate() + if (err != nil) != tc.wantErr { + t.Errorf("Validate() error = %v, wantErr = %v", err, tc.wantErr) + } + }) + } +} + +// TestManifestRule_ModuleTypeRename verifies detection and fixing of a module +// type rename rule. +func TestManifestRule_ModuleTypeRename(t *testing.T) { + mr := ManifestRule{ + ID: "test-module-rename", + Description: "Rename old.module to new.module", + Severity: "error", + OldModuleType: "old.module", + NewModuleType: "new.module", + } + + input := ` +modules: + - name: my-server + type: old.module + config: + port: 8080 + - name: other + type: other.module + config: + key: value +` + rule, err := mr.ToRule() + if err != nil { + t.Fatalf("ToRule() error: %v", err) + } + if rule.ID != "test-module-rename" { + t.Errorf("unexpected rule ID: %s", rule.ID) + } + if rule.Severity != "error" { + t.Errorf("unexpected severity: %s", rule.Severity) + } + + doc := parseYAMLNode(t, input) + findings := rule.Check(doc, []byte(input)) + if len(findings) != 1 { + t.Fatalf("expected 1 finding, got %d: %v", len(findings), findings) + } + if findings[0].RuleID != "test-module-rename" { + t.Errorf("unexpected finding RuleID: %s", findings[0].RuleID) + } + if !findings[0].Fixable { + t.Error("finding should be fixable") + } + if !strings.Contains(findings[0].Message, "old.module") { + t.Errorf("message should mention old type, got: %s", findings[0].Message) + } + + // Apply fix. + changes := rule.Fix(doc) + if len(changes) != 1 { + t.Fatalf("expected 1 change, got %d", len(changes)) + } + + out, _ := yaml.Marshal(doc) + result := string(out) + if strings.Contains(result, "old.module") { + t.Error("old.module should have been renamed") + } + if !strings.Contains(result, "new.module") { + t.Error("new.module should appear after rename") + } + // other.module should be untouched + if !strings.Contains(result, "other.module") { + t.Error("other.module should remain") + } +} + +// TestManifestRule_ModuleTypeRename_DefaultSeverity verifies that severity +// defaults to "warning" when not specified. +func TestManifestRule_ModuleTypeRename_DefaultSeverity(t *testing.T) { + mr := ManifestRule{ + ID: "test-default-sev", + Description: "desc", + OldModuleType: "old", + NewModuleType: "new", + } + rule, err := mr.ToRule() + if err != nil { + t.Fatalf("ToRule() error: %v", err) + } + if rule.Severity != "warning" { + t.Errorf("expected default severity 'warning', got %q", rule.Severity) + } +} + +// TestManifestRule_ModuleTypeRename_CustomMessage verifies custom message override. +func TestManifestRule_ModuleTypeRename_CustomMessage(t *testing.T) { + mr := ManifestRule{ + ID: "test-custom-msg", + Description: "desc", + OldModuleType: "old", + NewModuleType: "new", + Message: "Please migrate now!", + } + rule, err := mr.ToRule() + if err != nil { + t.Fatalf("ToRule() error: %v", err) + } + + input := `modules: + - name: x + type: old +` + doc := parseYAMLNode(t, input) + findings := rule.Check(doc, []byte(input)) + if len(findings) != 1 { + t.Fatalf("expected 1 finding, got %d", len(findings)) + } + if findings[0].Message != "Please migrate now!" { + t.Errorf("expected custom message, got: %s", findings[0].Message) + } +} + +// TestManifestRule_StepTypeRename verifies detection and fixing of a step type +// rename rule. +func TestManifestRule_StepTypeRename(t *testing.T) { + mr := ManifestRule{ + ID: "test-step-rename", + Description: "Rename old.step to new.step", + OldStepType: "step.old_action", + NewStepType: "step.new_action", + } + + input := ` +pipelines: + main: + steps: + - name: do_thing + type: step.old_action + config: + key: value + - name: do_other + type: step.keep + config: + key: value +` + rule, err := mr.ToRule() + if err != nil { + t.Fatalf("ToRule() error: %v", err) + } + + doc := parseYAMLNode(t, input) + findings := rule.Check(doc, []byte(input)) + if len(findings) != 1 { + t.Fatalf("expected 1 finding, got %d: %v", len(findings), findings) + } + if !findings[0].Fixable { + t.Error("step type rename should be fixable") + } + + changes := rule.Fix(doc) + if len(changes) != 1 { + t.Fatalf("expected 1 change, got %d", len(changes)) + } + + out, _ := yaml.Marshal(doc) + result := string(out) + if strings.Contains(result, "step.old_action") { + t.Error("old step type should have been renamed") + } + if !strings.Contains(result, "step.new_action") { + t.Error("new step type should appear after rename") + } + // Unaffected step should remain. + if !strings.Contains(result, "step.keep") { + t.Error("step.keep should be unaffected") + } +} + +// TestManifestRule_ModuleConfigKeyRename verifies config key renaming within a +// specific module type. +func TestManifestRule_ModuleConfigKeyRename(t *testing.T) { + mr := ManifestRule{ + ID: "test-module-key", + Description: "Rename old_field to new_field in my.module", + ModuleType: "my.module", + OldKey: "old_field", + NewKey: "new_field", + } + + input := ` +modules: + - name: m1 + type: my.module + config: + old_field: value1 + other_field: value2 + - name: m2 + type: other.module + config: + old_field: should_not_be_touched +` + rule, err := mr.ToRule() + if err != nil { + t.Fatalf("ToRule() error: %v", err) + } + + doc := parseYAMLNode(t, input) + findings := rule.Check(doc, []byte(input)) + if len(findings) != 1 { + t.Fatalf("expected 1 finding (only in my.module), got %d: %v", len(findings), findings) + } + if !findings[0].Fixable { + t.Error("config key rename should be fixable") + } + + changes := rule.Fix(doc) + if len(changes) != 1 { + t.Fatalf("expected 1 change, got %d", len(changes)) + } + + out, _ := yaml.Marshal(doc) + result := string(out) + + // The my.module config should use new_field. + if strings.Contains(result, "old_field: value1") { + t.Error("my.module old_field should have been renamed to new_field") + } + if !strings.Contains(result, "new_field: value1") { + t.Error("new_field with value1 should appear in my.module") + } + // other.module's old_field should be untouched. + if !strings.Contains(result, "old_field: should_not_be_touched") { + t.Error("other.module old_field should remain unchanged") + } +} + +// TestManifestRule_StepConfigKeyRename verifies config key renaming within a +// specific step type. +func TestManifestRule_StepConfigKeyRename(t *testing.T) { + mr := ManifestRule{ + ID: "test-step-key", + Description: "Rename deprecated_field to current_field in step.my_step", + StepType: "step.my_step", + OldKey: "deprecated_field", + NewKey: "current_field", + } + + input := ` +pipelines: + main: + steps: + - name: run_step + type: step.my_step + config: + deprecated_field: hello + - name: other + type: step.other + config: + deprecated_field: should_stay +` + rule, err := mr.ToRule() + if err != nil { + t.Fatalf("ToRule() error: %v", err) + } + + doc := parseYAMLNode(t, input) + findings := rule.Check(doc, []byte(input)) + if len(findings) != 1 { + t.Fatalf("expected 1 finding (only in step.my_step), got %d: %v", len(findings), findings) + } + + changes := rule.Fix(doc) + if len(changes) != 1 { + t.Fatalf("expected 1 change, got %d", len(changes)) + } + + out, _ := yaml.Marshal(doc) + result := string(out) + + if strings.Contains(result, "deprecated_field: hello") { + t.Error("step.my_step deprecated_field should have been renamed") + } + if !strings.Contains(result, "current_field: hello") { + t.Error("current_field should appear after rename") + } + // step.other should be unchanged. + if !strings.Contains(result, "deprecated_field: should_stay") { + t.Error("step.other deprecated_field should remain") + } +} + +// TestManifestRule_NoFindings verifies that rules don't emit false positives. +func TestManifestRule_NoFindings(t *testing.T) { + mr := ManifestRule{ + ID: "no-match", + Description: "Should not match", + OldModuleType: "nonexistent.type", + NewModuleType: "new.type", + } + rule, err := mr.ToRule() + if err != nil { + t.Fatalf("ToRule() error: %v", err) + } + + input := ` +modules: + - name: x + type: http.server + config: + address: :8080 +` + doc := parseYAMLNode(t, input) + findings := rule.Check(doc, []byte(input)) + if len(findings) != 0 { + t.Errorf("expected no findings, got %d", len(findings)) + } + changes := rule.Fix(doc) + if len(changes) != 0 { + t.Errorf("expected no changes, got %d", len(changes)) + } +} + +// TestManifestRule_ModuleConfigKeyRename_Collision verifies that when newKey +// already exists in a module config, the finding is reported as not-fixable and +// the Fix function does not create duplicate keys. +func TestManifestRule_ModuleConfigKeyRename_Collision(t *testing.T) { + mr := ManifestRule{ + ID: "test-collision-mod", + Description: "Rename old_key to new_key in my.module", + ModuleType: "my.module", + OldKey: "old_key", + NewKey: "new_key", + } + rule, err := mr.ToRule() + if err != nil { + t.Fatalf("ToRule() error: %v", err) + } + + // Config has both old_key and new_key — a collision scenario. + input := ` +modules: + - name: m1 + type: my.module + config: + old_key: old_value + new_key: new_value +` + doc := parseYAMLNode(t, input) + findings := rule.Check(doc, []byte(input)) + if len(findings) != 1 { + t.Fatalf("expected 1 finding, got %d", len(findings)) + } + // Finding should NOT be fixable when newKey already exists. + if findings[0].Fixable { + t.Error("finding should not be fixable when newKey already exists (collision)") + } + if !strings.Contains(findings[0].Message, "cannot auto-rename") { + t.Errorf("message should mention collision, got: %s", findings[0].Message) + } + + // Fix should not modify anything. + changes := rule.Fix(doc) + if len(changes) != 0 { + t.Errorf("expected no changes when newKey already exists, got %d", len(changes)) + } + + // YAML should remain unchanged. + out, _ := yaml.Marshal(doc) + result := string(out) + if !strings.Contains(result, "old_key: old_value") { + t.Error("old_key should remain when newKey already exists") + } + if !strings.Contains(result, "new_key: new_value") { + t.Error("new_key should remain unchanged") + } +} + +// TestManifestRule_StepConfigKeyRename_Collision verifies that when newKey +// already exists in a step config, the finding is not-fixable and Fix is a no-op. +func TestManifestRule_StepConfigKeyRename_Collision(t *testing.T) { + mr := ManifestRule{ + ID: "test-collision-step", + Description: "Rename timeout_ms to timeoutMs in step.my_step", + StepType: "step.my_step", + OldKey: "timeout_ms", + NewKey: "timeoutMs", + } + rule, err := mr.ToRule() + if err != nil { + t.Fatalf("ToRule() error: %v", err) + } + + // Both old and new keys are present — collision scenario. + input := ` +pipelines: + main: + steps: + - name: run + type: step.my_step + config: + timeout_ms: 1000 + timeoutMs: 2000 +` + doc := parseYAMLNode(t, input) + findings := rule.Check(doc, []byte(input)) + if len(findings) != 1 { + t.Fatalf("expected 1 finding, got %d", len(findings)) + } + if findings[0].Fixable { + t.Error("finding should not be fixable when newKey already exists (collision)") + } + if !strings.Contains(findings[0].Message, "cannot auto-rename") { + t.Errorf("message should mention collision, got: %s", findings[0].Message) + } + + changes := rule.Fix(doc) + if len(changes) != 0 { + t.Errorf("expected no changes when newKey already exists, got %d", len(changes)) + } +} + +// TestManifestRule_Validate_MultipleKinds verifies that Validate errors when +// more than one rule kind is configured. +func TestManifestRule_Validate_MultipleKinds(t *testing.T) { + mr := ManifestRule{ + ID: "multi-kind", + Description: "Two rule kinds set", + OldModuleType: "a.old", + NewModuleType: "a.new", + ModuleType: "a.old", + OldKey: "foo", + NewKey: "bar", + } + if err := mr.Validate(); err == nil { + t.Error("expected error when multiple rule kinds are configured") + } +} + +// TestManifestRule_Validate_InvalidSeverity verifies that Validate rejects +// unexpected severity values. +func TestManifestRule_Validate_InvalidSeverity(t *testing.T) { + mr := ManifestRule{ + ID: "bad-sev", + Description: "d", + Severity: "critical", + OldModuleType: "a", + NewModuleType: "b", + } + if err := mr.Validate(); err == nil { + t.Error("expected error for invalid severity 'critical'") + } +} + +// TestLoadRulesFromDir_NonexistentDir verifies graceful error on missing directory. +func TestLoadRulesFromDir_NonexistentDir(t *testing.T) { + _, err := LoadRulesFromDir("/nonexistent/plugin/dir") + if err == nil { + t.Error("expected error for nonexistent directory") + } +} + +// TestLoadRulesFromDir_Empty verifies that an empty plugin directory returns +// no rules without error. +func TestLoadRulesFromDir_Empty(t *testing.T) { + dir := t.TempDir() + rules, err := LoadRulesFromDir(dir) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(rules) != 0 { + t.Errorf("expected 0 rules, got %d", len(rules)) + } +} + +// TestLoadRulesFromDir_SkipsMalformedManifest verifies that malformed plugin.json +// files are silently skipped. +func TestLoadRulesFromDir_SkipsMalformedManifest(t *testing.T) { + dir := t.TempDir() + pluginDir := filepath.Join(dir, "bad-plugin") + if err := os.MkdirAll(pluginDir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(pluginDir, "plugin.json"), []byte("not valid json{{"), 0o644); err != nil { + t.Fatal(err) + } + + rules, err := LoadRulesFromDir(dir) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(rules) != 0 { + t.Errorf("expected 0 rules from malformed manifest, got %d", len(rules)) + } +} + +// TestLoadRulesFromDir_LoadsRulesFromManifest simulates an external plugin +// directory with a plugin.json that declares modernize rules, and verifies +// that the rules are loaded and work correctly. +func TestLoadRulesFromDir_LoadsRulesFromManifest(t *testing.T) { + dir := t.TempDir() + + // Simulate an external plugin "my-vendor-plugin" that has migrated its + // module type name and renamed a config field. + pluginDir := filepath.Join(dir, "my-vendor-plugin") + if err := os.MkdirAll(pluginDir, 0o755); err != nil { + t.Fatal(err) + } + + manifest := map[string]any{ + "name": "my-vendor-plugin", + "version": "2.0.0", + "author": "Vendor Inc.", + "description": "My vendor plugin", + "moduleTypes": []string{"vendor.new_connector"}, + "modernizeRules": []map[string]any{ + { + "id": "vendor-rename-module-type", + "description": "Rename vendor.old_connector to vendor.new_connector (v2.0 migration)", + "severity": "error", + "oldModuleType": "vendor.old_connector", + "newModuleType": "vendor.new_connector", + }, + { + "id": "vendor-rename-config-key", + "description": "Rename apiEndpoint to endpoint in vendor.new_connector config", + "moduleType": "vendor.new_connector", + "oldKey": "apiEndpoint", + "newKey": "endpoint", + }, + }, + } + data, err := json.Marshal(manifest) + if err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(pluginDir, "plugin.json"), data, 0o644); err != nil { + t.Fatal(err) + } + + rules, err := LoadRulesFromDir(dir) + if err != nil { + t.Fatalf("LoadRulesFromDir() error: %v", err) + } + if len(rules) != 2 { + t.Fatalf("expected 2 rules, got %d", len(rules)) + } + + // Verify rule IDs. + ruleIDs := map[string]bool{} + for _, r := range rules { + ruleIDs[r.ID] = true + } + if !ruleIDs["vendor-rename-module-type"] { + t.Error("missing rule vendor-rename-module-type") + } + if !ruleIDs["vendor-rename-config-key"] { + t.Error("missing rule vendor-rename-config-key") + } + + // Run the rules against a config that uses the old module type. + input := ` +modules: + - name: my-connector + type: vendor.old_connector + config: + apiEndpoint: https://api.example.com +` + doc := parseYAMLNode(t, input) + var findings []Finding + for _, r := range rules { + findings = append(findings, r.Check(doc, []byte(input))...) + } + // Expect 1 finding: old module type (the config key rule won't fire because + // the module still has the old type — type matches new_connector, not old). + if len(findings) != 1 { + t.Errorf("expected 1 finding for old module type, got %d: %v", len(findings), findings) + } + if findings[0].RuleID != "vendor-rename-module-type" { + t.Errorf("expected vendor-rename-module-type finding, got %s", findings[0].RuleID) + } +} + +// TestLoadRulesFromDir_MultiplePlugins verifies that rules from multiple plugins +// are all collected. +func TestLoadRulesFromDir_MultiplePlugins(t *testing.T) { + dir := t.TempDir() + + // Plugin A: one module type rename rule. + pluginA := filepath.Join(dir, "plugin-a") + if err := os.MkdirAll(pluginA, 0o755); err != nil { + t.Fatal(err) + } + manifestA := map[string]any{ + "name": "plugin-a", + "version": "1.0.0", + "author": "Dev", + "description": "Plugin A", + "modernizeRules": []map[string]any{ + { + "id": "plugin-a-rule", + "description": "Plugin A rename", + "oldModuleType": "a.old", + "newModuleType": "a.new", + }, + }, + } + writeJSON(t, filepath.Join(pluginA, "plugin.json"), manifestA) + + // Plugin B: one step type rename rule. + pluginB := filepath.Join(dir, "plugin-b") + if err := os.MkdirAll(pluginB, 0o755); err != nil { + t.Fatal(err) + } + manifestB := map[string]any{ + "name": "plugin-b", + "version": "1.0.0", + "author": "Dev", + "description": "Plugin B", + "modernizeRules": []map[string]any{ + { + "id": "plugin-b-rule", + "description": "Plugin B step rename", + "oldStepType": "step.b_old", + "newStepType": "step.b_new", + }, + }, + } + writeJSON(t, filepath.Join(pluginB, "plugin.json"), manifestB) + + rules, err := LoadRulesFromDir(dir) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(rules) != 2 { + t.Errorf("expected 2 rules from 2 plugins, got %d", len(rules)) + } +} + +// TestLoadRulesFromDir_SkipsInvalidRules verifies that invalid rule descriptors +// within an otherwise-valid manifest are silently skipped. +func TestLoadRulesFromDir_SkipsInvalidRules(t *testing.T) { + dir := t.TempDir() + pluginDir := filepath.Join(dir, "bad-rules-plugin") + if err := os.MkdirAll(pluginDir, 0o755); err != nil { + t.Fatal(err) + } + manifest := map[string]any{ + "name": "bad-rules-plugin", + "version": "1.0.0", + "author": "Dev", + "description": "Plugin with an invalid rule", + "modernizeRules": []map[string]any{ + // Missing newModuleType — should be skipped. + { + "id": "bad-rule", + "description": "Incomplete rule", + "oldModuleType": "a.type", + }, + // Valid rule. + { + "id": "good-rule", + "description": "Complete rule", + "oldModuleType": "b.old", + "newModuleType": "b.new", + }, + }, + } + writeJSON(t, filepath.Join(pluginDir, "plugin.json"), manifest) + + rules, err := LoadRulesFromDir(dir) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + // Only the valid rule should be loaded. + if len(rules) != 1 { + t.Errorf("expected 1 valid rule, got %d", len(rules)) + } + if rules[0].ID != "good-rule" { + t.Errorf("expected good-rule, got %s", rules[0].ID) + } +} + +// TestLoadRulesFromDir_NoModernizeRulesField verifies that a plugin.json without +// a modernizeRules field contributes 0 rules without error. +func TestLoadRulesFromDir_NoModernizeRulesField(t *testing.T) { + dir := t.TempDir() + pluginDir := filepath.Join(dir, "standard-plugin") + if err := os.MkdirAll(pluginDir, 0o755); err != nil { + t.Fatal(err) + } + manifest := map[string]any{ + "name": "standard-plugin", + "version": "1.0.0", + "author": "Dev", + "description": "A plugin without modernize rules", + "moduleTypes": []string{"my.type"}, + } + writeJSON(t, filepath.Join(dir, "standard-plugin", "plugin.json"), manifest) + + rules, err := LoadRulesFromDir(dir) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(rules) != 0 { + t.Errorf("expected 0 rules for plugin without modernizeRules, got %d", len(rules)) + } +} + +// TestManifestRule_JSONRoundTrip verifies that ManifestRule serialises and +// deserialises correctly. +func TestManifestRule_JSONRoundTrip(t *testing.T) { + original := ManifestRule{ + ID: "my-rule", + Description: "Rename old to new", + Severity: "error", + Message: "Please update", + OldModuleType: "my.old", + NewModuleType: "my.new", + } + data, err := json.Marshal(original) + if err != nil { + t.Fatalf("json.Marshal error: %v", err) + } + var decoded ManifestRule + if err := json.Unmarshal(data, &decoded); err != nil { + t.Fatalf("json.Unmarshal error: %v", err) + } + if decoded.ID != original.ID || + decoded.Description != original.Description || + decoded.Severity != original.Severity || + decoded.Message != original.Message || + decoded.OldModuleType != original.OldModuleType || + decoded.NewModuleType != original.NewModuleType { + t.Errorf("round-trip mismatch: got %+v, want %+v", decoded, original) + } +} + +// writeJSON is a test helper to write a JSON-marshalled value to a file. +func writeJSON(t *testing.T, path string, v any) { + t.Helper() + data, err := json.Marshal(v) + if err != nil { + t.Fatalf("json.Marshal: %v", err) + } + if err := os.WriteFile(path, data, 0o644); err != nil { + t.Fatalf("WriteFile %s: %v", path, err) + } +} diff --git a/module/pipeline_step_graphql.go b/module/pipeline_step_graphql.go index 77cccc3f..a42aa2a1 100644 --- a/module/pipeline_step_graphql.go +++ b/module/pipeline_step_graphql.go @@ -316,8 +316,9 @@ func (s *GraphQLStep) Execute(ctx context.Context, pc *PipelineContext) (*StepRe // Execute request (with optional network error retry) output, statusCode, err := s.doRequest(ctx, resolvedURL, reqBody, bearerToken) if err != nil { - // 401 retry with token refresh - if statusCode == http.StatusUnauthorized && s.auth != nil { + switch { + case statusCode == http.StatusUnauthorized && s.auth != nil: + // 401 retry with token refresh s.oauthEntry.invalidate() bearerToken, err = s.fetchTokenDirect(ctx) if err != nil { @@ -327,13 +328,13 @@ func (s *GraphQLStep) Execute(ctx context.Context, pc *PipelineContext) (*StepRe if err != nil { return nil, err } - } else if s.retryOnNetworkError && statusCode == 0 { + case s.retryOnNetworkError && statusCode == 0: // Network-level error (no HTTP response) — retry once output, statusCode, err = s.doRequest(ctx, resolvedURL, reqBody, bearerToken) if err != nil { return nil, err } - } else { + default: return nil, err } } diff --git a/plugin/engine_plugin.go b/plugin/engine_plugin.go index b1695300..0b9ecf3c 100644 --- a/plugin/engine_plugin.go +++ b/plugin/engine_plugin.go @@ -7,6 +7,7 @@ import ( "github.com/GoCodeAlone/workflow/capability" "github.com/GoCodeAlone/workflow/config" "github.com/GoCodeAlone/workflow/deploy" + "github.com/GoCodeAlone/workflow/modernize" "github.com/GoCodeAlone/workflow/schema" ) @@ -107,6 +108,22 @@ type PipelineTriggerConfigProvider interface { PipelineTriggerConfigWrappers() map[string]TriggerConfigWrapperFunc } +// ModernizeRulesProvider is optionally implemented by EnginePlugins that +// wish to supply custom modernize rules for the wfctl modernize command. +// The rules returned by this interface are intended to be collected by the +// engine or tooling and merged with the core built-in rules when +// modernizing workflow configs that use this plugin's module/step types. +// +// External (process-isolated) plugins declare their rules in plugin.json +// under the "modernizeRules" key and have them loaded automatically via +// modernize.LoadRulesFromDir. In-process Go plugins can implement this +// interface to expose function-based rules that perform arbitrary YAML +// transforms. At present this interface serves as an API/extension point; +// integrating it into a particular engine or CLI is up to that consumer. +type ModernizeRulesProvider interface { + ModernizeRules() []modernize.Rule +} + // NativePluginProvider is optionally implemented by EnginePlugins that also // contribute NativePlugins (e.g., for Marketplace visibility, UI pages, or // HTTP route handlers). The PluginContext provides shared resources (DB, logger). diff --git a/plugin/manifest.go b/plugin/manifest.go index ea0f801e..13ef66bc 100644 --- a/plugin/manifest.go +++ b/plugin/manifest.go @@ -9,6 +9,7 @@ import ( "strings" "github.com/GoCodeAlone/workflow/dynamic" + "github.com/GoCodeAlone/workflow/modernize" "github.com/GoCodeAlone/workflow/schema" ) @@ -49,6 +50,13 @@ type PluginManifest struct { // Used by MCP/LSP for hover docs, completions, and output documentation. StepSchemas []*schema.StepSchema `json:"stepSchemas,omitempty" yaml:"stepSchemas,omitempty"` + // ModernizeRules declares migration rules for the wfctl modernize command. + // Each rule describes a common migration pattern (type renames, config key + // renames) that users of this plugin may need to apply when upgrading. + // These rules are loaded automatically when --plugin-dir is passed to + // wfctl modernize or wfctl mcp. + ModernizeRules []modernize.ManifestRule `json:"modernizeRules,omitempty" yaml:"modernizeRules,omitempty"` + // OverridableTypes lists type names (modules, steps, triggers, handlers) that may be // overridden by later-loaded plugins without requiring LoadPluginWithOverride. // Typically used for placeholder/mock implementations.