fix(bnp): validate plugin MCP manifests

docs/adr/0001-plugin-mcp-artifacts-are-validated-before-packing.md

@@ -0,0 +1,28 @@
+# Plugin MCP artifacts are validated before packing
+
+Plugin SDK v0.3.2 serializes Connect service descriptors into
+`PluginManifest.mcp_tools`. Without a CLI gate, an artifact could carry
+inconsistent procedures, missing RBAC coverage, invalid schemas, unstable
+names, or no runtime HTTP handler and fail only after installation.
+
+Decision: both `ninja plugin build` and `ninja plugin verify` validate the MCP
+surface. Descriptor procedures must agree with service and method fields, be
+unique, map to supported RBAC roles, contain object input schemas, and derive a
+scoped name no longer than 128 bytes. The protobuf package must end in an API
+major such as `v1`, `plugin.mod` must provide scope and plugin identity, and any
+descriptor-bearing manifest must set `has_http_handler`. The CLI pins Plugin
+SDK v0.3.2 so builds preserve the typed descriptor field.
+
+Relying only on the CMS loader was rejected because it lets known-invalid
+artifacts reach the registry. Validating only during build was rejected because
+`verify` must independently gate externally supplied artifacts.
+
+Consequences:
+
+- Invalid MCP artifacts fail before upload or installation.
+- The CLI and CMS enforce the same descriptor and runtime invariants.
+- Wiki artifacts built with this CLI retain all typed MCP descriptors.
+
+Keywords: ninja plugin build, ninja plugin verify, Plugin SDK v0.3.2,
+MCPToolDescriptor, mcp_tools, has_http_handler, plugin.mod scope, RBAC, JSON
+Schema, plugin_ninja_wiki_v1

go.mod

@@ -5,7 +5,7 @@ go 1.26.4
 require (
 	connectrpc.com/connect v1.20.0
 	git.dev.alexdunmow.com/block/core v0.18.2
-	git.dev.alexdunmow.com/block/pluginsdk v0.2.7
+	git.dev.alexdunmow.com/block/pluginsdk v0.3.2
 	github.com/chromedp/cdproto v0.0.0-20260321001828-e3e3800016bc
 	github.com/chromedp/chromedp v0.15.1
 	github.com/klauspost/compress v1.18.6

go.sum

@@ -2,8 +2,8 @@ connectrpc.com/connect v1.20.0 h1:6TNDAB+WeNd2uolWNlYczB5E0KNNaVMNUEx8JEUsPmQ=
 connectrpc.com/connect v1.20.0/go.mod h1:A2ygJrukXwWy32vkCAAHNVguZrqZ+jeZ9rGRnGR4dN4=
 git.dev.alexdunmow.com/block/core v0.18.2 h1:+3OfZ424yoc1k1CucXYARk8TBkh8Z693HRkhf5Ci2wU=
 git.dev.alexdunmow.com/block/core v0.18.2/go.mod h1:GGuUu826AoJepC/hKLGJ7BX3PQaDss9ueCT0se6Ao2w=
-git.dev.alexdunmow.com/block/pluginsdk v0.2.7 h1:iL6Qvg2xHqHtii/0ZF9TIjjDzMrYkmxvLwqU300nmac=
+git.dev.alexdunmow.com/block/pluginsdk v0.3.2 h1:aLGkLzmJ0+docoke1e9sZTtugNugPaD3PNJEonzurgI=
-git.dev.alexdunmow.com/block/pluginsdk v0.2.7/go.mod h1:Z+eG+WZxAP0jfreLqlGcc0kkWKt8RWevzWyWn8d+dhM=
+git.dev.alexdunmow.com/block/pluginsdk v0.3.2/go.mod h1:Z+eG+WZxAP0jfreLqlGcc0kkWKt8RWevzWyWn8d+dhM=
 github.com/BurntSushi/toml v1.6.0 h1:dRaEfpa2VI55EwlIW72hMRHdWouJeRF7TPYhI+AUQjk=
 github.com/BurntSushi/toml v1.6.0/go.mod h1:ukJfTF/6rtPPRCnwkur4qwRxa8vTRFBF0uk2lLoLwho=
 github.com/chromedp/cdproto v0.0.0-20260321001828-e3e3800016bc h1:wkN/LMi5vc60pBRWx6qpbk/aEvq3/ZVNpnMvsw8PVVU=

internal/bnp/build.go

@@ -12,8 +12,8 @@ import (
 	"strconv"
 
 	abiv1 "git.dev.alexdunmow.com/block/pluginsdk/abi/v1"
-	core "git.dev.alexdunmow.com/block/pluginsdk/plugin"
 	"git.dev.alexdunmow.com/block/pluginsdk/egress"
+	core "git.dev.alexdunmow.com/block/pluginsdk/plugin"
 	"google.golang.org/protobuf/proto"
 )
 
@@ -165,6 +165,9 @@ func Build(ctx context.Context, opts BuildOptions) (*BuildResult, error) {
 	if err := applyManifestYAML(dir, manifest); err != nil {
 		return nil, err
 	}
+	if err := ValidateMCPManifest(manifest, mod.Plugin.Scope, mod.Plugin.Name); err != nil {
+		return nil, err
+	}
 
 	manifestBytes, err := proto.Marshal(manifest)
 	if err != nil {

internal/bnp/mcp_tools.go

@@ -0,0 +1,160 @@
+package bnp
+
+import (
+	"encoding/json"
+	"fmt"
+	"strings"
+	"unicode"
+
+	abiv1 "git.dev.alexdunmow.com/block/pluginsdk/abi/v1"
+)
+
+// ValidateMCPManifest enforces the descriptor/runtime contract shared with the
+// CMS before an artifact can be packed or accepted by `ninja plugin verify`.
+func ValidateMCPManifest(manifest *abiv1.PluginManifest, registryScope, pluginName string) error {
+	descriptors := manifest.GetMcpTools()
+	if len(descriptors) == 0 {
+		return nil
+	}
+	if !manifest.GetHasHttpHandler() {
+		return fmt.Errorf("bnp: MCP descriptors require has_http_handler so tools are callable at runtime")
+	}
+
+	seenProcedures := make(map[string]bool, len(descriptors))
+	seenToolNames := make(map[string]bool, len(descriptors))
+	for _, descriptor := range descriptors {
+		if descriptor == nil {
+			return fmt.Errorf("bnp: MCP descriptor is nil")
+		}
+		procedure := strings.TrimSpace(descriptor.GetProcedure())
+		serviceName, methodName, ok := splitConnectProcedure(procedure)
+		if !ok || serviceName != descriptor.GetServiceFullName() || methodName != descriptor.GetMethodName() {
+			return fmt.Errorf("bnp: MCP descriptor has inconsistent procedure %q", procedure)
+		}
+		if seenProcedures[procedure] {
+			return fmt.Errorf("bnp: duplicate MCP procedure %q", procedure)
+		}
+		seenProcedures[procedure] = true
+
+		role, mapped := manifest.GetRbacMethodRoles()[procedure]
+		if !mapped || !supportedMCPRole(role) {
+			return fmt.Errorf("bnp: MCP procedure %q has no supported RBAC role", procedure)
+		}
+		if !validMCPObjectSchema(descriptor.GetInputSchemaJson()) {
+			return fmt.Errorf("bnp: MCP procedure %q has an invalid input schema", procedure)
+		}
+
+		toolName, err := mcpToolName(registryScope, pluginName, serviceName, methodName)
+		if err != nil {
+			return fmt.Errorf("bnp: MCP procedure %q: %w", procedure, err)
+		}
+		if seenToolNames[toolName] {
+			return fmt.Errorf("bnp: MCP procedures produce duplicate tool name %q", toolName)
+		}
+		seenToolNames[toolName] = true
+	}
+	return nil
+}
+
+func supportedMCPRole(role string) bool {
+	switch role {
+	case "", "public", "viewer", "admin", "superadmin":
+		return true
+	default:
+		return false
+	}
+}
+
+func validMCPObjectSchema(raw []byte) bool {
+	var schema map[string]any
+	if json.Unmarshal(raw, &schema) != nil {
+		return false
+	}
+	typeName, _ := schema["type"].(string)
+	return typeName == "object"
+}
+
+func splitConnectProcedure(procedure string) (serviceName, methodName string, ok bool) {
+	trimmed := strings.TrimPrefix(strings.TrimSpace(procedure), "/")
+	serviceName, methodName, ok = strings.Cut(trimmed, "/")
+	return serviceName, methodName, ok && serviceName != "" && methodName != "" && !strings.Contains(methodName, "/")
+}
+
+func mcpToolName(registryScope, pluginName, serviceFullName, methodName string) (string, error) {
+	scope := mcpIdentifier(strings.TrimPrefix(strings.TrimSpace(registryScope), "@"))
+	plugin := mcpIdentifier(pluginName)
+	if scope == "" || plugin == "" {
+		return "", fmt.Errorf("registry scope and plugin name must be non-empty MCP identifiers")
+	}
+
+	parts := strings.Split(serviceFullName, ".")
+	if len(parts) < 2 {
+		return "", fmt.Errorf("service %q has no protobuf package", serviceFullName)
+	}
+	apiMajor := parts[len(parts)-2]
+	if !validAPIMajor(apiMajor) {
+		return "", fmt.Errorf("service %q protobuf package must end in an API major such as v1", serviceFullName)
+	}
+	service := mcpIdentifier(strings.TrimSuffix(parts[len(parts)-1], "Service"))
+	if service == "" {
+		return "", fmt.Errorf("service %q has no service qualifier", serviceFullName)
+	}
+
+	domain := "plugin_" + scope + "_" + plugin + "_" + apiMajor
+	if service != plugin {
+		domain += "_" + service
+	}
+	toolName := domain + "_" + mcpIdentifier(methodName)
+	if len(toolName) > 128 {
+		return "", fmt.Errorf("tool name %q exceeds MCP's 128-byte limit", toolName)
+	}
+	return toolName, nil
+}
+
+func validAPIMajor(value string) bool {
+	if len(value) < 2 || value[0] != 'v' {
+		return false
+	}
+	for _, character := range value[1:] {
+		if character < '0' || character > '9' {
+			return false
+		}
+	}
+	return true
+}
+
+func mcpIdentifier(value string) string {
+	value = snakeIdentifier(strings.TrimSpace(value))
+	var builder strings.Builder
+	previousSeparator := false
+	for _, character := range value {
+		if (character >= 'a' && character <= 'z') || (character >= '0' && character <= '9') {
+			builder.WriteRune(character)
+			previousSeparator = false
+			continue
+		}
+		if builder.Len() > 0 && !previousSeparator {
+			builder.WriteByte('_')
+			previousSeparator = true
+		}
+	}
+	return strings.TrimSuffix(builder.String(), "_")
+}
+
+func snakeIdentifier(value string) string {
+	var builder strings.Builder
+	runes := []rune(value)
+	for index, current := range runes {
+		if unicode.IsUpper(current) {
+			previousLower := index > 0 && unicode.IsLower(runes[index-1])
+			nextLower := index+1 < len(runes) && unicode.IsLower(runes[index+1])
+			if index > 0 && (previousLower || nextLower) {
+				builder.WriteByte('_')
+			}
+			builder.WriteRune(unicode.ToLower(current))
+			continue
+		}
+		builder.WriteRune(current)
+	}
+	return builder.String()
+}

internal/bnp/mcp_tools_test.go

@@ -0,0 +1,110 @@
+package bnp
+
+import (
+	"path/filepath"
+	"strings"
+	"testing"
+
+	abiv1 "git.dev.alexdunmow.com/block/pluginsdk/abi/v1"
+	"google.golang.org/protobuf/proto"
+)
+
+func wikiMCPManifest(hasHTTPHandler bool) *abiv1.PluginManifest {
+	procedure := "/wiki.v1.WikiService/SaveArticle"
+	return &abiv1.PluginManifest{
+		AbiVersion:      1,
+		Name:            "wiki",
+		Version:         "0.1.12",
+		HasHttpHandler:  hasHTTPHandler,
+		RbacMethodRoles: map[string]string{procedure: "admin"},
+		McpTools: []*abiv1.MCPToolDescriptor{{
+			Procedure:       procedure,
+			ServiceFullName: "wiki.v1.WikiService",
+			MethodName:      "SaveArticle",
+			InputSchemaJson: []byte(`{"type":"object","properties":{"article":{"type":"object"}}}`),
+		}},
+	}
+}
+
+func TestValidateMCPManifestAcceptsCallableWikiDescriptor(t *testing.T) {
+	manifest := wikiMCPManifest(true)
+	if err := ValidateMCPManifest(manifest, "@ninja", "wiki"); err != nil {
+		t.Fatalf("ValidateMCPManifest() error = %v", err)
+	}
+	name, err := mcpToolName("@ninja", "wiki", "wiki.v1.WikiService", "SaveArticle")
+	if err != nil {
+		t.Fatalf("mcpToolName() error = %v", err)
+	}
+	if name != "plugin_ninja_wiki_v1_save_article" {
+		t.Fatalf("tool name = %q", name)
+	}
+}
+
+func TestValidateMCPManifestRejectsMissingRuntimeHandler(t *testing.T) {
+	err := ValidateMCPManifest(wikiMCPManifest(false), "@ninja", "wiki")
+	if err == nil || !strings.Contains(err.Error(), "has_http_handler") {
+		t.Fatalf("error = %v, want has_http_handler rejection", err)
+	}
+}
+
+func TestValidateMCPManifestRejectsMalformedDescriptors(t *testing.T) {
+	tests := []struct {
+		name   string
+		mutate func(*abiv1.PluginManifest)
+		want   string
+	}{
+		{
+			name: "unmapped procedure",
+			mutate: func(manifest *abiv1.PluginManifest) {
+				manifest.RbacMethodRoles = nil
+			},
+			want: "RBAC role",
+		},
+		{
+			name: "invalid schema",
+			mutate: func(manifest *abiv1.PluginManifest) {
+				manifest.McpTools[0].InputSchemaJson = []byte(`{"type":"string"}`)
+			},
+			want: "input schema",
+		},
+		{
+			name: "missing API major",
+			mutate: func(manifest *abiv1.PluginManifest) {
+				descriptor := manifest.McpTools[0]
+				descriptor.Procedure = "/wiki.WikiService/SaveArticle"
+				descriptor.ServiceFullName = "wiki.WikiService"
+				manifest.RbacMethodRoles = map[string]string{descriptor.Procedure: "admin"}
+			},
+			want: "API major",
+		},
+	}
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			manifest := wikiMCPManifest(true)
+			test.mutate(manifest)
+			err := ValidateMCPManifest(manifest, "@ninja", "wiki")
+			if err == nil || !strings.Contains(err.Error(), test.want) {
+				t.Fatalf("error = %v, want %q", err, test.want)
+			}
+		})
+	}
+}
+
+func TestVerifyRejectsMCPDescriptorsWithoutRuntimeHandler(t *testing.T) {
+	manifestBytes, err := proto.Marshal(wikiMCPManifest(false))
+	if err != nil {
+		t.Fatalf("marshal manifest: %v", err)
+	}
+	out := filepath.Join(t.TempDir(), "wiki.bnp")
+	_, err = packArtifact(out, []packEntry{
+		{ArtifactPath: fileWasm, Data: []byte("wasm")},
+		{ArtifactPath: fileMod, Data: []byte("[plugin]\nname = \"wiki\"\nscope = \"@ninja\"\nversion = \"0.1.12\"\n")},
+		{ArtifactPath: fileManifest, Data: manifestBytes},
+	})
+	if err != nil {
+		t.Fatalf("pack artifact: %v", err)
+	}
+	if _, err := Verify(out); err == nil || !strings.Contains(err.Error(), "has_http_handler") {
+		t.Fatalf("Verify() error = %v, want has_http_handler rejection", err)
+	}
+}

internal/bnp/verify.go

@@ -121,6 +121,9 @@ func Verify(bnpPath string) (*VerifyResult, error) {
 	if modName != name {
 		return nil, fmt.Errorf("bnp: manifest name %q != plugin.mod name %q", name, modName)
 	}
+	if err := ValidateMCPManifest(manifest, parseModString(modBytes, "scope"), name); err != nil {
+		return nil, err
+	}
 
 	return &VerifyResult{
 		Name:            name,
@@ -332,9 +335,13 @@ func dirExists(p string) bool {
 // mirroring the reader's tolerant line scan (works whether or not the key sits
 // under a [plugin] table).
 func parseModName(data []byte) string {
+	return parseModString(data, "name")
+}
+
+func parseModString(data []byte, key string) string {
 	sc := bufio.NewScanner(bytes.NewReader(data))
 	for sc.Scan() {
-		after, ok := strings.CutPrefix(strings.TrimSpace(sc.Text()), "name")
+		after, ok := strings.CutPrefix(strings.TrimSpace(sc.Text()), key)
 		if !ok {
 			continue
 		}