[WIP] Add validation for external plugins in strict mode#206
Conversation
…idator Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>
|
@copilot resume where you left off. Ensure your work was completed satisfactorily. |
…ve conflicts keeping all features Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>
Rebased on main (merged 3 commits from origin/main) in commit |
There was a problem hiding this comment.
Pull request overview
This pull request adds comprehensive validation support for external plugins in strict mode, along with enhanced snake_case/camelCase detection to help users avoid common configuration errors. The changes enable wfctl to load external plugin type definitions from a plugin directory and recognize them during validation, preventing false "unknown type" errors.
Changes:
- Added
LoadPluginTypesFromDirfunction to dynamically load plugin type declarations from external plugin manifests - Added
--plugin-dirflag to bothwfctl validateandwfctl template validatecommands to support external plugin validation - Implemented
CamelToSnakehelper and snake_case detection logic to provide helpful error messages when users accidentally use snake_case instead of camelCase in config fields - Enhanced type registry functions to merge dynamically loaded plugin types with built-in types
- Added comprehensive test coverage for plugin loading and snake_case detection
Reviewed changes
Copilot reviewed 9 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| schema/validate.go | Added CamelToSnake helper function and snake_case detection logic in validateModuleConfig to provide helpful hints when config fields use snake_case instead of camelCase |
| schema/schema.go | Implemented LoadPluginTypesFromDir to scan plugin directories and register external plugin types dynamically |
| schema/schema_test.go | Added comprehensive tests for CamelToSnake function, snake_case detection, and LoadPluginTypesFromDir with proper cleanup patterns |
| cmd/wfctl/validate.go | Added --plugin-dir flag and integrated plugin loading before validation; enhanced reorderFlags to handle new flag |
| cmd/wfctl/template_validate.go | Added --plugin-dir flag and enhanced snake_case detection for both modules and pipeline steps |
| cmd/wfctl/type_registry.go | Modified type registry functions to merge dynamic types from schema package with static built-in types |
| cmd/wfctl/template_validate_test.go | Added tests for snake_case warnings and --plugin-dir flag functionality |
| cmd/wfctl/main_test.go | Added test for --plugin-dir flag with snake_case config validation |
| schema/module_schema.go | Fixed formatting alignment for Outputs field in step.base64_decode schema |
| cmd/wfctl/registry.go | Fixed struct field alignment formatting |
| cmd/wfctl/multi_registry_test.go | Fixed map literal formatting alignment |
| cmd/wfctl/diff.go | Fixed struct field alignment formatting |
| cmd/wfctl/contract.go | Fixed struct field and constant alignment formatting |
| cmd/wfctl/compat.go | Fixed struct field alignment formatting |
| cmd/wfctl/registry_validate.go | Removed trailing newline |
| func LoadPluginTypesFromDir(pluginDir string) error { | ||
| entries, err := os.ReadDir(pluginDir) | ||
| if err != nil { | ||
| return fmt.Errorf("read plugin dir %q: %w", pluginDir, err) | ||
| } | ||
| 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 the trusted plugins directory | ||
| if err != nil { | ||
| continue | ||
| } | ||
| var m pluginManifestTypes | ||
| if err := json.Unmarshal(data, &m); err != nil { | ||
| continue | ||
| } | ||
| for _, t := range m.ModuleTypes { | ||
| RegisterModuleType(t) | ||
| } | ||
| for _, t := range m.StepTypes { | ||
| // Step types share the module type registry (identified by "step." prefix). | ||
| RegisterModuleType(t) | ||
| } | ||
| for _, t := range m.TriggerTypes { | ||
| RegisterTriggerType(t) | ||
| } | ||
| for _, t := range m.WorkflowTypes { | ||
| RegisterWorkflowType(t) | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The LoadPluginTypesFromDir function silently skips malformed plugin.json files (lines 355-356) and missing plugin.json files (lines 351-352). While this is documented as expected behavior ("Unknown or malformed manifests are silently skipped"), consider whether this could hide configuration errors that users should be aware of.
If a user expects a plugin to be loaded but it fails due to malformed JSON or a missing manifest, they won't receive any feedback about why their plugin types aren't being recognized. This could lead to confusing "unknown type" validation errors later.
Consider adding optional debug logging or returning a list of skipped manifests with reasons, especially since this is invoked by explicit user action (--plugin-dir flag). At minimum, the docstring could clarify that errors are silently skipped to help users understand the behavior when debugging.
| func CamelToSnake(s string) string { | ||
| var b strings.Builder | ||
| for i, r := range s { | ||
| if i > 0 && unicode.IsUpper(r) { | ||
| b.WriteByte('_') | ||
| } | ||
| b.WriteRune(unicode.ToLower(r)) | ||
| } | ||
| return b.String() | ||
| } |
There was a problem hiding this comment.
The CamelToSnake function does not handle empty strings or single-character strings correctly in edge cases. For an empty string, it will return an empty string without issue. However, consider adding test coverage for edge cases like:
- Empty string: "" (currently returns "" correctly)
- Single uppercase letter: "A" (returns "a")
- Already snake_case with leading underscore: "_private" (returns "_private")
- Mixed case with numbers: "http2Protocol" (returns "http2_protocol")
The current implementation works correctly for the common cases shown in tests, but documenting these edge case behaviors would improve maintainability.
| func TestRunTemplateValidatePluginDir(t *testing.T) { | ||
| pluginsDir := t.TempDir() | ||
| // Create a fake plugin with a custom module type | ||
| pluginSubdir := filepath.Join(pluginsDir, "my-external-plugin") | ||
| if err := os.MkdirAll(pluginSubdir, 0755); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| manifest := `{"moduleTypes": ["custom.external.module"]}` | ||
| if err := os.WriteFile(filepath.Join(pluginSubdir, "plugin.json"), []byte(manifest), 0644); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| // Config that uses the external plugin module type | ||
| dir := t.TempDir() | ||
| configContent := ` | ||
| modules: | ||
| - name: ext-mod | ||
| type: custom.external.module | ||
| ` | ||
| configPath := filepath.Join(dir, "workflow.yaml") | ||
| if err := os.WriteFile(configPath, []byte(configContent), 0644); err != nil { | ||
| t.Fatalf("failed to write test config: %v", err) | ||
| } | ||
|
|
||
| // Without --plugin-dir: should fail with unknown type | ||
| err := runTemplateValidate([]string{"-config", configPath}) | ||
| if err == nil { | ||
| t.Fatal("expected error for unknown external module type without --plugin-dir") | ||
| } | ||
|
|
||
| // With --plugin-dir: should pass | ||
| if err := runTemplateValidate([]string{"-plugin-dir", pluginsDir, "-config", configPath}); err != nil { | ||
| t.Errorf("expected valid config with --plugin-dir, got: %v", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Tests that call LoadPluginTypesFromDir should use t.Cleanup to unregister the dynamically loaded types to prevent test pollution. The types are registered globally and persist across tests if not cleaned up.
The proper pattern is demonstrated in schema/schema_test.go:891-895:
t.Cleanup(func() {
UnregisterModuleType(customModuleType)
UnregisterTriggerType(customTriggerType)
UnregisterWorkflowType(customWorkflowType)
})Without cleanup, types registered in this test will remain visible to subsequent tests that call KnownModuleTypes(), KnownTriggerTypes(), or KnownWorkflowTypes(), potentially causing unexpected behavior or test failures.
| func TestRunValidatePluginDir(t *testing.T) { | ||
| // Create a fake external plugin directory with a plugin.json declaring a custom module type. | ||
| pluginsDir := t.TempDir() | ||
| pluginSubdir := filepath.Join(pluginsDir, "my-ext-plugin") | ||
| if err := os.MkdirAll(pluginSubdir, 0755); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| manifest := `{"moduleTypes": ["custom.ext.validate.testonly"]}` | ||
| if err := os.WriteFile(filepath.Join(pluginSubdir, "plugin.json"), []byte(manifest), 0644); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| // Config using the external plugin module type | ||
| dir := t.TempDir() | ||
| configContent := ` | ||
| modules: | ||
| - name: ext-mod | ||
| type: custom.ext.validate.testonly | ||
| ` | ||
| path := writeTestConfig(t, dir, "workflow.yaml", configContent) | ||
|
|
||
| // Without --plugin-dir: should fail (unknown type) | ||
| if err := runValidate([]string{path}); err == nil { | ||
| t.Fatal("expected error for unknown external module type without --plugin-dir") | ||
| } | ||
|
|
||
| // With --plugin-dir: should pass | ||
| if err := runValidate([]string{"--plugin-dir", pluginsDir, path}); err != nil { | ||
| t.Errorf("expected valid config with --plugin-dir, got: %v", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Tests that call LoadPluginTypesFromDir via runValidate should use t.Cleanup to unregister the dynamically loaded types to prevent test pollution. The types are registered globally and persist across tests if not cleaned up.
The proper pattern is demonstrated in schema/schema_test.go:891-895:
t.Cleanup(func() {
schema.UnregisterModuleType(customModuleType)
schema.UnregisterTriggerType(customTriggerType)
schema.UnregisterWorkflowType(customWorkflowType)
})Without cleanup, types registered in this test will remain visible to subsequent tests, potentially causing unexpected behavior or test failures.
| func CamelToSnake(s string) string { | ||
| var b strings.Builder | ||
| for i, r := range s { | ||
| if i > 0 && unicode.IsUpper(r) { | ||
| b.WriteByte('_') | ||
| } | ||
| b.WriteRune(unicode.ToLower(r)) | ||
| } |
There was a problem hiding this comment.
The CamelToSnake function handles consecutive uppercase letters by inserting an underscore before each uppercase letter (e.g., "webhookURL" becomes "webhook_u_r_l"). While this is technically correct behavior and is tested, it may not match user expectations for acronyms.
Many camelCase conventions treat acronyms as a single unit (e.g., "webhookURL" might be expected to convert to "webhook_url" rather than "webhook_u_r_l"). Consider whether the current behavior aligns with the config field naming conventions used in the codebase. If users typically write "webhookURL" but expect hints about "webhook_url", this could cause confusion.
However, if config fields never use multi-letter acronyms in this way, this is a non-issue. Review the actual config field names in ModuleSchemaRegistry to verify this edge case doesn't occur in practice.
| func CamelToSnake(s string) string { | |
| var b strings.Builder | |
| for i, r := range s { | |
| if i > 0 && unicode.IsUpper(r) { | |
| b.WriteByte('_') | |
| } | |
| b.WriteRune(unicode.ToLower(r)) | |
| } | |
| // It treats consecutive uppercase letters (acronyms) as a single word: | |
| // "webhookURL" → "webhook_url", "HTTPRequest" → "http_request". | |
| func CamelToSnake(s string) string { | |
| var b strings.Builder | |
| runes := []rune(s) | |
| for i, r := range runes { | |
| if i > 0 && unicode.IsUpper(r) { | |
| prev := runes[i-1] | |
| var next rune | |
| if i+1 < len(runes) { | |
| next = runes[i+1] | |
| } | |
| // Insert underscore at word boundaries: | |
| // 1. lower/digit → upper (e.g., "dbPath" → "db_path") | |
| // 2. acronym → word, before the last upper (e.g., "HTTPRequest" → "http_request") | |
| if unicode.IsLower(prev) || unicode.IsDigit(prev) || | |
| (unicode.IsUpper(prev) && (next == 0 || unicode.IsLower(next))) { | |
| b.WriteByte('_') | |
| } | |
| } | |
| b.WriteRune(unicode.ToLower(r)) | |
| } |
|
@copilot apply changes based on the comments in this thread tests failing, linter failing. |
…t cleanup, example config keys Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>
Applied all review feedback in commit
|
LoadPluginTypesFromDirtoschema/schema.goto load external plugin manifests--plugin-dirflag towfctl validateto load plugins before validation--plugin-dirflag towfctl template validateto load plugins before validationCamelToSnakehelper toschema/validate.goCamelToSnaketo handle acronyms correctly (webhookURL → webhook_url, HTTPRequest → http_request)schema/validate.go'svalidateModuleConfigcmd/wfctl/template_validate.go'svalidateWorkflowConfigKnownModuleTypes()/KnownStepTypes()/KnownTriggerTypes()in type_registry.go to merge dynamic schema typest.Cleanupto prevent test pollution)example/ui-build-and-serve.yamlcorrected to use camelCase keys (spaFallback,cacheMaxAge)t.CleanupinTestRunTemplateValidatePluginDirto unregister plugin typest.CleanupinTestRunValidatePluginDirto unregister plugin types💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.