Fix PluginManifest to gracefully parse legacy object-style capabilities in plugin.json (#322)

* Initial plan

* Fix PluginManifest to handle legacy object-style capabilities in plugin.json

Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>

* Return error for unsupported capabilities JSON type; add negative test

Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>

* Resolve merge conflict: add MinEngineVersion, adopt rawManifest/withRawCaps pattern, move Dependency

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>

Author: Copilot

plugin/manifest.go

@@ -93,7 +93,8 @@ type Dependency struct {
 //
 // When the legacy object format is detected, its type lists are merged into the
 // top-level ModuleTypes, StepTypes, and TriggerTypes fields so callers always
-// find types in a consistent location.
+// find types in a consistent location. Any other JSON type (string, number,
+// bool) is rejected with a descriptive error.
 func (m *PluginManifest) UnmarshalJSON(data []byte) error {
 	// rawManifest breaks the recursion: it is the same layout as PluginManifest
 	// but without the custom UnmarshalJSON method.
@@ -114,32 +115,70 @@ func (m *PluginManifest) UnmarshalJSON(data []byte) error {
 	if len(raw.Capabilities) == 0 {
 		return nil
 	}
-	switch raw.Capabilities[0] {
+
+	// Peek at the first non-whitespace byte to decide which branch to take.
+	// This avoids silently ignoring genuinely invalid capability values.
+	switch firstNonSpace(raw.Capabilities) {
+	case 0, 'n':
+		// Empty or JSON null – treat as absent.
+
 	case '[':
-		// New format: array of CapabilityDecl
+		// New format: array of CapabilityDecl.
 		var caps []CapabilityDecl
 		if err := json.Unmarshal(raw.Capabilities, &caps); err != nil {
 			return fmt.Errorf("invalid capabilities array: %w", err)
 		}
 		m.Capabilities = caps
+
 	case '{':
 		// Legacy format: object with configProvider, moduleTypes, stepTypes, triggerTypes.
 		// Merge type lists into the top-level fields so callers see them consistently.
 		var legacyCaps struct {
-			ModuleTypes  []string `json:"moduleTypes"`
-			StepTypes    []string `json:"stepTypes"`
-			TriggerTypes []string `json:"triggerTypes"`
+			ModuleTypes   []string `json:"moduleTypes"`
+			StepTypes     []string `json:"stepTypes"`
+			TriggerTypes  []string `json:"triggerTypes"`
+			WorkflowTypes []string `json:"workflowTypes"`
 		}
 		if err := json.Unmarshal(raw.Capabilities, &legacyCaps); err != nil {
 			return fmt.Errorf("invalid capabilities object: %w", err)
 		}
-		m.ModuleTypes = append(m.ModuleTypes, legacyCaps.ModuleTypes...)
-		m.StepTypes = append(m.StepTypes, legacyCaps.StepTypes...)
-		m.TriggerTypes = append(m.TriggerTypes, legacyCaps.TriggerTypes...)
+		m.ModuleTypes = appendUnique(m.ModuleTypes, legacyCaps.ModuleTypes...)
+		m.StepTypes = appendUnique(m.StepTypes, legacyCaps.StepTypes...)
+		m.TriggerTypes = appendUnique(m.TriggerTypes, legacyCaps.TriggerTypes...)
+		m.WorkflowTypes = appendUnique(m.WorkflowTypes, legacyCaps.WorkflowTypes...)
+
+	default:
+		return fmt.Errorf("capabilities: unsupported JSON type (expected array or object, got %q)", string(raw.Capabilities))
 	}
+
 	return nil
 }
 
+// firstNonSpace returns the first non-whitespace byte in b, or 0 if b is empty/all-whitespace.
+func firstNonSpace(b []byte) byte {
+	for _, c := range b {
+		if c != ' ' && c != '\t' && c != '\r' && c != '\n' {
+			return c
+		}
+	}
+	return 0
+}
+
+// appendUnique appends values to dst, skipping any that are already present.
+func appendUnique(dst []string, values ...string) []string {
+	existing := make(map[string]struct{}, len(dst))
+	for _, v := range dst {
+		existing[v] = struct{}{}
+	}
+	for _, v := range values {
+		if _, ok := existing[v]; !ok {
+			dst = append(dst, v)
+			existing[v] = struct{}{}
+		}
+	}
+	return dst
+}
+
 // Validate checks that a manifest has all required fields and valid semver.
 func (m *PluginManifest) Validate() error {
 	if m.Name == "" {

plugin/manifest_test.go

@@ -426,86 +426,136 @@ func TestManifestEngineFieldsLoadFromFile(t *testing.T) {
 	}
 }
 
-func TestPluginManifest_LegacyCapabilities(t *testing.T) {
-	// Legacy format: capabilities is a JSON object with configProvider, moduleTypes, etc.
-	legacyJSON := `{
-		"name": "legacy-plugin",
+// TestManifestLegacyCapabilitiesObject verifies that a plugin.json whose
+// "capabilities" field is a plain JSON object (the format used by external
+// plugins such as workflow-plugin-authz) is parsed without error and that the
+// type lists nested inside the object are promoted to the manifest's top-level
+// ModuleTypes/StepTypes/TriggerTypes fields.
+func TestManifestLegacyCapabilitiesObject(t *testing.T) {
+	const legacyJSON = `{
+		"name": "workflow-plugin-authz",
 		"version": "1.0.0",
-		"author": "Test",
-		"description": "Legacy capabilities test",
+		"description": "RBAC authorization plugin using Casbin",
+		"author": "GoCodeAlone",
+		"license": "MIT",
+		"type": "external",
+		"tier": "core",
+		"minEngineVersion": "0.3.11",
+		"keywords": ["authz", "rbac", "casbin", "authorization", "policy"],
+		"homepage": "https://github.com/GoCodeAlone/workflow-plugin-authz",
+		"repository": "https://github.com/GoCodeAlone/workflow-plugin-authz",
 		"capabilities": {
-			"configProvider": true,
-			"moduleTypes": ["test.module"],
-			"stepTypes": ["step.test"],
-			"triggerTypes": ["trigger.test"]
+			"configProvider": false,
+			"moduleTypes": ["authz.casbin"],
+			"stepTypes": [
+				"step.authz_check_casbin",
+				"step.authz_add_policy",
+				"step.authz_remove_policy",
+				"step.authz_role_assign"
+			],
+			"triggerTypes": []
 		}
 	}`
 
 	var m PluginManifest
 	if err := json.Unmarshal([]byte(legacyJSON), &m); err != nil {
-		t.Fatalf("Unmarshal legacy capabilities: %v", err)
+		t.Fatalf("unexpected unmarshal error for legacy capabilities object: %v", err)
 	}
-	if len(m.ModuleTypes) != 1 || m.ModuleTypes[0] != "test.module" {
-		t.Errorf("ModuleTypes = %v, want [test.module]", m.ModuleTypes)
-	}
-	if len(m.StepTypes) != 1 || m.StepTypes[0] != "step.test" {
-		t.Errorf("StepTypes = %v, want [step.test]", m.StepTypes)
-	}
-	if len(m.TriggerTypes) != 1 || m.TriggerTypes[0] != "trigger.test" {
-		t.Errorf("TriggerTypes = %v, want [trigger.test]", m.TriggerTypes)
-	}
-	// Legacy object format should not populate Capabilities slice
+
+	// Capabilities array should be nil / empty – the object format has no CapabilityDecl items.
 	if len(m.Capabilities) != 0 {
-		t.Errorf("Capabilities = %v, want empty for legacy object format", m.Capabilities)
+		t.Errorf("Capabilities = %v, want empty", m.Capabilities)
 	}
-}
 
-func TestPluginManifest_NewCapabilitiesArrayFormat(t *testing.T) {
-	// New format: capabilities is a JSON array of CapabilityDecl
-	newJSON := `{
-		"name": "new-plugin",
-		"version": "1.0.0",
-		"author": "Test",
-		"description": "New capabilities test",
-		"moduleTypes": ["test.module"],
-		"stepTypes": ["step.test"],
-		"capabilities": [{"name": "step.test", "role": "provider"}]
-	}`
+	// moduleTypes from the nested object should be promoted to the top level.
+	if len(m.ModuleTypes) != 1 || m.ModuleTypes[0] != "authz.casbin" {
+		t.Errorf("ModuleTypes = %v, want [authz.casbin]", m.ModuleTypes)
+	}
 
-	var m PluginManifest
-	if err := json.Unmarshal([]byte(newJSON), &m); err != nil {
-		t.Fatalf("Unmarshal new capabilities: %v", err)
+	// stepTypes should be promoted.
+	wantSteps := []string{
+		"step.authz_check_casbin",
+		"step.authz_add_policy",
+		"step.authz_remove_policy",
+		"step.authz_role_assign",
 	}
-	if len(m.Capabilities) != 1 || m.Capabilities[0].Name != "step.test" {
-		t.Errorf("Capabilities = %v, want [{step.test provider 0}]", m.Capabilities)
+	if len(m.StepTypes) != len(wantSteps) {
+		t.Errorf("StepTypes len = %d, want %d; got %v", len(m.StepTypes), len(wantSteps), m.StepTypes)
+	} else {
+		for i, want := range wantSteps {
+			if m.StepTypes[i] != want {
+				t.Errorf("StepTypes[%d] = %q, want %q", i, m.StepTypes[i], want)
+			}
+		}
 	}
-	if len(m.ModuleTypes) != 1 || m.ModuleTypes[0] != "test.module" {
-		t.Errorf("ModuleTypes = %v, want [test.module]", m.ModuleTypes)
+
+	// triggerTypes is an empty array – TriggerTypes should remain nil/empty.
+	if len(m.TriggerTypes) != 0 {
+		t.Errorf("TriggerTypes = %v, want empty", m.TriggerTypes)
 	}
 }
 
-func TestPluginManifest_LegacyCapabilitiesMergesWithTopLevel(t *testing.T) {
-	// Top-level fields should be merged with types from legacy capabilities object
-	legacyJSON := `{
-		"name": "merged-plugin",
+// TestManifestLegacyCapabilitiesObjectFile verifies that LoadManifest succeeds
+// for a plugin.json that uses the legacy object-style capabilities field.
+func TestManifestLegacyCapabilitiesObjectFile(t *testing.T) {
+	const legacyJSON = `{
+		"name": "workflow-plugin-authz",
 		"version": "1.0.0",
-		"author": "Test",
-		"description": "Merge test",
-		"moduleTypes": ["existing.module"],
+		"description": "RBAC authorization plugin",
+		"author": "GoCodeAlone",
 		"capabilities": {
-			"moduleTypes": ["caps.module"],
-			"stepTypes": ["step.caps"]
+			"moduleTypes": ["authz.casbin"],
+			"stepTypes": ["step.authz_check"],
+			"triggerTypes": []
 		}
 	}`
 
-	var m PluginManifest
-	if err := json.Unmarshal([]byte(legacyJSON), &m); err != nil {
-		t.Fatalf("Unmarshal: %v", err)
+	dir := t.TempDir()
+	path := filepath.Join(dir, "plugin.json")
+	if err := os.WriteFile(path, []byte(legacyJSON), 0644); err != nil {
+		t.Fatal(err)
+	}
+
+	m, err := LoadManifest(path)
+	if err != nil {
+		t.Fatalf("LoadManifest error: %v", err)
+	}
+	if len(m.ModuleTypes) != 1 || m.ModuleTypes[0] != "authz.casbin" {
+		t.Errorf("ModuleTypes = %v, want [authz.casbin]", m.ModuleTypes)
 	}
-	if len(m.ModuleTypes) != 2 {
-		t.Errorf("ModuleTypes = %v, want [existing.module caps.module]", m.ModuleTypes)
+	if len(m.StepTypes) != 1 || m.StepTypes[0] != "step.authz_check" {
+		t.Errorf("StepTypes = %v, want [step.authz_check]", m.StepTypes)
 	}
-	if len(m.StepTypes) != 1 || m.StepTypes[0] != "step.caps" {
-		t.Errorf("StepTypes = %v, want [step.caps]", m.StepTypes)
+}
+
+// TestManifestCapabilitiesInvalidFormat verifies that a plugin.json whose
+// "capabilities" field is neither an array nor an object (e.g. a bare string)
+// is rejected with a descriptive error.
+func TestManifestCapabilitiesInvalidFormat(t *testing.T) {
+	cases := []struct {
+		name string
+		json string
+	}{
+		{
+			name: "string value",
+			json: `{"name":"p","version":"1.0.0","author":"A","description":"D","capabilities":"oops"}`,
+		},
+		{
+			name: "numeric value",
+			json: `{"name":"p","version":"1.0.0","author":"A","description":"D","capabilities":42}`,
+		},
+		{
+			name: "boolean value",
+			json: `{"name":"p","version":"1.0.0","author":"A","description":"D","capabilities":true}`,
+		},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			var m PluginManifest
+			err := json.Unmarshal([]byte(tc.json), &m)
+			if err == nil {
+				t.Errorf("expected error for capabilities %s, got nil", tc.name)
+			}
+		})
 	}
 }