diff --git a/.github/workflows/daily-news.lock.yml b/.github/workflows/daily-news.lock.yml index 16ca0f1c461..941eb203dba 100644 --- a/.github/workflows/daily-news.lock.yml +++ b/.github/workflows/daily-news.lock.yml @@ -1,4 +1,4 @@ -# gh-aw-metadata: {"schema_version":"v4","frontmatter_hash":"847860af3924384aa2f4bd0188a573e31264f6225f10ed1ff1dd8cb0ca014fe5","body_hash":"55ac3a789fdbd8eca450f17c99cb9b78e5eafede9392b9e5444dce5e97f9cbee","strict":true,"agent_id":"copilot"} +# gh-aw-metadata: {"schema_version":"v4","frontmatter_hash":"8d41a250f29248203bc447b1d39fe538173aa7b849fdbc6d8c2dadeb04b5d229","body_hash":"55ac3a789fdbd8eca450f17c99cb9b78e5eafede9392b9e5444dce5e97f9cbee","strict":true,"agent_id":"copilot"} # gh-aw-manifest: {"version":1,"secrets":["GH_AW_GITHUB_MCP_SERVER_TOKEN","GH_AW_GITHUB_TOKEN","GH_AW_OTEL_GRAFANA_AUTHORIZATION","GH_AW_OTEL_GRAFANA_ENDPOINT","GH_AW_OTEL_SENTRY_AUTHORIZATION","GH_AW_OTEL_SENTRY_ENDPOINT","GITHUB_TOKEN","TAVILY_API_KEY"],"actions":[{"repo":"actions/cache/restore","sha":"27d5ce7f107fe9357f9df03efb73ab90386fccae","version":"v5.0.5"},{"repo":"actions/cache/save","sha":"27d5ce7f107fe9357f9df03efb73ab90386fccae","version":"v5.0.5"},{"repo":"actions/checkout","sha":"de0fac2e4500dabe0009e67214ff5f5447ce83dd","version":"v6.0.2"},{"repo":"actions/download-artifact","sha":"3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c","version":"v8.0.1"},{"repo":"actions/github-script","sha":"3a2844b7e9c422d3c10d287c895573f7108da1b3","version":"v9.0.0"},{"repo":"actions/setup-node","sha":"48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e","version":"v6.4.0"},{"repo":"actions/setup-python","sha":"a309ff8b426b58ec0e2a45f0f869d46889d02405","version":"v6.2.0"},{"repo":"actions/upload-artifact","sha":"043fb46d1a93c77aae656e7c1c64a875d1fc6a0a","version":"v7.0.1"}],"containers":[{"image":"ghcr.io/github/gh-aw-firewall/agent:0.25.58"},{"image":"ghcr.io/github/gh-aw-firewall/api-proxy:0.25.58"},{"image":"ghcr.io/github/gh-aw-firewall/squid:0.25.58"},{"image":"ghcr.io/github/gh-aw-mcpg:v0.3.22"},{"image":"ghcr.io/github/github-mcp-server:v1.1.0"},{"image":"node:lts-alpine","digest":"sha256:2bdb65ed1dab192432bc31c95f94155ca5ad7fc1392fb7eb7526ab682fa5bf14","pinned_image":"node:lts-alpine@sha256:2bdb65ed1dab192432bc31c95f94155ca5ad7fc1392fb7eb7526ab682fa5bf14"}]} # ___ _ _ # / _ \ | | (_) @@ -246,24 +246,24 @@ jobs: run: | bash "${RUNNER_TEMP}/gh-aw/actions/create_prompt_first.sh" { - cat << 'GH_AW_PROMPT_91827a5740df86de_EOF' + cat << 'GH_AW_PROMPT_559c1a7d5e97c891_EOF' - GH_AW_PROMPT_91827a5740df86de_EOF + GH_AW_PROMPT_559c1a7d5e97c891_EOF cat "${RUNNER_TEMP}/gh-aw/prompts/xpia.md" cat "${RUNNER_TEMP}/gh-aw/prompts/temp_folder_prompt.md" cat "${RUNNER_TEMP}/gh-aw/prompts/markdown.md" cat "${RUNNER_TEMP}/gh-aw/prompts/cache_memory_prompt.md" cat "${RUNNER_TEMP}/gh-aw/prompts/repo_memory_prompt.md" cat "${RUNNER_TEMP}/gh-aw/prompts/safe_outputs_prompt.md" - cat << 'GH_AW_PROMPT_91827a5740df86de_EOF' + cat << 'GH_AW_PROMPT_559c1a7d5e97c891_EOF' Tools: create_discussion, upload_asset(max:5), missing_tool, missing_data, noop upload_asset: provide a file path; returns a URL; assets are published after the workflow completes (safeoutputs). - GH_AW_PROMPT_91827a5740df86de_EOF + GH_AW_PROMPT_559c1a7d5e97c891_EOF cat "${RUNNER_TEMP}/gh-aw/prompts/mcp_cli_tools_prompt.md" - cat << 'GH_AW_PROMPT_91827a5740df86de_EOF' + cat << 'GH_AW_PROMPT_559c1a7d5e97c891_EOF' The following GitHub context information is available for this workflow: {{#if github.actor}} @@ -292,9 +292,9 @@ jobs: {{/if}} - GH_AW_PROMPT_91827a5740df86de_EOF + GH_AW_PROMPT_559c1a7d5e97c891_EOF cat "${RUNNER_TEMP}/gh-aw/prompts/github_mcp_tools_with_safeoutputs_prompt.md" - cat << 'GH_AW_PROMPT_91827a5740df86de_EOF' + cat << 'GH_AW_PROMPT_559c1a7d5e97c891_EOF' {{#runtime-import .github/workflows/shared/mcp/tavily.md}} {{#runtime-import .github/skills/jqschema/SKILL.md}} @@ -305,7 +305,7 @@ jobs: {{#runtime-import .github/shared/editorial.md}} {{#runtime-import .github/workflows/shared/noop-reminder.md}} {{#runtime-import .github/workflows/daily-news.md}} - GH_AW_PROMPT_91827a5740df86de_EOF + GH_AW_PROMPT_559c1a7d5e97c891_EOF } > "$GH_AW_PROMPT" - name: Interpolate variables and render templates uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 @@ -671,9 +671,9 @@ jobs: mkdir -p /tmp/gh-aw/safeoutputs mkdir -p /tmp/gh-aw/mcp-logs/safeoutputs mkdir -p "${RUNNER_TEMP}/gh-aw/safeoutputs/upload-artifacts" - cat > "${RUNNER_TEMP}/gh-aw/safeoutputs/config.json" << GH_AW_SAFE_OUTPUTS_CONFIG_141cb3c981409d53_EOF + cat > "${RUNNER_TEMP}/gh-aw/safeoutputs/config.json" << GH_AW_SAFE_OUTPUTS_CONFIG_951003e9b0d04979_EOF {"create_discussion":{"category":"daily-news","close_older_discussions":true,"expires":72,"fallback_to_issue":true,"max":1},"create_report_incomplete_issue":{},"missing_data":{},"missing_tool":{},"noop":{"max":1,"report-as-issue":"true"},"push_repo_memory":{"memories":[{"dir":"/tmp/gh-aw/repo-memory/default","id":"default","max_file_count":100,"max_file_size":102400,"max_patch_size":10240}]},"report_incomplete":{},"upload_artifact":{"max-size-bytes":104857600,"max-uploads":3,"retention-days":30,"skip-archive":true},"upload_asset":{"allowed-exts":[".png",".jpg",".jpeg",".svg"],"branch":"assets/${GITHUB_WORKFLOW}","max":5,"max-size":10240}} - GH_AW_SAFE_OUTPUTS_CONFIG_141cb3c981409d53_EOF + GH_AW_SAFE_OUTPUTS_CONFIG_951003e9b0d04979_EOF - name: Generate Safe Outputs Tools env: GH_AW_TOOLS_META_JSON: | @@ -888,7 +888,7 @@ jobs: mkdir -p /home/runner/.copilot GH_AW_NODE=$(which node 2>/dev/null || command -v node 2>/dev/null || echo node) - cat << GH_AW_MCP_CONFIG_e0ef50bab4d0afa5_EOF | "$GH_AW_NODE" "${RUNNER_TEMP}/gh-aw/actions/start_mcp_gateway.cjs" + cat << GH_AW_MCP_CONFIG_4f87f7c49c163b5d_EOF | "$GH_AW_NODE" "${RUNNER_TEMP}/gh-aw/actions/start_mcp_gateway.cjs" { "mcpServers": { "github": { @@ -954,7 +954,7 @@ jobs: } } } - GH_AW_MCP_CONFIG_e0ef50bab4d0afa5_EOF + GH_AW_MCP_CONFIG_4f87f7c49c163b5d_EOF - name: Mount MCP servers as CLIs id: mount-mcp-clis continue-on-error: true diff --git a/docs/adr/36010-consolidate-duplicated-workflow-helpers.md b/docs/adr/36010-consolidate-duplicated-workflow-helpers.md new file mode 100644 index 00000000000..e42a90a91e5 --- /dev/null +++ b/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.* diff --git a/pkg/cli/audit_report_render_guard.go b/pkg/cli/audit_report_render_guard.go deleted file mode 100644 index 7f1e458cd3a..00000000000 --- a/pkg/cli/audit_report_render_guard.go +++ /dev/null @@ -1 +0,0 @@ -package cli diff --git a/pkg/cli/audit_report_render_jobs.go b/pkg/cli/audit_report_render_jobs.go deleted file mode 100644 index 7f1e458cd3a..00000000000 --- a/pkg/cli/audit_report_render_jobs.go +++ /dev/null @@ -1 +0,0 @@ -package cli diff --git a/pkg/cli/audit_report_render_overview.go b/pkg/cli/audit_report_render_overview.go deleted file mode 100644 index 7f1e458cd3a..00000000000 --- a/pkg/cli/audit_report_render_overview.go +++ /dev/null @@ -1 +0,0 @@ -package cli diff --git a/pkg/cli/audit_report_render_tools.go b/pkg/cli/audit_report_render_tools.go deleted file mode 100644 index 7f1e458cd3a..00000000000 --- a/pkg/cli/audit_report_render_tools.go +++ /dev/null @@ -1 +0,0 @@ -package cli diff --git a/pkg/cli/mcp_inspect.go b/pkg/cli/mcp_inspect.go index 14ae867fe15..00700739eeb 100644 --- a/pkg/cli/mcp_inspect.go +++ b/pkg/cli/mcp_inspect.go @@ -211,9 +211,14 @@ 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). @@ -221,19 +226,6 @@ func renderMCPInspectionTree(workflowPath string, workflowData *workflow.Workflo 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 { diff --git a/pkg/cli/mcp_inspect_tree_test.go b/pkg/cli/mcp_inspect_tree_test.go index ad1546c2146..47a7e439f4a 100644 --- a/pkg/cli/mcp_inspect_tree_test.go +++ b/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") } diff --git a/pkg/workflow/central_slash_command_workflow.go b/pkg/workflow/central_slash_command_workflow.go index 853a33c3e88..c052bbc875b 100644 --- a/pkg/workflow/central_slash_command_workflow.go +++ b/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) } diff --git a/pkg/workflow/engine_helpers.go b/pkg/workflow/engine_helpers.go index 399ef739d90..99737d3882d 100644 --- a/pkg/workflow/engine_helpers.go +++ b/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 { diff --git a/pkg/workflow/engine_helpers_test.go b/pkg/workflow/engine_helpers_test.go index 581a19f86ce..730c5990443 100644 --- a/pkg/workflow/engine_helpers_test.go +++ b/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 diff --git a/pkg/workflow/observability_otlp.go b/pkg/workflow/observability_otlp.go index 1d5d61d258c..90b67e39c88 100644 --- a/pkg/workflow/observability_otlp.go +++ b/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, ",") diff --git a/pkg/workflow/redact_secrets.go b/pkg/workflow/redact_secrets.go index fc61dd5ba42..c4bf247315f 100644 --- a/pkg/workflow/redact_secrets.go +++ b/pkg/workflow/redact_secrets.go @@ -17,9 +17,9 @@ const secretsPrefix = "secrets." // actionReferencePattern is replaced by a fast string scan in CollectActionReferences. // Pattern (informational): `(?m)^\s+(?:-\s+)?uses:\s+(\S+)(?:\s+#\s*(.+?))?$` -// escapeSingleQuote escapes single quotes and backslashes in a string to prevent injection -// when embedding data in single-quoted YAML strings -func escapeSingleQuote(s string) string { +// escapeSingleQuoteBackslash escapes single quotes and backslashes in a string to prevent +// injection when embedding data in single-quoted YAML strings using backslash escaping. +func escapeSingleQuoteBackslash(s string) string { // First escape backslashes, then escape single quotes s = strings.ReplaceAll(s, `\`, `\\`) s = strings.ReplaceAll(s, `'`, `\'`) @@ -185,7 +185,7 @@ func (c *Compiler) generateSecretRedactionStep(yaml *strings.Builder, yamlConten // Escape each secret reference to prevent injection when embedding in YAML escapedRefs := make([]string, len(secretReferences)) for i, ref := range secretReferences { - escapedRefs[i] = escapeSingleQuote(ref) + escapedRefs[i] = escapeSingleQuoteBackslash(ref) } fmt.Fprintf(yaml, " GH_AW_SECRET_NAMES: '%s'\n", strings.Join(escapedRefs, ",")) @@ -193,7 +193,7 @@ func (c *Compiler) generateSecretRedactionStep(yaml *strings.Builder, yamlConten // Each secret will be available as an environment variable for _, secretName := range secretReferences { // Escape secret name to prevent injection in YAML - escapedSecretName := escapeSingleQuote(secretName) + escapedSecretName := escapeSingleQuoteBackslash(secretName) // Use original secretName in GitHub Actions expression since it's already validated // to only contain safe characters (uppercase letters, numbers, underscores) fmt.Fprintf(yaml, " SECRET_%s: ${{ secrets.%s }}\n", escapedSecretName, secretName) diff --git a/pkg/workflow/redact_secrets_test.go b/pkg/workflow/redact_secrets_test.go index caff688a9cb..d09fcca6298 100644 --- a/pkg/workflow/redact_secrets_test.go +++ b/pkg/workflow/redact_secrets_test.go @@ -83,6 +83,13 @@ func TestCollectSecretReferences(t *testing.T) { } } +func TestEscapeSingleQuoteBackslash(t *testing.T) { + got := escapeSingleQuoteBackslash(`owner's\workflow`) + if got != `owner\'s\\workflow` { + t.Fatalf("escapeSingleQuoteBackslash() = %q", got) + } +} + func TestCollectActionReferences(t *testing.T) { tests := []struct { name string diff --git a/pkg/workflow/safe_outputs_actions.go b/pkg/workflow/safe_outputs_actions.go index e4b18ba27c9..55359b6b414 100644 --- a/pkg/workflow/safe_outputs_actions.go +++ b/pkg/workflow/safe_outputs_actions.go @@ -3,7 +3,6 @@ package workflow import ( "context" "encoding/base64" - "encoding/json" "fmt" "os" "path/filepath" @@ -466,30 +465,19 @@ func buildCustomSafeOutputActionsJSON(data *WorkflowData) string { return "" } - actionMapping := make(map[string]string, len(data.SafeOutputs.Actions)) + actionNames := make([]string, 0, len(data.SafeOutputs.Actions)) for actionName := range data.SafeOutputs.Actions { - normalizedName := stringutil.NormalizeSafeOutputIdentifier(actionName) - actionMapping[normalizedName] = normalizedName - } - - // Sort keys for deterministic output - keys := make([]string, 0, len(actionMapping)) - for k := range actionMapping { - keys = append(keys, k) - } - sort.Strings(keys) - - ordered := make(map[string]string, len(keys)) - for _, k := range keys { - ordered[k] = actionMapping[k] + actionNames = append(actionNames, actionName) } - jsonBytes, err := json.Marshal(ordered) + jsonStr, err := buildNormalizedSortedJSON(actionNames, func(normalizedName string) string { + return normalizedName + }) if err != nil { safeOutputActionsLog.Printf("Warning: failed to marshal custom safe output actions: %v", err) return "" } - return string(jsonBytes) + return jsonStr } // actionOutputKey returns the step output key for a given normalized action name. diff --git a/pkg/workflow/safe_outputs_config_helpers.go b/pkg/workflow/safe_outputs_config_helpers.go index 9789a9ca559..48fa5292712 100644 --- a/pkg/workflow/safe_outputs_config_helpers.go +++ b/pkg/workflow/safe_outputs_config_helpers.go @@ -17,6 +17,31 @@ import ( var safeOutputsConfigGenLog = logger.New("workflow:safe_outputs_config_generation_helpers") +func buildNormalizedSortedJSON(names []string, valueFn func(string) string) (string, error) { + values := make(map[string]string, len(names)) + for _, name := range names { + normalizedName := stringutil.NormalizeSafeOutputIdentifier(name) + values[normalizedName] = valueFn(normalizedName) + } + + keys := make([]string, 0, len(values)) + for k := range values { + keys = append(keys, k) + } + sort.Strings(keys) + + ordered := make(map[string]string, len(keys)) + for _, k := range keys { + ordered[k] = values[k] + } + + jsonBytes, err := json.Marshal(ordered) + if err != nil { + return "", err + } + return string(jsonBytes), nil +} + // buildCustomSafeOutputJobsJSON builds a JSON mapping of custom safe output job names to empty // strings, for use in the GH_AW_SAFE_OUTPUT_JOBS env var of the handler manager step. // This allows the handler manager to silently skip messages handled by custom safe-output job @@ -26,29 +51,15 @@ func buildCustomSafeOutputJobsJSON(data *WorkflowData) string { return "" } - // Build mapping of normalized job names to empty strings (no URL output for custom jobs) - jobMapping := make(map[string]string, len(data.SafeOutputs.Jobs)) + jobNames := make([]string, 0, len(data.SafeOutputs.Jobs)) for jobName := range data.SafeOutputs.Jobs { - normalizedName := stringutil.NormalizeSafeOutputIdentifier(jobName) - jobMapping[normalizedName] = "" - } - - // Sort keys for deterministic output - keys := make([]string, 0, len(jobMapping)) - for k := range jobMapping { - keys = append(keys, k) - } - sort.Strings(keys) - - ordered := make(map[string]string, len(keys)) - for _, k := range keys { - ordered[k] = jobMapping[k] + jobNames = append(jobNames, jobName) } - jsonBytes, err := json.Marshal(ordered) + jsonStr, err := buildNormalizedSortedJSON(jobNames, func(string) string { return "" }) if err != nil { safeOutputsConfigGenLog.Printf("Warning: failed to marshal custom safe output jobs: %v", err) return "" } - return string(jsonBytes) + return jsonStr } diff --git a/pkg/workflow/safe_outputs_config_helpers_test.go b/pkg/workflow/safe_outputs_config_helpers_test.go index 8690a193a47..f1282bf8c18 100644 --- a/pkg/workflow/safe_outputs_config_helpers_test.go +++ b/pkg/workflow/safe_outputs_config_helpers_test.go @@ -3,9 +3,11 @@ package workflow import ( + "encoding/json" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestUsesPatchesAndCheckouts(t *testing.T) { @@ -149,3 +151,28 @@ func TestUsesPatchesAndCheckouts(t *testing.T) { }) } } + +func TestBuildCustomSafeOutputJobsJSON(t *testing.T) { + data := &WorkflowData{ + SafeOutputs: &SafeOutputsConfig{ + Jobs: map[string]*SafeJobConfig{ + "z-job": {}, + "a-job": {}, + }, + }, + } + + jsonStr := buildCustomSafeOutputJobsJSON(data) + require.NotEmpty(t, jsonStr) + assert.JSONEq(t, `{"a_job":"","z_job":""}`, jsonStr) + + var result map[string]string + require.NoError(t, json.Unmarshal([]byte(jsonStr), &result)) + assert.Empty(t, result["a_job"]) + assert.Empty(t, result["z_job"]) +} + +func TestBuildCustomSafeOutputJobsJSONEmpty(t *testing.T) { + assert.Empty(t, buildCustomSafeOutputJobsJSON(&WorkflowData{SafeOutputs: &SafeOutputsConfig{}})) + assert.Empty(t, buildCustomSafeOutputJobsJSON(&WorkflowData{SafeOutputs: nil})) +} diff --git a/pkg/workflow/strings.go b/pkg/workflow/strings.go index 590f9ba241f..dc7ae1970b1 100644 --- a/pkg/workflow/strings.go +++ b/pkg/workflow/strings.go @@ -150,6 +150,12 @@ func ShortenCommand(command string) string { return shortened } +// escapeYAMLSingleQuoted escapes single quotes for YAML single-quoted scalars by doubling each +// apostrophe per YAML 1.2. +func escapeYAMLSingleQuoted(value string) string { + return strings.ReplaceAll(value, "'", "''") +} + // GenerateHeredocDelimiterFromSeed creates a stable heredoc delimiter derived from a seed // (typically the workflow frontmatter hash hex string) so that repeated compilations of the // same workflow produce identical lock files. diff --git a/pkg/workflow/strings_test.go b/pkg/workflow/strings_test.go index 4506c5cb997..f5db5a723dd 100644 --- a/pkg/workflow/strings_test.go +++ b/pkg/workflow/strings_test.go @@ -277,6 +277,11 @@ func TestShortenCommand(t *testing.T) { } } +func TestEscapeYAMLSingleQuoted(t *testing.T) { + assert.Equal(t, "owner''s workflow", escapeYAMLSingleQuoted("owner's workflow")) + assert.Equal(t, "plain", escapeYAMLSingleQuoted("plain")) +} + func TestSanitizeName(t *testing.T) { tests := []struct { name string