Refactor duplicated workflow helpers and remove dead audit render stubs

docs/adr/36010-consolidate-duplicated-workflow-helpers.md

@@ -0,0 +1,39 @@
+# ADR-36010: Consolidate Duplicated Workflow Helpers into Canonical Shared Functions
+
+**Date**: 2026-05-31
+**Status**: Draft
+
+## Context
+
+A semantic clustering audit of `pkg/workflow` and `pkg/cli` surfaced a small set of verified duplicates and dead code: engine-ID resolution (`EngineConfig.ID` → `AI` fallback) was implemented independently in both `pkg/cli/mcp_inspect.go` and `pkg/workflow/observability_otlp.go`; two YAML single-quote escapers (`escapeYAMLSingleQuotedScalar` and `escapeSingleQuotedYAMLString`) had byte-identical behavior; the safe-output JSON builders for jobs and actions repeated the same normalize/sort/marshal flow; and four `pkg/cli/audit_report_render_*.go` files were empty `package cli` stubs. Duplicated logic across packages drifts over time and obscures the single intended behavior, while empty stubs imply structure that does not exist.
+
+## Decision
+
+We will consolidate each cluster of duplicated logic into a single canonical helper and delete the dead stubs. Specifically: `workflow.ResolveEngineID` becomes the one engine-ID resolver (exported so `pkg/cli` can reuse it while keeping its CLI-specific `"unknown"` fallback at the call site); `escapeYAMLSingleQuoted` in `pkg/workflow/strings.go` becomes the one YAML single-quote escaper; `buildNormalizedSortedJSON` captures the shared normalize/sort/marshal flow for safe-output job and action maps; and the distinct backslash-based escaper is renamed `escapeSingleQuoteBackslash` to make its differing semantics explicit rather than collapsing it into the YAML escaper. The four empty audit-render files are removed. The refactor is behavior-preserving and covered by focused regression tests.
+
+## Alternatives Considered
+
+### Alternative 1: Leave the duplicates in place
+Each duplicate is small and individually harmless. Leaving them avoids new cross-package coupling. Rejected because the audit confirmed the implementations are genuinely identical, and divergence over time (a fix applied to one copy but not the other) is the exact failure mode this codebase wants to avoid for security-adjacent helpers like YAML escaping.
+
+### Alternative 2: Extract the helpers into a new dedicated utility package
+A neutral `internal/util`-style package would avoid `pkg/cli` taking a dependency on `pkg/workflow`. Rejected because `pkg/cli` already depends on `pkg/workflow` for the `WorkflowData` types these helpers operate on, so the canonical home is the package that owns the domain type; a new package would add an indirection layer without removing any dependency edge.
+
+## Consequences
+
+### Positive
+- Single source of truth for engine-ID resolution, YAML single-quote escaping, and safe-output JSON normalization — fixes apply in exactly one place.
+- Renaming the backslash escaper to `escapeSingleQuoteBackslash` makes the two distinct escaping semantics explicit and prevents an accidental future merge of the two.
+- Net reduction in code and the removal of four misleading empty package stubs.
+
+### Negative
+- `pkg/cli` now calls `workflow.ResolveEngineID`, tightening (though not newly introducing) the cli→workflow coupling; the CLI-specific `"unknown"` fallback now lives at the call site, slightly separated from the resolution logic.
+- Shared helpers raise the blast radius of a future change: a regression in `escapeYAMLSingleQuoted` or `buildNormalizedSortedJSON` now affects every caller at once.
+
+### Neutral
+- Output shape of the safe-output JSON and OTLP env rendering is unchanged; the refactor is verified behavior-preserving by the added tests.
+- `ResolveEngineID` returns `""` (not `"unknown"`) when no engine is available; the `"unknown"` presentation concern is intentionally pushed to each caller.
+
+---
+
+*This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/26699729023) workflow. The PR author must review, complete, and finalize this document before the PR can merge.*

pkg/cli/mcp_inspect.go

@@ -211,29 +211,21 @@ func renderMCPInspectionTree(workflowPath string, workflowData *workflow.Workflo
 		serversTree.Child(fmt.Sprintf("%s (%s)", config.Name, config.Type))
 	}
 
+	engineID := workflow.ResolveEngineID(workflowData)
+	if engineID == "" {
+		engineID = "unknown"
+	}
+
 	return tree.
 		Root("Workflow: " + workflowLabel).
-		Child("Engine: " + resolveWorkflowEngineID(workflowData)).
+		Child("Engine: " + engineID).
 		Child(serversTree).
 		Enumerator(tree.RoundedEnumerator).
 		EnumeratorStyle(styles.TreeEnumerator).
 		ItemStyle(styles.TreeNode).
 		String()
 }
 
-func resolveWorkflowEngineID(workflowData *workflow.WorkflowData) string {
-	if workflowData == nil {
-		return "unknown"
-	}
-	if workflowData.EngineConfig != nil && workflowData.EngineConfig.ID != "" {
-		return workflowData.EngineConfig.ID
-	}
-	if workflowData.AI != "" {
-		return workflowData.AI
-	}
-	return "unknown"
-}
-
 // NewMCPInspectSubcommand creates the mcp inspect subcommand
 // This is the former mcp inspect command now nested under mcp
 func NewMCPInspectSubcommand() *cobra.Command {

pkg/cli/mcp_inspect_tree_test.go

@@ -63,42 +63,10 @@ func TestRenderMCPInspectionTree_SortsServersDeterministically(t *testing.T) {
 	assert.Less(t, githubStdioIdx, playwrightIdx)
 }
 
-func TestResolveWorkflowEngineID(t *testing.T) {
-	tests := []struct {
-		name         string
-		workflowData *workflow.WorkflowData
-		want         string
-	}{
-		{
-			name:         "nil workflow data",
-			workflowData: nil,
-			want:         "unknown",
-		},
-		{
-			name: "engine config id",
-			workflowData: &workflow.WorkflowData{
-				EngineConfig: &workflow.EngineConfig{ID: "copilot"},
-				AI:           "claude",
-			},
-			want: "copilot",
-		},
-		{
-			name: "fallback to ai",
-			workflowData: &workflow.WorkflowData{
-				AI: "claude",
-			},
-			want: "claude",
-		},
-		{
-			name:         "unknown",
-			workflowData: &workflow.WorkflowData{},
-			want:         "unknown",
-		},
-	}
+func TestRenderMCPInspectionTree_UnknownEngineFallback(t *testing.T) {
+	result := renderMCPInspectionTree("/tmp/audit-workflows.md", &workflow.WorkflowData{}, []parser.RegistryMCPServerConfig{
+		{BaseMCPServerConfig: types.BaseMCPServerConfig{Type: "stdio"}, Name: "github"},
+	})
 
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			assert.Equal(t, tt.want, resolveWorkflowEngineID(tt.workflowData))
-		})
-	}
+	assert.Contains(t, result, "Engine: unknown")
 }

pkg/workflow/central_slash_command_workflow.go

@@ -498,5 +498,5 @@ func uniqueSorted(values []string) []string {
 }
 
 func escapeSingleQuotedYAMLString(input string) string {
-	return strings.ReplaceAll(input, "'", "''")
+	return escapeYAMLSingleQuoted(input)
 }

pkg/workflow/engine_helpers.go

@@ -76,6 +76,18 @@ func getEngineEnvOverrides(workflowData *WorkflowData) map[string]string {
 	return workflowData.EngineConfig.Env
 }
 
+// ResolveEngineID returns the workflow engine ID, preferring engine.id over the legacy AI field.
+// It returns an empty string when no engine ID is available.
+func ResolveEngineID(workflowData *WorkflowData) string {
+	if workflowData == nil {
+		return ""
+	}
+	if workflowData.EngineConfig != nil && workflowData.EngineConfig.ID != "" {
+		return workflowData.EngineConfig.ID
+	}
+	return workflowData.AI
+}
+
 // engineEnvHasKey reports whether the given env var key is present in the engine.env map.
 // Returns false if workflowData or EngineConfig is nil, or if the key is not in the map.
 func engineEnvHasKey(workflowData *WorkflowData, key string) bool {

pkg/workflow/engine_helpers_test.go

@@ -12,8 +12,49 @@ import (
 	"testing"
 
 	"github.com/github/gh-aw/pkg/constants"
+	"github.com/stretchr/testify/assert"
 )
 
+func TestResolveEngineID(t *testing.T) {
+	tests := []struct {
+		name         string
+		workflowData *WorkflowData
+		want         string
+	}{
+		{
+			name:         "nil workflow data",
+			workflowData: nil,
+			want:         "",
+		},
+		{
+			name: "engine config id",
+			workflowData: &WorkflowData{
+				EngineConfig: &EngineConfig{ID: "copilot"},
+				AI:           "claude",
+			},
+			want: "copilot",
+		},
+		{
+			name: "fallback to ai",
+			workflowData: &WorkflowData{
+				AI: "claude",
+			},
+			want: "claude",
+		},
+		{
+			name:         "empty when unavailable",
+			workflowData: &WorkflowData{},
+			want:         "",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equal(t, tt.want, ResolveEngineID(tt.workflowData))
+		})
+	}
+}
+
 func TestBuildStandardNpmEngineInstallSteps(t *testing.T) {
 	tests := []struct {
 		name           string

pkg/workflow/observability_otlp.go

@@ -19,12 +19,6 @@ var otlpLog = logger.New("workflow:observability_otlp")
 // equals (`=`) per the OpenTelemetry env-var resource attribute grammar.
 var otelResourceValueEscaper = strings.NewReplacer(`\`, `\\`, ",", `\,`, "=", `\=`)
 
-// escapeYAMLSingleQuotedScalar escapes single quotes for YAML single-quoted
-// scalars by doubling each `'` per YAML 1.2.
-func escapeYAMLSingleQuotedScalar(value string) string {
-	return strings.ReplaceAll(value, "'", "''")
-}
-
 var sentryEndpointExpressionPattern = regexp.MustCompile(`(?i)^\$\{\{\s*secrets\.` + regexp.QuoteMeta(constants.OTELSentryEndpointSecretName) + `\s*\}\}$`)
 
 func normalizeOTLPHeadersForEndpoint(raw any, endpoint string) string {
@@ -688,7 +682,7 @@ func (c *Compiler) injectOTLPConfig(workflowData *WorkflowData) {
 	//    compatibility (MCP gateway, legacy scripts). OTEL_SERVICE_NAME is
 	//    workflow-specific when WorkflowID is available.
 	otlpEnvLines := fmt.Sprintf("  OTEL_EXPORTER_OTLP_ENDPOINT: %s\n  OTEL_SERVICE_NAME: %s", firstEndpoint, serviceName)
-	otlpEnvLines += "\n  OTEL_RESOURCE_ATTRIBUTES: '" + escapeYAMLSingleQuotedScalar(otelResourceAttributes(workflowData)) + "'"
+	otlpEnvLines += "\n  OTEL_RESOURCE_ATTRIBUTES: '" + escapeYAMLSingleQuoted(otelResourceAttributes(workflowData)) + "'"
 
 	// 3. Inject per-endpoint headers env vars.
 	//    OTEL_EXPORTER_OTLP_HEADERS = first endpoint headers (backward compat).
@@ -706,7 +700,7 @@ func (c *Compiler) injectOTLPConfig(workflowData *WorkflowData) {
 	// The value is single-quoted to prevent YAML parsers from interpreting the
 	// leading '[' as a YAML sequence node rather than a plain string.
 	if encoded := encodeOTLPEndpoints(entries); encoded != "" {
-		escapedEncoded := escapeYAMLSingleQuotedScalar(encoded)
+		escapedEncoded := escapeYAMLSingleQuoted(encoded)
 		otlpEnvLines += "\n  GH_AW_OTLP_ENDPOINTS: '" + escapedEncoded + "'"
 		otlpLog.Printf("Injected GH_AW_OTLP_ENDPOINTS env var")
 	}
@@ -729,7 +723,7 @@ func (c *Compiler) injectOTLPConfig(workflowData *WorkflowData) {
 		customAttrs = workflowData.ParsedFrontmatter.Observability.OTLP.Attributes
 	}
 	if encoded := encodeOTLPCustomAttributes(customAttrs); encoded != "" {
-		escapedEncoded := escapeYAMLSingleQuotedScalar(encoded)
+		escapedEncoded := escapeYAMLSingleQuoted(encoded)
 		otlpEnvLines += "\n  GH_AW_OTLP_ATTRIBUTES: '" + escapedEncoded + "'"
 		otlpLog.Printf("Injected GH_AW_OTLP_ATTRIBUTES env var (%d custom attributes)", len(customAttrs))
 	}
@@ -773,16 +767,6 @@ func otelServiceName(workflowData *WorkflowData) string {
 	return defaultServiceName + "." + sanitizedWorkflowName
 }
 
-func resolveWorkflowEngineID(workflowData *WorkflowData) string {
-	if workflowData == nil {
-		return ""
-	}
-	if workflowData.EngineConfig != nil && workflowData.EngineConfig.ID != "" {
-		return workflowData.EngineConfig.ID
-	}
-	return workflowData.AI
-}
-
 func escapeOTELResourceAttributeValue(value string) string {
 	return otelResourceValueEscaper.Replace(value)
 }
@@ -801,7 +785,7 @@ func otelResourceAttributes(workflowData *WorkflowData) string {
 		"gh-aw.run.id=${{ github.run_id }}",
 		"github.run_id=${{ github.run_id }}",
 	}
-	if engineID := resolveWorkflowEngineID(workflowData); engineID != "" {
+	if engineID := ResolveEngineID(workflowData); engineID != "" {
 		attrs = append(attrs, "gh-aw.engine.id="+escapeOTELResourceAttributeValue(engineID))
 	}
 	return strings.Join(attrs, ",")