Address all 9 review thread comments: fix pr_merge method, release_upload templates, schema accuracy, token required, type corrections, runtime sync tests

Agent-Logs-Url: https://github.com/GoCodeAlone/workflow-plugin-github/sessions/e1f18cab-168a-453e-a97d-dd290c4cfcae

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

Author: Copilot

internal/schemas.go

@@ -16,13 +16,6 @@ func (p *githubPlugin) ModuleSchemas() []sdk.ModuleSchemaData {
 			Category:    "github",
 			Description: "Receives GitHub webhook events via HTTP, verifies HMAC-SHA256 signatures, and publishes normalised GitEvent messages to a configurable topic.",
 			ConfigFields: []sdk.ConfigField{
-				{
-					Name:        "provider",
-					Type:        "string",
-					Description: "Webhook provider identifier",
-					DefaultValue: "github",
-					Required:    false,
-				},
 				{
 					Name:        "secret",
 					Type:        "string",
@@ -52,7 +45,7 @@ func (p *githubPlugin) ModuleSchemas() []sdk.ModuleSchemaData {
 				{Name: "author", Type: "string", Description: "Event author username"},
 				{Name: "message", Type: "string", Description: "Commit message or PR title"},
 				{Name: "url", Type: "string", Description: "URL to the commit or PR"},
-				{Name: "raw_payload", Type: "string", Description: "Raw JSON webhook payload"},
+				{Name: "raw_payload", Type: "object", Description: "Raw JSON webhook payload"},
 				{Name: "timestamp", Type: "string", Description: "Event timestamp in RFC3339 format"},
 			},
 		},

internal/schemas_test.go

@@ -3,34 +3,54 @@ package internal
 import (
 	"encoding/json"
 	"os"
+	"sort"
 	"testing"
 
 	"github.com/GoCodeAlone/workflow/plugin"
 )
 
 // TestModuleSchemas verifies that the plugin's SchemaProvider returns schema
-// descriptors for both advertised module types.
+// descriptors for both advertised module types and stays in sync with
+// ModuleTypes() and plugin.json moduleTypes.
 func TestModuleSchemas(t *testing.T) {
 	p := &githubPlugin{}
 	schemas := p.ModuleSchemas()
+	runtimeTypes := p.ModuleTypes()
 
 	if len(schemas) != 2 {
 		t.Fatalf("expected 2 module schemas, got %d", len(schemas))
 	}
 
-	byType := make(map[string]int, len(schemas))
+	// Every schema type must appear in the runtime ModuleTypes() list.
+	runtimeSet := make(map[string]bool, len(runtimeTypes))
+	for _, mt := range runtimeTypes {
+		runtimeSet[mt] = true
+	}
+	for _, s := range schemas {
+		if !runtimeSet[s.Type] {
+			t.Errorf("schema type %q is not in githubPlugin.ModuleTypes()", s.Type)
+		}
+	}
+
+	// Every runtime module type must have a schema entry.
+	schemaSet := make(map[string]int, len(schemas))
 	for i, s := range schemas {
-		byType[s.Type] = i
+		schemaSet[s.Type] = i
+	}
+	for _, mt := range runtimeTypes {
+		if _, ok := schemaSet[mt]; !ok {
+			t.Errorf("ModuleTypes() returns %q but no corresponding ModuleSchema exists", mt)
+		}
 	}
 
 	for _, wantType := range []string{"git.webhook", "github.app"} {
-		if _, ok := byType[wantType]; !ok {
+		if _, ok := schemaSet[wantType]; !ok {
 			t.Errorf("missing module schema for type %q", wantType)
 		}
 	}
 
-	// git.webhook should have at least the four documented config fields.
-	webhookIdx, ok := byType["git.webhook"]
+	// git.webhook should have the core documented config fields.
+	webhookIdx, ok := schemaSet["git.webhook"]
 	if !ok {
 		t.Fatalf("git.webhook schema not found")
 	}
@@ -41,15 +61,27 @@ func TestModuleSchemas(t *testing.T) {
 	if webhook.Description == "" {
 		t.Error("git.webhook schema: Description must not be empty")
 	}
-	if len(webhook.ConfigFields) < 4 {
-		t.Errorf("git.webhook schema: expected at least 4 config fields, got %d", len(webhook.ConfigFields))
+	webhookFieldNames := make(map[string]bool, len(webhook.ConfigFields))
+	for _, f := range webhook.ConfigFields {
+		webhookFieldNames[f.Name] = true
+	}
+	for _, want := range []string{"secret", "events", "topic"} {
+		if !webhookFieldNames[want] {
+			t.Errorf("git.webhook schema: required config field %q is missing", want)
+		}
 	}
 	if len(webhook.Outputs) == 0 {
 		t.Error("git.webhook schema: expected at least one output")
 	}
+	// raw_payload must be declared as an object (json.RawMessage), not string.
+	for _, f := range webhook.Outputs {
+		if f.Name == "raw_payload" && f.Type != "object" {
+			t.Errorf("git.webhook schema: raw_payload output type should be %q, got %q", "object", f.Type)
+		}
+	}
 
 	// github.app should declare the three required config fields.
-	appIdx, ok := byType["github.app"]
+	appIdx, ok := schemaSet["github.app"]
 	if !ok {
 		t.Fatalf("github.app schema not found")
 	}
@@ -71,10 +103,37 @@ func TestModuleSchemas(t *testing.T) {
 			t.Errorf("github.app schema: field %q should be marked required", want)
 		}
 	}
+
+	// Cross-check against plugin.json moduleTypes.
+	data, err := os.ReadFile("../plugin.json")
+	if err != nil {
+		t.Fatalf("plugin.json not found: %v", err)
+	}
+	var manifest struct {
+		ModuleTypes []string `json:"moduleTypes"`
+	}
+	if err := json.Unmarshal(data, &manifest); err != nil {
+		t.Fatalf("parse plugin.json: %v", err)
+	}
+	jsonModuleSet := make(map[string]bool, len(manifest.ModuleTypes))
+	for _, mt := range manifest.ModuleTypes {
+		jsonModuleSet[mt] = true
+	}
+	for _, mt := range runtimeTypes {
+		if !jsonModuleSet[mt] {
+			t.Errorf("githubPlugin.ModuleTypes() returns %q but plugin.json moduleTypes does not include it", mt)
+		}
+	}
+	for _, mt := range manifest.ModuleTypes {
+		if !runtimeSet[mt] {
+			t.Errorf("plugin.json moduleTypes includes %q but githubPlugin.ModuleTypes() does not", mt)
+		}
+	}
 }
 
 // TestPluginStepSchemasJSON verifies that plugin.json can be parsed and that
-// it declares a stepSchemas entry for every step type the plugin advertises.
+// it declares a stepSchemas entry for every step type the plugin advertises,
+// and that both are in sync with the runtime StepTypes() list.
 func TestPluginStepSchemasJSON(t *testing.T) {
 	// Locate plugin.json relative to the repository root (one level up from internal/).
 	data, err := os.ReadFile("../plugin.json")
@@ -111,6 +170,40 @@ func TestPluginStepSchemasJSON(t *testing.T) {
 		t.Errorf("plugin.json: stepSchemas count (%d) does not match stepTypes count (%d)",
 			len(manifest.StepSchemas), len(manifest.StepTypes))
 	}
+
+	// Cross-check JSON manifest against the runtime StepTypes() list.
+	p := &githubPlugin{}
+	runtimeTypes := p.StepTypes()
+
+	runtimeSet := make(map[string]bool, len(runtimeTypes))
+	for _, st := range runtimeTypes {
+		runtimeSet[st] = true
+	}
+	jsonTypeSet := make(map[string]bool, len(manifest.StepTypes))
+	for _, st := range manifest.StepTypes {
+		jsonTypeSet[st] = true
+	}
+
+	for _, rt := range runtimeTypes {
+		if !jsonTypeSet[rt] {
+			t.Errorf("githubPlugin.StepTypes() returns %q but plugin.json stepTypes does not include it", rt)
+		}
+	}
+	for _, jt := range manifest.StepTypes {
+		if !runtimeSet[jt] {
+			t.Errorf("plugin.json stepTypes includes %q but githubPlugin.StepTypes() does not return it", jt)
+		}
+	}
+
+	// Verify both lists have the same length (no duplicates or gaps).
+	sortedRuntime := append([]string(nil), runtimeTypes...)
+	sortedJSON := append([]string(nil), manifest.StepTypes...)
+	sort.Strings(sortedRuntime)
+	sort.Strings(sortedJSON)
+	if len(sortedRuntime) != len(sortedJSON) {
+		t.Errorf("runtime StepTypes() count (%d) != plugin.json stepTypes count (%d)",
+			len(sortedRuntime), len(sortedJSON))
+	}
 }
 
 // TestPluginManifestEngineValidation verifies that plugin.json is parseable as a

internal/step_pr_merge.go

@@ -5,6 +5,8 @@ import (
 	"fmt"
 	"os"
 
+	"github.com/google/go-github/v69/github"
+
 	sdk "github.com/GoCodeAlone/workflow/plugin/external/sdk"
 )
 
@@ -81,8 +83,9 @@ func (s *prMergeStep) Execute(
 	commitTitle := resolveField(s.config.CommitTitle, triggerData, stepOutputs, current)
 
 	client := NewSDKClient(token)
+	method := resolveField(s.config.Method, triggerData, stepOutputs, current)
 	result, _, err := client.GH.PullRequests.Merge(ctx, owner, repo, s.config.PRNumber,
-		commitTitle, nil)
+		commitTitle, &github.PullRequestOptions{MergeMethod: method})
 	if err != nil {
 		return errorResult(fmt.Sprintf("merge PR: %v", err)), nil
 	}

internal/step_release_upload.go

@@ -4,6 +4,8 @@ import (
 	"context"
 	"fmt"
 	"os"
+	"strconv"
+	"strings"
 
 	"github.com/google/go-github/v69/github"
 
@@ -27,12 +29,13 @@ type releaseUploadStep struct {
 }
 
 type releaseUploadConfig struct {
-	Owner     string `yaml:"owner"`
-	Repo      string `yaml:"repo"`
-	ReleaseID int64  `yaml:"release_id"`
-	File      string `yaml:"file"`
-	Name      string `yaml:"name"`
-	Token     string `yaml:"token"`
+	Owner        string `yaml:"owner"`
+	Repo         string `yaml:"repo"`
+	ReleaseID    int64  `yaml:"release_id"`
+	ReleaseIDRaw string `yaml:"-"` // raw string for dynamic {{.field}} resolution
+	File         string `yaml:"file"`
+	Name         string `yaml:"name"`
+	Token        string `yaml:"token"`
 }
 
 func newReleaseUploadStep(name string, raw map[string]any) (*releaseUploadStep, error) {
@@ -52,8 +55,20 @@ func newReleaseUploadStep(name string, raw map[string]any) (*releaseUploadStep,
 		cfg.ReleaseID = v
 	case float64:
 		cfg.ReleaseID = int64(v)
+	case string:
+		if v != "" {
+			if strings.Contains(v, "{{") && strings.Contains(v, "}}") {
+				cfg.ReleaseIDRaw = v
+			} else {
+				n, err := strconv.ParseInt(v, 10, 64)
+				if err != nil {
+					return nil, fmt.Errorf("step.gh_release_upload %q: config.release_id is not a valid integer: %w", name, err)
+				}
+				cfg.ReleaseID = n
+			}
+		}
 	}
-	if cfg.ReleaseID == 0 {
+	if cfg.ReleaseID == 0 && cfg.ReleaseIDRaw == "" {
 		return nil, fmt.Errorf("step.gh_release_upload %q: config.release_id is required", name)
 	}
 	cfg.File, _ = raw["file"].(string)
@@ -86,6 +101,20 @@ func (s *releaseUploadStep) Execute(
 		assetName = filePath
 	}
 
+	// Resolve release_id — may be a static int or a dynamic template reference.
+	releaseID := s.config.ReleaseID
+	if s.config.ReleaseIDRaw != "" {
+		resolved := resolveField(s.config.ReleaseIDRaw, triggerData, stepOutputs, current)
+		n, err := strconv.ParseInt(resolved, 10, 64)
+		if err != nil {
+			return errorResult(fmt.Sprintf("release_id resolved to non-integer value %q: %v", resolved, err)), nil
+		}
+		releaseID = n
+	}
+	if releaseID == 0 {
+		return errorResult("release_id resolved to zero — check pipeline context"), nil
+	}
+
 	f, err := os.Open(filePath) //nolint:gosec // G304: path from step config, trusted
 	if err != nil {
 		return errorResult(fmt.Sprintf("open file %q: %v", filePath, err)), nil
@@ -98,7 +127,7 @@ func (s *releaseUploadStep) Execute(
 	}
 
 	client := NewSDKClient(token)
-	asset, _, err := client.GH.Repositories.UploadReleaseAsset(ctx, owner, repo, s.config.ReleaseID,
+	asset, _, err := client.GH.Repositories.UploadReleaseAsset(ctx, owner, repo, releaseID,
 		&github.UploadOptions{Name: assetName},
 		f)
 	if err != nil {