Skip to content

Commit 929d97b

Browse files
Copilotintel352
andauthored
[WIP] Add validation for external plugins in strict mode (#206)
* Initial plan * Add external plugin support and snake_case/camelCase detection to validator Co-authored-by: intel352 <77607+intel352@users.noreply.github.com> * Merge origin/main into copilot/add-external-plugins-validation, resolve conflicts keeping all features Co-authored-by: intel352 <77607+intel352@users.noreply.github.com> * Fix CI failures and apply review feedback: CamelToSnake acronyms, test cleanup, example config keys Co-authored-by: intel352 <77607+intel352@users.noreply.github.com> * Co-authored-by: intel352 <77607+intel352@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: intel352 <77607+intel352@users.noreply.github.com> Co-authored-by: Jonathan Langevin <codingsloth@pm.me>
1 parent 507ff2a commit 929d97b

18 files changed

Lines changed: 580 additions & 116 deletions

cmd/wfctl/compat.go

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,11 +21,11 @@ type CompatibilityInfo struct {
2121

2222
// compatCheckResult holds the result of a compatibility check.
2323
type compatCheckResult struct {
24-
EngineVersion string `json:"engineVersion"`
25-
RequiredModules []compatItem `json:"requiredModules"`
26-
RequiredSteps []compatItem `json:"requiredSteps"`
27-
Compatible bool `json:"compatible"`
28-
Issues []string `json:"issues,omitempty"`
24+
EngineVersion string `json:"engineVersion"`
25+
RequiredModules []compatItem `json:"requiredModules"`
26+
RequiredSteps []compatItem `json:"requiredSteps"`
27+
Compatible bool `json:"compatible"`
28+
Issues []string `json:"issues,omitempty"`
2929
}
3030

3131
// compatItem represents a single required type and whether it's available.

cmd/wfctl/contract.go

Lines changed: 15 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -15,9 +15,9 @@ import (
1515

1616
// Contract is a snapshot of what a workflow application config exposes.
1717
type Contract struct {
18-
Version string `json:"version"`
19-
ConfigHash string `json:"configHash"`
20-
GeneratedAt string `json:"generatedAt"`
18+
Version string `json:"version"`
19+
ConfigHash string `json:"configHash"`
20+
GeneratedAt string `json:"generatedAt"`
2121
Endpoints []EndpointContract `json:"endpoints"`
2222
Modules []ModuleContract `json:"modules"`
2323
Steps []string `json:"steps"`
@@ -59,25 +59,25 @@ type contractComparison struct {
5959
type changeType string
6060

6161
const (
62-
changeAdded changeType = "ADDED"
63-
changeRemoved changeType = "REMOVED"
64-
changeChanged changeType = "CHANGED"
62+
changeAdded changeType = "ADDED"
63+
changeRemoved changeType = "REMOVED"
64+
changeChanged changeType = "CHANGED"
6565
changeUnchanged changeType = "UNCHANGED"
6666
)
6767

6868
type endpointChange struct {
69-
Method string
70-
Path string
71-
Pipeline string
72-
Change changeType
73-
Detail string
74-
IsBreaking bool
69+
Method string
70+
Path string
71+
Pipeline string
72+
Change changeType
73+
Detail string
74+
IsBreaking bool
7575
}
7676

7777
type moduleChange struct {
78-
Name string
79-
Type string
80-
Change changeType
78+
Name string
79+
Type string
80+
Change changeType
8181
}
8282

8383
type eventChange struct {

cmd/wfctl/diff.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,7 @@ type PipelineDiff struct {
112112

113113
// BreakingChangeSummary aggregates breaking-change warnings across the diff.
114114
type BreakingChangeSummary struct {
115-
ModuleName string `json:"moduleName"`
115+
ModuleName string `json:"moduleName"`
116116
Changes []BreakingChange `json:"changes"`
117117
}
118118

cmd/wfctl/main_test.go

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@ import (
66
"path/filepath"
77
"strings"
88
"testing"
9+
10+
"github.com/GoCodeAlone/workflow/schema"
911
)
1012

1113
func writeTestConfig(t *testing.T, dir, name, content string) string {
@@ -185,6 +187,33 @@ modules:
185187
}
186188
}
187189

190+
func TestRunValidateSnakeCaseConfig(t *testing.T) {
191+
dir := t.TempDir()
192+
// "content_type" is the snake_case form of the known camelCase field "contentType"
193+
snakeCaseConfig := `
194+
modules:
195+
- name: handler
196+
type: http.handler
197+
config:
198+
content_type: "application/json"
199+
triggers:
200+
http:
201+
port: 8080
202+
`
203+
path := writeTestConfig(t, dir, "snake.yaml", snakeCaseConfig)
204+
// validateFile returns the detailed error; runValidate returns a summary
205+
err := validateFile(path, false, false, false)
206+
if err == nil {
207+
t.Fatal("expected error for snake_case config field")
208+
}
209+
if !strings.Contains(err.Error(), "content_type") {
210+
t.Errorf("expected error to mention snake_case field 'content_type', got: %v", err)
211+
}
212+
if !strings.Contains(err.Error(), "contentType") {
213+
t.Errorf("expected error to suggest camelCase 'contentType', got: %v", err)
214+
}
215+
}
216+
188217
func TestRunPluginMissingSubcommand(t *testing.T) {
189218
err := runPlugin([]string{})
190219
if err == nil {
@@ -255,3 +284,38 @@ func TestRunPluginDocsMissingDir(t *testing.T) {
255284
t.Fatal("expected error when no directory given")
256285
}
257286
}
287+
288+
func TestRunValidatePluginDir(t *testing.T) {
289+
// Create a fake external plugin directory with a plugin.json declaring a custom module type.
290+
pluginsDir := t.TempDir()
291+
pluginSubdir := filepath.Join(pluginsDir, "my-ext-plugin")
292+
if err := os.MkdirAll(pluginSubdir, 0755); err != nil {
293+
t.Fatal(err)
294+
}
295+
manifest := `{"moduleTypes": ["custom.ext.validate.testonly"]}`
296+
if err := os.WriteFile(filepath.Join(pluginSubdir, "plugin.json"), []byte(manifest), 0644); err != nil {
297+
t.Fatal(err)
298+
}
299+
300+
// Config using the external plugin module type
301+
dir := t.TempDir()
302+
configContent := `
303+
modules:
304+
- name: ext-mod
305+
type: custom.ext.validate.testonly
306+
`
307+
path := writeTestConfig(t, dir, "workflow.yaml", configContent)
308+
309+
// Without --plugin-dir: should fail (unknown type)
310+
if err := runValidate([]string{path}); err == nil {
311+
t.Fatal("expected error for unknown external module type without --plugin-dir")
312+
}
313+
314+
// With --plugin-dir: should pass
315+
if err := runValidate([]string{"--plugin-dir", pluginsDir, path}); err != nil {
316+
t.Errorf("expected valid config with --plugin-dir, got: %v", err)
317+
}
318+
t.Cleanup(func() {
319+
schema.UnregisterModuleType("custom.ext.validate.testonly")
320+
})
321+
}

cmd/wfctl/multi_registry_test.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -307,7 +307,7 @@ func TestMultiRegistryFetchPriority(t *testing.T) {
307307
func TestMultiRegistryFetchFallback(t *testing.T) {
308308
// Source A errors for "unique-plugin", source B has it.
309309
srcA := &mockRegistrySource{
310-
name: "primary",
310+
name: "primary",
311311
manifests: map[string]*RegistryManifest{},
312312
fetchErr: map[string]error{
313313
"unique-plugin": fmt.Errorf("not found"),
@@ -439,14 +439,14 @@ func TestMultiRegistryListDedup(t *testing.T) {
439439
srcA := &mockRegistrySource{
440440
name: "primary",
441441
manifests: map[string]*RegistryManifest{
442-
"shared": {Name: "shared"},
442+
"shared": {Name: "shared"},
443443
"only-in-a": {Name: "only-in-a"},
444444
},
445445
}
446446
srcB := &mockRegistrySource{
447447
name: "secondary",
448448
manifests: map[string]*RegistryManifest{
449-
"shared": {Name: "shared"},
449+
"shared": {Name: "shared"},
450450
"only-in-b": {Name: "only-in-b"},
451451
},
452452
}

cmd/wfctl/registry.go

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -17,21 +17,21 @@ const (
1717

1818
// RegistryManifest is the manifest format for the GoCodeAlone/workflow-registry.
1919
type RegistryManifest struct {
20-
Name string `json:"name"`
21-
Version string `json:"version"`
22-
Author string `json:"author"`
23-
Description string `json:"description"`
24-
Source string `json:"source,omitempty"`
25-
Type string `json:"type"`
26-
Tier string `json:"tier"`
27-
License string `json:"license"`
28-
MinEngineVersion string `json:"minEngineVersion,omitempty"`
29-
Repository string `json:"repository,omitempty"`
30-
Keywords []string `json:"keywords,omitempty"`
31-
Homepage string `json:"homepage,omitempty"`
20+
Name string `json:"name"`
21+
Version string `json:"version"`
22+
Author string `json:"author"`
23+
Description string `json:"description"`
24+
Source string `json:"source,omitempty"`
25+
Type string `json:"type"`
26+
Tier string `json:"tier"`
27+
License string `json:"license"`
28+
MinEngineVersion string `json:"minEngineVersion,omitempty"`
29+
Repository string `json:"repository,omitempty"`
30+
Keywords []string `json:"keywords,omitempty"`
31+
Homepage string `json:"homepage,omitempty"`
3232
Capabilities *RegistryCapabilities `json:"capabilities,omitempty"`
33-
Downloads []PluginDownload `json:"downloads,omitempty"`
34-
Assets *PluginAssets `json:"assets,omitempty"`
33+
Downloads []PluginDownload `json:"downloads,omitempty"`
34+
Assets *PluginAssets `json:"assets,omitempty"`
3535
}
3636

3737
// RegistryCapabilities describes what module/step/trigger types a plugin provides.

cmd/wfctl/registry_validate.go

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -177,4 +177,3 @@ func FormatValidationErrors(errs []ValidationError) string {
177177
}
178178
return b.String()
179179
}
180-

cmd/wfctl/template_validate.go

Lines changed: 30 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import (
1111
"text/template"
1212

1313
"github.com/GoCodeAlone/workflow/config"
14+
"github.com/GoCodeAlone/workflow/schema"
1415
"gopkg.in/yaml.v3"
1516
)
1617

@@ -77,6 +78,7 @@ func runTemplateValidate(args []string) error {
7778
configFile := fs2.String("config", "", "Validate a specific config file instead of templates")
7879
strict := fs2.Bool("strict", false, "Fail on warnings (not just errors)")
7980
format := fs2.String("format", "text", "Output format: text or json")
81+
pluginDir := fs2.String("plugin-dir", "", "Directory of installed external plugins; their types are loaded before validation")
8082
fs2.Usage = func() {
8183
fmt.Fprintf(fs2.Output(), `Usage: wfctl template validate [options]
8284
@@ -90,6 +92,14 @@ Options:
9092
return err
9193
}
9294

95+
// Load external plugin types before validation so their module/trigger/workflow
96+
// types are recognised and don't cause false "unknown type" errors.
97+
if *pluginDir != "" {
98+
if err := schema.LoadPluginTypesFromDir(*pluginDir); err != nil {
99+
return fmt.Errorf("failed to load plugins from %s: %w", *pluginDir, err)
100+
}
101+
}
102+
93103
knownModules := KnownModuleTypes()
94104
knownSteps := KnownStepTypes()
95105
knownTriggers := KnownTriggerTypes()
@@ -353,15 +363,23 @@ func validateWorkflowConfig(name string, cfg *config.WorkflowConfig, knownModule
353363
result.Errors = append(result.Errors, fmt.Sprintf("module %q uses unknown type %q", mod.Name, mod.Type))
354364
} else {
355365
result.ModuleValid++
356-
// Warn on unknown config fields
366+
// Warn on unknown config fields (with snake_case hint)
357367
if mod.Config != nil && len(info.ConfigKeys) > 0 {
358368
knownKeys := make(map[string]bool)
369+
snakeToCamel := make(map[string]string)
359370
for _, k := range info.ConfigKeys {
360371
knownKeys[k] = true
372+
if snake := schema.CamelToSnake(k); snake != k {
373+
snakeToCamel[snake] = k
374+
}
361375
}
362376
for key := range mod.Config {
363377
if !knownKeys[key] {
364-
result.Warnings = append(result.Warnings, fmt.Sprintf("module %q (%s) config field %q not in known fields", mod.Name, mod.Type, key))
378+
if camel, ok := snakeToCamel[key]; ok {
379+
result.Warnings = append(result.Warnings, fmt.Sprintf("module %q (%s) config field %q uses snake_case; use camelCase %q instead", mod.Name, mod.Type, key, camel))
380+
} else {
381+
result.Warnings = append(result.Warnings, fmt.Sprintf("module %q (%s) config field %q not in known fields", mod.Name, mod.Type, key))
382+
}
365383
}
366384
}
367385
}
@@ -401,15 +419,23 @@ func validateWorkflowConfig(name string, cfg *config.WorkflowConfig, knownModule
401419
result.Errors = append(result.Errors, fmt.Sprintf("pipeline %q step uses unknown type %q", pipelineName, stepType))
402420
} else {
403421
result.StepValid++
404-
// Config key warnings
422+
// Config key warnings (with snake_case hint)
405423
if stepCfg, ok := stepMap["config"].(map[string]any); ok && len(stepInfo.ConfigKeys) > 0 {
406424
knownKeys := make(map[string]bool)
425+
snakeToCamel := make(map[string]string)
407426
for _, k := range stepInfo.ConfigKeys {
408427
knownKeys[k] = true
428+
if snake := schema.CamelToSnake(k); snake != k {
429+
snakeToCamel[snake] = k
430+
}
409431
}
410432
for key := range stepCfg {
411433
if !knownKeys[key] {
412-
result.Warnings = append(result.Warnings, fmt.Sprintf("pipeline %q step %q (%s) config field %q not in known fields", pipelineName, stepMap["name"], stepType, key))
434+
if camel, ok := snakeToCamel[key]; ok {
435+
result.Warnings = append(result.Warnings, fmt.Sprintf("pipeline %q step %q (%s) config field %q uses snake_case; use camelCase %q instead", pipelineName, stepMap["name"], stepType, key, camel))
436+
} else {
437+
result.Warnings = append(result.Warnings, fmt.Sprintf("pipeline %q step %q (%s) config field %q not in known fields", pipelineName, stepMap["name"], stepType, key))
438+
}
413439
}
414440
}
415441
}

0 commit comments

Comments
 (0)