diff --git a/docs/adr/26551-migrate-github-app-token-input-to-client-id.md b/docs/adr/26551-migrate-github-app-token-input-to-client-id.md new file mode 100644 index 00000000000..375d5f0fc87 --- /dev/null +++ b/docs/adr/26551-migrate-github-app-token-input-to-client-id.md @@ -0,0 +1,88 @@ +# ADR-26551: Migrate GitHub App Token Input from `app-id` to `client-id` with Backward Compatibility + +**Date**: 2026-04-16 +**Status**: Draft +**Deciders**: pelikhan, Copilot + +--- + +## Part 1 — Narrative (Human-Friendly) + +### Context + +The `actions/create-github-app-token` action from GitHub has deprecated the `app-id` input field in favour of `client-id`. Compiled agentic workflows that mint GitHub App tokens were emitting upstream deprecation warnings because the code generator always emitted `with.app-id`. At the same time, existing workflow frontmatter authored by users already uses `app-id`, and a hard breaking change would silently break those workflows. The system needed to close the gap between what upstream now expects (`client-id`) and what authors had been writing (`app-id`), without requiring every user to manually update their files. + +### Decision + +We will migrate all generated token-minting steps to emit `client-id` instead of `app-id` when calling `actions/create-github-app-token`, while simultaneously accepting both field names in frontmatter parsing so that existing workflows continue to work. When both fields are present, `client-id` takes precedence. We will also ship an automated `gh aw fix` codemod (`github-app-app-id-to-client-id`) that rewrites `app-id` to `client-id` within `github-app` blocks, and update the JSON Schema to use `anyOf` with two valid required-field combinations (`client-id + private-key` or `app-id + private-key`). This approach eliminates deprecation noise immediately, preserves backward compatibility, and gives users a path to migrate automatically. + +### Alternatives Considered + +#### Alternative 1: Hard breaking change — remove `app-id` support immediately + +Immediately stop accepting `app-id` in frontmatter and require `client-id` everywhere. This would be the simplest long-term state but would break all existing workflow configurations without warning and require every author to update their files before their workflows would compile. Given that users have no tooling to detect the breakage until compile time, this was rejected as too disruptive. + +#### Alternative 2: Emit both `app-id` and `client-id` in generated output + +Emit both fields in the generated YAML step to satisfy both old and new versions of `actions/create-github-app-token`. This would silence deprecation warnings on the new action but produce noisy, confusing generated output and could cause unexpected behaviour if the upstream action changes its precedence rules. It was rejected because it couples generated output to undocumented upstream precedence logic. + +#### Alternative 3: Accept `app-id` in frontmatter only; always emit `client-id` in compiled output (chosen approach, with codemod) + +Accept both field names in parsing, but always emit `client-id` in compiled output. Pair this with a codemod that rewrites frontmatter on demand. This cleanly separates authoring compatibility from compiled output correctness, gives a migration path, and avoids the noise of the hard-cut approach. This is the approach implemented in this PR. + +### Consequences + +#### Positive +- Deprecation warnings from `actions/create-github-app-token` are eliminated immediately once workflows are compiled or recompiled. +- Existing workflows using `app-id` continue to compile and run without modification. +- Authors can migrate automatically using `gh aw fix`, reducing manual effort. +- The JSON Schema accurately reflects the valid combinations, enabling editor validation for both old and new syntax. + +#### Negative +- The internal `GitHubAppConfig.AppID` field now carries ambiguous semantics — it stores whatever the user provided (`client-id` or `app-id`) under a field named `AppID`. This mismatch between the Go field name and the preferred YAML key is mild technical debt. +- The `anyOf` schema constraint is harder to read and may produce less clear validation error messages than a single `required` array. +- Maintaining two accepted field names indefinitely (if `app-id` is never removed) adds ongoing surface area to the parser. + +#### Neutral +- The `app-id` field in the JSON Schema is now documented as a deprecated alias, which may prompt questions from authors reading schema documentation. +- All existing tests that asserted `app-id` in compiled output were updated to assert `client-id`; new tests were added for `client-id` parsing paths. +- The codemod is registered in the global codemod registry and will run as part of `gh aw fix` alongside all other codemods. + +--- + +## Part 2 — Normative Specification (RFC 2119) + +> The key words **MUST**, **MUST NOT**, **REQUIRED**, **SHALL**, **SHALL NOT**, **SHOULD**, **SHOULD NOT**, **RECOMMENDED**, **MAY**, and **OPTIONAL** in this section are to be interpreted as described in [RFC 2119](https://www.rfc-editor.org/rfc/rfc2119). + +### Compiled Output — Token Minting Steps + +1. All token-minting steps generated for `actions/create-github-app-token` **MUST** emit `with.client-id`, not `with.app-id`. +2. Generated workflows **MUST NOT** emit both `with.client-id` and `with.app-id` in the same step. + +### Frontmatter Parsing — Field Acceptance + +1. The frontmatter parser **MUST** accept `client-id` as the primary GitHub App identifier field within any `github-app` object. +2. The frontmatter parser **MUST** accept `app-id` as a backward-compatible alias for `client-id` within any `github-app` object. +3. When both `client-id` and `app-id` are present in the same `github-app` object, the parser **MUST** use `client-id` and **SHOULD** ignore `app-id`. +4. A `github-app` object **MUST** include `private-key` alongside either `client-id` or `app-id`; objects missing `private-key` **MUST** be rejected. + +### Schema Validation + +1. The JSON Schema for `github-app` objects **MUST** express validity via `anyOf` with two accepted combinations: `["client-id", "private-key"]` and `["app-id", "private-key"]`. +2. The `app-id` schema property **MUST** be documented as a deprecated alias for `client-id`. +3. Schema examples **SHOULD** use `client-id` rather than `app-id`. + +### Codemod + +1. The `github-app-app-id-to-client-id` codemod **MUST** rename `app-id` keys to `client-id` only within `github-app` YAML blocks. +2. The codemod **MUST NOT** rename `app-id` keys that appear outside of `github-app` blocks, regardless of nesting depth. +3. The codemod **MUST** be a no-op when no `github-app.app-id` fields are present in the frontmatter. +4. The codemod **MUST** be registered in the global codemod registry returned by `GetAllCodemods()`. + +### Conformance + +An implementation is considered conformant with this ADR if it satisfies all **MUST** and **MUST NOT** requirements above. Failure to meet any **MUST** or **MUST NOT** requirement constitutes non-conformance. + +--- + +*This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/24493163553) workflow. The PR author must review, complete, and finalize this document before the PR can merge.* diff --git a/pkg/cli/codemod_github_app_client_id.go b/pkg/cli/codemod_github_app_client_id.go new file mode 100644 index 00000000000..52c5c0d7f07 --- /dev/null +++ b/pkg/cli/codemod_github_app_client_id.go @@ -0,0 +1,101 @@ +package cli + +import ( + "slices" + "strings" + + "github.com/github/gh-aw/pkg/logger" +) + +var githubAppClientIDCodemodLog = logger.New("cli:codemod_github_app_client_id") + +// getGitHubAppClientIDCodemod creates a codemod that migrates github-app.app-id to github-app.client-id. +func getGitHubAppClientIDCodemod() Codemod { + return Codemod{ + ID: "github-app-app-id-to-client-id", + Name: "Rename 'github-app.app-id' to 'github-app.client-id'", + Description: "Renames deprecated 'app-id:' to 'client-id:' inside github-app blocks.", + IntroducedIn: "0.68.4", + Apply: func(content string, frontmatter map[string]any) (string, bool, error) { + if !hasDeprecatedGitHubAppIDField(frontmatter) { + return content, false, nil + } + newContent, applied, err := applyFrontmatterLineTransform(content, renameGitHubAppIDToClientID) + if applied { + githubAppClientIDCodemodLog.Print("Renamed github-app app-id to client-id") + } + return newContent, applied, err + }, + } +} + +// hasDeprecatedGitHubAppIDField returns true when any github-app object contains app-id. +func hasDeprecatedGitHubAppIDField(frontmatter map[string]any) bool { + return containsDeprecatedGitHubAppID(frontmatter) +} + +func containsDeprecatedGitHubAppID(value any) bool { + switch v := value.(type) { + case map[string]any: + for key, child := range v { + if key == "github-app" { + if appMap, ok := child.(map[string]any); ok { + if _, hasAppID := appMap["app-id"]; hasAppID { + return true + } + } + } + if containsDeprecatedGitHubAppID(child) { + return true + } + } + case []any: + return slices.ContainsFunc(v, containsDeprecatedGitHubAppID) + } + return false +} + +func renameGitHubAppIDToClientID(lines []string) ([]string, bool) { + result := make([]string, 0, len(lines)) + modified := false + + inGitHubApp := false + var githubAppIndent string + + for i, line := range lines { + trimmed := strings.TrimSpace(line) + + if len(trimmed) == 0 { + result = append(result, line) + continue + } + + if !strings.HasPrefix(trimmed, "#") && inGitHubApp && hasExitedBlock(line, githubAppIndent) { + inGitHubApp = false + } + + if strings.HasPrefix(trimmed, "github-app:") { + inGitHubApp = true + githubAppIndent = getIndentation(line) + result = append(result, line) + continue + } + + if inGitHubApp { + lineIndent := getIndentation(line) + if isDescendant(lineIndent, githubAppIndent) && strings.HasPrefix(trimmed, "app-id:") { + newLine, replaced := findAndReplaceInLine(line, "app-id", "client-id") + if replaced { + result = append(result, newLine) + modified = true + githubAppClientIDCodemodLog.Printf("Renamed github-app app-id on line %d", i+1) + continue + } + } + } + + result = append(result, line) + } + + return result, modified +} diff --git a/pkg/cli/codemod_github_app_client_id_test.go b/pkg/cli/codemod_github_app_client_id_test.go new file mode 100644 index 00000000000..9863800128b --- /dev/null +++ b/pkg/cli/codemod_github_app_client_id_test.go @@ -0,0 +1,113 @@ +//go:build !integration + +package cli + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestGitHubAppClientIDCodemod(t *testing.T) { + codemod := getGitHubAppClientIDCodemod() + + t.Run("renames app-id under github-app blocks", func(t *testing.T) { + content := `--- +github-app: + app-id: ${{ vars.TOP_LEVEL_APP_ID }} + private-key: ${{ secrets.TOP_LEVEL_APP_PRIVATE_KEY }} +on: + github-app: + app-id: ${{ vars.ACTIVATION_APP_ID }} + private-key: ${{ secrets.ACTIVATION_APP_PRIVATE_KEY }} +checkout: + - repository: org/repo + github-app: + app-id: ${{ vars.CHECKOUT_APP_ID }} + private-key: ${{ secrets.CHECKOUT_APP_PRIVATE_KEY }} +--- +` + frontmatter := map[string]any{ + "github-app": map[string]any{ + "app-id": "${{ vars.TOP_LEVEL_APP_ID }}", + "private-key": "${{ secrets.TOP_LEVEL_APP_PRIVATE_KEY }}", + }, + "on": map[string]any{ + "github-app": map[string]any{ + "app-id": "${{ vars.ACTIVATION_APP_ID }}", + "private-key": "${{ secrets.ACTIVATION_APP_PRIVATE_KEY }}", + }, + }, + "checkout": []any{ + map[string]any{ + "repository": "org/repo", + "github-app": map[string]any{ + "app-id": "${{ vars.CHECKOUT_APP_ID }}", + "private-key": "${{ secrets.CHECKOUT_APP_PRIVATE_KEY }}", + }, + }, + }, + } + + result, modified, err := codemod.Apply(content, frontmatter) + require.NoError(t, err, "Should not error when applying codemod") + assert.True(t, modified, "Should modify content") + assert.NotContains(t, result, "app-id:", "Should remove deprecated app-id key from github-app blocks") + assert.Contains(t, result, "client-id: ${{ vars.TOP_LEVEL_APP_ID }}", "Should migrate top-level github-app") + assert.Contains(t, result, "client-id: ${{ vars.ACTIVATION_APP_ID }}", "Should migrate on.github-app") + assert.Contains(t, result, "client-id: ${{ vars.CHECKOUT_APP_ID }}", "Should migrate checkout github-app") + }) + + t.Run("does not modify content without github-app.app-id", func(t *testing.T) { + content := `--- +github-app: + client-id: ${{ vars.APP_ID }} + private-key: ${{ secrets.APP_PRIVATE_KEY }} +--- +` + frontmatter := map[string]any{ + "github-app": map[string]any{ + "client-id": "${{ vars.APP_ID }}", + "private-key": "${{ secrets.APP_PRIVATE_KEY }}", + }, + } + + result, modified, err := codemod.Apply(content, frontmatter) + require.NoError(t, err, "Should not error") + assert.False(t, modified, "Should not modify already migrated content") + assert.Equal(t, content, result, "Content should remain unchanged") + }) + + t.Run("does not rename app-id outside github-app blocks", func(t *testing.T) { + content := `--- +engine: + provider: + auth: + client-id: APP_CLIENT_ID +tools: + custom: + app-id: value +--- +` + frontmatter := map[string]any{ + "engine": map[string]any{ + "provider": map[string]any{ + "auth": map[string]any{ + "client-id": "APP_CLIENT_ID", + }, + }, + }, + "tools": map[string]any{ + "custom": map[string]any{ + "app-id": "value", + }, + }, + } + + result, modified, err := codemod.Apply(content, frontmatter) + require.NoError(t, err, "Should not error") + assert.False(t, modified, "Should not modify non github-app app-id keys") + assert.Equal(t, content, result, "Content should remain unchanged") + }) +} diff --git a/pkg/cli/fix_codemods.go b/pkg/cli/fix_codemods.go index 9bdb843c2a4..e0f627d7eaf 100644 --- a/pkg/cli/fix_codemods.go +++ b/pkg/cli/fix_codemods.go @@ -47,6 +47,7 @@ func GetAllCodemods() []Codemod { getPlaywrightDomainsToNetworkAllowedCodemod(), // Migrate tools.playwright.allowed_domains to network.allowed getExpiresIntegerToDayStringCodemod(), // Convert expires integer (days) to string with 'd' suffix getGitHubAppCodemod(), // Rename deprecated 'app' to 'github-app' + getGitHubAppClientIDCodemod(), // Rename deprecated github-app.app-id to github-app.client-id getSafeInputsToMCPScriptsCodemod(), // Rename safe-inputs to mcp-scripts getPluginsToDependenciesCodemod(), // Migrate plugins to dependencies (plugins removed in favour of APM) getGitHubReposToAllowedReposCodemod(), // Rename deprecated tools.github.repos to tools.github.allowed-repos diff --git a/pkg/cli/fix_codemods_test.go b/pkg/cli/fix_codemods_test.go index feddc2e0d78..04ec1009338 100644 --- a/pkg/cli/fix_codemods_test.go +++ b/pkg/cli/fix_codemods_test.go @@ -43,7 +43,7 @@ func TestGetAllCodemods_ReturnsAllCodemods(t *testing.T) { codemods := GetAllCodemods() // Verify we have the expected number of codemods - expectedCount := 29 + expectedCount := 30 assert.Len(t, codemods, expectedCount, "Should return all %d codemods", expectedCount) // Verify all codemods have required fields @@ -130,6 +130,7 @@ func TestGetAllCodemods_InExpectedOrder(t *testing.T) { "playwright-allowed-domains-migration", "expires-integer-to-string", "app-to-github-app", + "github-app-app-id-to-client-id", "safe-inputs-to-mcp-scripts", "plugins-to-dependencies", "github-repos-to-allowed-repos", diff --git a/pkg/parser/import_field_extractor.go b/pkg/parser/import_field_extractor.go index 0bec0c6e6fc..380c67eacd9 100644 --- a/pkg/parser/import_field_extractor.go +++ b/pkg/parser/import_field_extractor.go @@ -511,7 +511,7 @@ func computeImportRelPath(fullPath, importPath string) string { } // validateGitHubAppJSON validates that a JSON-encoded GitHub App configuration has the required -// fields (app-id and private-key). Returns the input JSON if valid, or "" otherwise. +// fields ((client-id or app-id) and private-key). Returns the input JSON if valid, or "" otherwise. func validateGitHubAppJSON(appJSON string) string { if appJSON == "" || appJSON == "null" { return "" @@ -520,7 +520,9 @@ func validateGitHubAppJSON(appJSON string) string { if err := json.Unmarshal([]byte(appJSON), &appMap); err != nil { return "" } - if _, hasID := appMap["app-id"]; !hasID { + _, hasClientID := appMap["client-id"] + _, hasAppID := appMap["app-id"] + if !hasClientID && !hasAppID { return "" } if _, hasKey := appMap["private-key"]; !hasKey { diff --git a/pkg/parser/schema_test.go b/pkg/parser/schema_test.go index c219cb4e45a..36d6000d785 100644 --- a/pkg/parser/schema_test.go +++ b/pkg/parser/schema_test.go @@ -498,6 +498,26 @@ func TestNormalizeForJSONSchema(t *testing.T) { } } +func TestValidateMainWorkflowFrontmatterWithSchemaAndLocation_GitHubAppClientID(t *testing.T) { + frontmatter := map[string]any{ + "name": "Client ID validation", + "on": map[string]any{ + "issues": map[string]any{ + "types": []any{"opened"}, + }, + }, + "github-app": map[string]any{ + "client-id": "${{ vars.APP_ID }}", + "private-key": "${{ secrets.APP_PRIVATE_KEY }}", + }, + } + + err := ValidateMainWorkflowFrontmatterWithSchemaAndLocation(frontmatter, "/tmp/gh-aw/client-id-schema-test.md") + if err != nil { + t.Fatalf("expected client-id in github-app to pass schema validation, got: %v", err) + } +} + // TestNormalizeForJSONSchema_NestedMap verifies recursive normalization of maps. func TestNormalizeForJSONSchema_NestedMap(t *testing.T) { input := map[string]any{ diff --git a/pkg/parser/schemas/main_workflow_schema.json b/pkg/parser/schemas/main_workflow_schema.json index 0f4d83fdf80..b77b8ff7b36 100644 --- a/pkg/parser/schemas/main_workflow_schema.json +++ b/pkg/parser/schemas/main_workflow_schema.json @@ -400,7 +400,7 @@ "apm-packages": { "packages": ["microsoft/apm-sample-package"], "github-app": { - "app-id": "${{ vars.APP_ID }}", + "client-id": "${{ vars.APP_ID }}", "private-key": "${{ secrets.APP_PRIVATE_KEY }}" } } @@ -1900,7 +1900,7 @@ "description": "GitHub App configuration for minting a token used in pre-activation reactions, activation status comments, and skip-if search queries. When configured, a single GitHub App installation access token is minted and shared across all these operations instead of using the default GITHUB_TOKEN. Can be defined in a shared agentic workflow and inherited by importing workflows.", "examples": [ { - "app-id": "${{ vars.APP_ID }}", + "client-id": "${{ vars.APP_ID }}", "private-key": "${{ secrets.APP_PRIVATE_KEY }}" } ] @@ -9865,7 +9865,12 @@ "properties": { "app-id": { "type": "string", - "description": "GitHub App ID (e.g., '${{ vars.APP_ID }}'). Required to mint a GitHub App token.", + "description": "Deprecated alias for client-id. GitHub App ID/client ID (e.g., '${{ vars.APP_ID }}').", + "examples": ["${{ vars.APP_ID }}"] + }, + "client-id": { + "type": "string", + "description": "GitHub App client ID (e.g., '${{ vars.APP_ID }}'). Required to mint a GitHub App token.", "examples": ["${{ vars.APP_ID }}"] }, "private-key": { @@ -9889,11 +9894,18 @@ "description": "Optional extra GitHub App-only permissions to merge into the minted token. Only takes effect for tools.github.github-app; ignored in other github-app contexts." } }, - "required": ["app-id", "private-key"], + "anyOf": [ + { + "required": ["client-id", "private-key"] + }, + { + "required": ["app-id", "private-key"] + } + ], "additionalProperties": false, "examples": [ { - "app-id": "${{ vars.APP_ID }}", + "client-id": "${{ vars.APP_ID }}", "private-key": "${{ secrets.APP_PRIVATE_KEY }}" } ] diff --git a/pkg/workflow/activation_github_token_test.go b/pkg/workflow/activation_github_token_test.go index 170f258c61a..a0092332863 100644 --- a/pkg/workflow/activation_github_token_test.go +++ b/pkg/workflow/activation_github_token_test.go @@ -117,7 +117,7 @@ func TestActivationGitHubApp(t *testing.T) { // Reaction step should use the app token assert.Contains(t, stepsStr, "github-token: ${{ steps.activation-app-token.outputs.token }}", "Reaction step should use app token") // App-id and private-key should be in the mint step - assert.Contains(t, stepsStr, "app-id: ${{ vars.APP_ID }}", "Mint step should contain app-id") + assert.Contains(t, stepsStr, "client-id: ${{ vars.APP_ID }}", "Mint step should contain client-id") assert.Contains(t, stepsStr, "private-key: ${{ secrets.APP_PRIVATE_KEY }}", "Mint step should contain private-key") }) @@ -349,7 +349,7 @@ Do something useful. lockStr := string(lockContent) // The token mint step should be generated assert.Contains(t, lockStr, "id: activation-app-token", "Token mint step should be generated") - assert.Contains(t, lockStr, "app-id: ${{ vars.APP_ID }}", "Token mint step should use app-id") + assert.Contains(t, lockStr, "client-id: ${{ vars.APP_ID }}", "Token mint step should use client-id") assert.Contains(t, lockStr, "github-token: ${{ steps.activation-app-token.outputs.token }}", "Reaction step should use app token") }) } diff --git a/pkg/workflow/checkout_config_parser.go b/pkg/workflow/checkout_config_parser.go index cf86baf6473..84220afcd56 100644 --- a/pkg/workflow/checkout_config_parser.go +++ b/pkg/workflow/checkout_config_parser.go @@ -123,7 +123,7 @@ func checkoutConfigFromMap(m map[string]any) (*CheckoutConfig, error) { } cfg.GitHubApp = parseAppConfig(appMap) if cfg.GitHubApp.AppID == "" || cfg.GitHubApp.PrivateKey == "" { - return nil, errors.New("checkout.github-app requires both app-id and private-key") + return nil, errors.New("checkout.github-app requires both client-id (or app-id) and private-key") } } diff --git a/pkg/workflow/checkout_manager_test.go b/pkg/workflow/checkout_manager_test.go index 135858d707f..d962a4cd371 100644 --- a/pkg/workflow/checkout_manager_test.go +++ b/pkg/workflow/checkout_manager_test.go @@ -301,6 +301,21 @@ func TestParseCheckoutConfigs(t *testing.T) { assert.Equal(t, []string{"repo-a", "repo-b"}, configs[0].GitHubApp.Repositories) }) + t.Run("github-app config accepts client-id", func(t *testing.T) { + raw := map[string]any{ + "repository": "owner/target-repo", + "github-app": map[string]any{ + "client-id": "${{ vars.CLIENT_ID }}", + "private-key": "${{ secrets.APP_PRIVATE_KEY }}", + }, + } + configs, err := ParseCheckoutConfigs(raw) + require.NoError(t, err, "github-app config with client-id should parse without error") + require.Len(t, configs, 1) + require.NotNil(t, configs[0].GitHubApp, "github-app config should be set") + assert.Equal(t, "${{ vars.CLIENT_ID }}", configs[0].GitHubApp.AppID, "client-id should populate AppID") + }) + t.Run("github-token and github-app are mutually exclusive", func(t *testing.T) { raw := map[string]any{ "github-token": "${{ secrets.MY_TOKEN }}", @@ -322,7 +337,7 @@ func TestParseCheckoutConfigs(t *testing.T) { } _, err := ParseCheckoutConfigs(raw) require.Error(t, err, "github-app without app-id should return error") - assert.Contains(t, err.Error(), "app-id and private-key") + assert.Contains(t, err.Error(), "client-id (or app-id) and private-key") }) t.Run("github-app config missing private-key returns error", func(t *testing.T) { @@ -333,7 +348,7 @@ func TestParseCheckoutConfigs(t *testing.T) { } _, err := ParseCheckoutConfigs(raw) require.Error(t, err, "github-app without private-key should return error") - assert.Contains(t, err.Error(), "app-id and private-key") + assert.Contains(t, err.Error(), "client-id (or app-id) and private-key") }) t.Run("github-app must be an object", func(t *testing.T) { diff --git a/pkg/workflow/compiler_github_mcp_steps.go b/pkg/workflow/compiler_github_mcp_steps.go index aa92bba7af9..29374bf1c9b 100644 --- a/pkg/workflow/compiler_github_mcp_steps.go +++ b/pkg/workflow/compiler_github_mcp_steps.go @@ -98,7 +98,7 @@ func (c *Compiler) generateGitHubMCPAppTokenMintingSteps(data *WorkflowData) []s } app := data.ParsedTools.GitHub.GitHubApp - githubConfigLog.Printf("Generating GitHub App token minting step for GitHub MCP server: app-id=%s", app.AppID) + githubConfigLog.Printf("Generating GitHub App token minting step for GitHub MCP server: client-id=%s", app.AppID) // Get permissions from the agent job - parse from YAML string var permissions *Permissions diff --git a/pkg/workflow/compiler_pre_activation_job.go b/pkg/workflow/compiler_pre_activation_job.go index 0c84b1e2b56..4a7b286cb19 100644 --- a/pkg/workflow/compiler_pre_activation_job.go +++ b/pkg/workflow/compiler_pre_activation_job.go @@ -540,7 +540,7 @@ func (c *Compiler) buildPreActivationAppTokenMintStep(app *GitHubAppConfig) []st steps = append(steps, fmt.Sprintf(" id: %s\n", tokenStepID)) steps = append(steps, fmt.Sprintf(" uses: %s\n", getActionPin("actions/create-github-app-token"))) steps = append(steps, " with:\n") - steps = append(steps, fmt.Sprintf(" app-id: %s\n", app.AppID)) + steps = append(steps, fmt.Sprintf(" client-id: %s\n", app.AppID)) steps = append(steps, fmt.Sprintf(" private-key: %s\n", app.PrivateKey)) owner := app.Owner diff --git a/pkg/workflow/frontmatter_extraction_yaml.go b/pkg/workflow/frontmatter_extraction_yaml.go index 8031117b451..45a6432ff7e 100644 --- a/pkg/workflow/frontmatter_extraction_yaml.go +++ b/pkg/workflow/frontmatter_extraction_yaml.go @@ -857,7 +857,7 @@ func (c *Compiler) extractLabelCommandConfig(frontmatter map[string]any) (labelN // isGitHubAppNestedField returns true if the trimmed YAML line represents a known // nested field or array item inside an on.github-app object. func isGitHubAppNestedField(trimmedLine string) bool { - githubAppFields := []string{"app-id:", "private-key:", "owner:", "repositories:"} + githubAppFields := []string{"app-id:", "client-id:", "private-key:", "owner:", "repositories:"} for _, field := range githubAppFields { if strings.HasPrefix(trimmedLine, field) { return true diff --git a/pkg/workflow/github_app_permissions_validation.go b/pkg/workflow/github_app_permissions_validation.go index 83328412410..c3ce915fe7d 100644 --- a/pkg/workflow/github_app_permissions_validation.go +++ b/pkg/workflow/github_app_permissions_validation.go @@ -255,13 +255,13 @@ func formatGitHubAppRequiredError(appOnlyScopes []PermissionScope) error { lines = append(lines, ""+"tools:") lines = append(lines, " github:") lines = append(lines, " github-app:") - lines = append(lines, " app-id: ${{ vars.APP_ID }}") + lines = append(lines, " client-id: ${{ vars.APP_ID }}") lines = append(lines, " private-key: ${{ secrets.APP_PRIVATE_KEY }}") lines = append(lines, "") lines = append(lines, "Or in the safe-outputs section:") lines = append(lines, "safe-outputs:") lines = append(lines, " github-app:") - lines = append(lines, " app-id: ${{ vars.APP_ID }}") + lines = append(lines, " client-id: ${{ vars.APP_ID }}") lines = append(lines, " private-key: ${{ secrets.APP_PRIVATE_KEY }}") return errors.New(strings.Join(lines, "\n")) diff --git a/pkg/workflow/github_mcp_app_token_test.go b/pkg/workflow/github_mcp_app_token_test.go index 55a0f5c562a..be4c9cd7e14 100644 --- a/pkg/workflow/github_mcp_app_token_test.go +++ b/pkg/workflow/github_mcp_app_token_test.go @@ -100,7 +100,7 @@ Test workflow with GitHub MCP app token minting. assert.Contains(t, lockContent, "Generate GitHub App token", "Token minting step should be present") assert.Contains(t, lockContent, "actions/create-github-app-token", "Should use create-github-app-token action") assert.Contains(t, lockContent, "id: github-mcp-app-token", "Should use github-mcp-app-token as step ID") - assert.Contains(t, lockContent, "app-id: ${{ vars.APP_ID }}", "Should use configured app ID") + assert.Contains(t, lockContent, "client-id: ${{ vars.APP_ID }}", "Should use configured app ID") assert.Contains(t, lockContent, "private-key: ${{ secrets.APP_PRIVATE_KEY }}", "Should use configured private key") // Verify permissions are passed to the app token minting @@ -259,7 +259,7 @@ Test org-wide GitHub MCP app token. // Verify other fields are still present assert.Contains(t, lockContent, "owner:", "Should include owner field") - assert.Contains(t, lockContent, "app-id:", "Should include app-id field") + assert.Contains(t, lockContent, "client-id:", "Should include client-id field") } // TestGitHubMCPAppTokenWithLockdownDetectionStep tests that determine-automatic-lockdown diff --git a/pkg/workflow/safe_outputs_app_config.go b/pkg/workflow/safe_outputs_app_config.go index f7276c19130..df67ab6a9ce 100644 --- a/pkg/workflow/safe_outputs_app_config.go +++ b/pkg/workflow/safe_outputs_app_config.go @@ -16,7 +16,7 @@ var safeOutputsAppLog = logger.New("workflow:safe_outputs_app") // GitHubAppConfig holds configuration for GitHub App-based token minting type GitHubAppConfig struct { - AppID string `yaml:"app-id,omitempty"` // GitHub App ID (e.g., "${{ vars.APP_ID }}") + AppID string `yaml:"client-id,omitempty"` // GitHub App client ID (or legacy app ID) (e.g., "${{ vars.APP_ID }}") PrivateKey string `yaml:"private-key,omitempty"` // GitHub App private key (e.g., "${{ secrets.APP_PRIVATE_KEY }}") Owner string `yaml:"owner,omitempty"` // Optional: owner of the GitHub App installation (defaults to current repository owner) Repositories []string `yaml:"repositories,omitempty"` // Optional: comma or newline-separated list of repositories to grant access to @@ -32,8 +32,13 @@ func parseAppConfig(appMap map[string]any) *GitHubAppConfig { safeOutputsAppLog.Print("Parsing GitHub App configuration") appConfig := &GitHubAppConfig{} - // Parse app-id (required) - if appID, exists := appMap["app-id"]; exists { + // Parse client-id/app-id (required) + // Prefer client-id when both are provided; app-id is accepted for backward compatibility. + if clientID, exists := appMap["client-id"]; exists { + if clientIDStr, ok := clientID.(string); ok { + appConfig.AppID = clientIDStr + } + } else if appID, exists := appMap["app-id"]; exists { if appIDStr, ok := appID.(string); ok { appConfig.AppID = appIDStr } @@ -148,7 +153,7 @@ func (c *Compiler) buildGitHubAppTokenMintStep(app *GitHubAppConfig, permissions steps = append(steps, " id: safe-outputs-app-token\n") steps = append(steps, fmt.Sprintf(" uses: %s\n", getActionPin("actions/create-github-app-token"))) steps = append(steps, " with:\n") - steps = append(steps, fmt.Sprintf(" app-id: %s\n", app.AppID)) + steps = append(steps, fmt.Sprintf(" client-id: %s\n", app.AppID)) steps = append(steps, fmt.Sprintf(" private-key: %s\n", app.PrivateKey)) // Add owner - default to current repository owner if not specified @@ -418,7 +423,7 @@ func (c *Compiler) buildActivationAppTokenMintStep(app *GitHubAppConfig, permiss steps = append(steps, " id: activation-app-token\n") steps = append(steps, fmt.Sprintf(" uses: %s\n", getActionPin("actions/create-github-app-token"))) steps = append(steps, " with:\n") - steps = append(steps, fmt.Sprintf(" app-id: %s\n", app.AppID)) + steps = append(steps, fmt.Sprintf(" client-id: %s\n", app.AppID)) steps = append(steps, fmt.Sprintf(" private-key: %s\n", app.PrivateKey)) // Add owner - default to current repository owner if not specified diff --git a/pkg/workflow/safe_outputs_validation.go b/pkg/workflow/safe_outputs_validation.go index 8f119a6d5c8..ff155eb247d 100644 --- a/pkg/workflow/safe_outputs_validation.go +++ b/pkg/workflow/safe_outputs_validation.go @@ -345,7 +345,7 @@ func validateSafeOutputsAllowWorkflows(safeOutputs *SafeOutputsConfig) error { "Add a GitHub App configuration to safe-outputs:\n\n"+ "safe-outputs:\n"+ " github-app:\n"+ - " app-id: ${{ vars.APP_ID }}\n"+ + " client-id: ${{ vars.APP_ID }}\n"+ " private-key: ${{ secrets.APP_PRIVATE_KEY }}\n"+ " %s:\n"+ " allow-workflows: true", diff --git a/pkg/workflow/skip_if_match_test.go b/pkg/workflow/skip_if_match_test.go index 2d1ffc7a643..0808f77caf8 100644 --- a/pkg/workflow/skip_if_match_test.go +++ b/pkg/workflow/skip_if_match_test.go @@ -427,9 +427,9 @@ This workflow uses a GitHub App token for org-wide search. t.Error("Expected unified GitHub App token mint step to be present") } - // Verify app-id and private-key are in the mint step - if !strings.Contains(lockContentStr, "app-id: ${{ secrets.WORKFLOW_APP_ID }}") { - t.Error("Expected app-id in the GitHub App token mint step") + // Verify client-id and private-key are in the mint step + if !strings.Contains(lockContentStr, "client-id: ${{ secrets.WORKFLOW_APP_ID }}") { + t.Error("Expected client-id in the GitHub App token mint step") } if !strings.Contains(lockContentStr, "private-key: ${{ secrets.WORKFLOW_APP_PRIVATE_KEY }}") { t.Error("Expected private-key in the GitHub App token mint step") diff --git a/pkg/workflow/skip_if_no_match_test.go b/pkg/workflow/skip_if_no_match_test.go index 6f4abb5d619..3d828207fe2 100644 --- a/pkg/workflow/skip_if_no_match_test.go +++ b/pkg/workflow/skip_if_no_match_test.go @@ -486,9 +486,9 @@ This workflow uses a GitHub App token for org-wide search. t.Error("Expected unified GitHub App token mint step to be present") } - // Verify app-id and private-key are in the mint step - if !strings.Contains(lockContentStr, "app-id: ${{ secrets.WORKFLOW_APP_ID }}") { - t.Error("Expected app-id in the GitHub App token mint step") + // Verify client-id and private-key are in the mint step + if !strings.Contains(lockContentStr, "client-id: ${{ secrets.WORKFLOW_APP_ID }}") { + t.Error("Expected client-id in the GitHub App token mint step") } if !strings.Contains(lockContentStr, "private-key: ${{ secrets.WORKFLOW_APP_PRIVATE_KEY }}") { t.Error("Expected private-key in the GitHub App token mint step") diff --git a/pkg/workflow/top_level_github_app_integration_test.go b/pkg/workflow/top_level_github_app_integration_test.go index 339e9d39288..187efcf9924 100644 --- a/pkg/workflow/top_level_github_app_integration_test.go +++ b/pkg/workflow/top_level_github_app_integration_test.go @@ -53,7 +53,7 @@ Test workflow verifying top-level github-app fallback for safe-outputs. // The safe-outputs job should use the top-level github-app for token minting assert.Contains(t, compiled, "id: safe-outputs-app-token", "Safe outputs job should generate a token minting step") - assert.Contains(t, compiled, "app-id: ${{ vars.APP_ID }}", + assert.Contains(t, compiled, "client-id: ${{ vars.APP_ID }}", "Token minting step should use the top-level APP_ID") assert.Contains(t, compiled, "private-key: ${{ secrets.APP_PRIVATE_KEY }}", "Token minting step should use the top-level APP_PRIVATE_KEY") @@ -94,7 +94,7 @@ Test workflow verifying top-level github-app fallback for activation. // The activation job should use the top-level github-app for token minting assert.Contains(t, compiled, "id: activation-app-token", "Activation job should generate a token minting step using top-level github-app") - assert.Contains(t, compiled, "app-id: ${{ vars.APP_ID }}", + assert.Contains(t, compiled, "client-id: ${{ vars.APP_ID }}", "Token minting step should use the top-level APP_ID") // The reaction step should use the minted app token assert.Contains(t, compiled, "github-token: ${{ steps.activation-app-token.outputs.token }}", @@ -138,7 +138,7 @@ Test workflow verifying top-level github-app fallback for checkout. // The checkout should use the top-level github-app for token minting assert.Contains(t, compiled, "id: checkout-app-token-0", "Checkout should generate a token minting step using top-level github-app") - assert.Contains(t, compiled, "app-id: ${{ vars.APP_ID }}", + assert.Contains(t, compiled, "client-id: ${{ vars.APP_ID }}", "Token minting step should use the top-level APP_ID") }) @@ -182,7 +182,7 @@ Test workflow verifying top-level github-app fallback for tools.github. // The agent job should use the top-level github-app for GitHub MCP token minting assert.Contains(t, compiled, "id: github-mcp-app-token", "Agent job should generate a GitHub MCP token minting step using top-level github-app") - assert.Contains(t, compiled, "app-id: ${{ vars.APP_ID }}", + assert.Contains(t, compiled, "client-id: ${{ vars.APP_ID }}", "Token minting step should use the top-level APP_ID") }) @@ -225,15 +225,15 @@ Test workflow verifying section-specific github-app takes precedence over top-le compiled := string(compiledBytes) // The safe-outputs job should use SAFE_OUTPUTS_APP_ID (section-specific), not APP_ID (top-level) - assert.Contains(t, compiled, "app-id: ${{ vars.SAFE_OUTPUTS_APP_ID }}", + assert.Contains(t, compiled, "client-id: ${{ vars.SAFE_OUTPUTS_APP_ID }}", "Safe outputs job should use section-specific SAFE_OUTPUTS_APP_ID") - assert.Contains(t, compiled, "app-id: ${{ vars.ACTIVATION_APP_ID }}", + assert.Contains(t, compiled, "client-id: ${{ vars.ACTIVATION_APP_ID }}", "Activation job should use section-specific ACTIVATION_APP_ID") // The top-level APP_ID should NOT appear anywhere because it's overridden by section-specific // configs in both on.github-app and safe-outputs.github-app. SAFE_OUTPUTS_APP_ID and // ACTIVATION_APP_ID are distinct values from APP_ID, so their presence does not conflict // with this assertion. - assert.NotContains(t, compiled, "app-id: ${{ vars.APP_ID }}", + assert.NotContains(t, compiled, "client-id: ${{ vars.APP_ID }}", "Top-level APP_ID should NOT be used when section-specific configs are present") }) @@ -288,7 +288,7 @@ func TestTopLevelGitHubAppWorkflowFiles(t *testing.T) { workflowFile: "../cli/workflows/test-top-level-github-app-safe-outputs.md", expectContains: []string{ "id: safe-outputs-app-token", - "app-id: ${{ vars.APP_ID }}", + "client-id: ${{ vars.APP_ID }}", "private-key: ${{ secrets.APP_PRIVATE_KEY }}", }, }, @@ -297,7 +297,7 @@ func TestTopLevelGitHubAppWorkflowFiles(t *testing.T) { workflowFile: "../cli/workflows/test-top-level-github-app-activation.md", expectContains: []string{ "id: activation-app-token", - "app-id: ${{ vars.APP_ID }}", + "client-id: ${{ vars.APP_ID }}", "github-token: ${{ steps.activation-app-token.outputs.token }}", }, }, @@ -306,15 +306,15 @@ func TestTopLevelGitHubAppWorkflowFiles(t *testing.T) { workflowFile: "../cli/workflows/test-top-level-github-app-checkout.md", expectContains: []string{ "id: checkout-app-token-0", - "app-id: ${{ vars.APP_ID }}", + "client-id: ${{ vars.APP_ID }}", }, }, { name: "section-specific override workflow file", workflowFile: "../cli/workflows/test-top-level-github-app-override.md", expectContains: []string{ - "app-id: ${{ vars.SAFE_OUTPUTS_APP_ID }}", - "app-id: ${{ vars.ACTIVATION_APP_ID }}", + "client-id: ${{ vars.SAFE_OUTPUTS_APP_ID }}", + "client-id: ${{ vars.ACTIVATION_APP_ID }}", }, }, { @@ -322,7 +322,7 @@ func TestTopLevelGitHubAppWorkflowFiles(t *testing.T) { workflowFile: "../cli/workflows/test-top-level-github-app-mcp.md", expectContains: []string{ "id: github-mcp-app-token", - "app-id: ${{ vars.APP_ID }}", + "client-id: ${{ vars.APP_ID }}", }, }, } diff --git a/pkg/workflow/workflow_github_app.go b/pkg/workflow/workflow_github_app.go index 6c444448c5d..c3fe464a0bf 100644 --- a/pkg/workflow/workflow_github_app.go +++ b/pkg/workflow/workflow_github_app.go @@ -106,7 +106,7 @@ func applyTopLevelGitHubAppFallbacks(data *WorkflowData) { // processOnSectionAndFilters) does not lose the fallback when it rebuilds ParsedTools // from the map. appMap := map[string]any{ - "app-id": fallback.AppID, + "client-id": fallback.AppID, "private-key": fallback.PrivateKey, } if fallback.Owner != "" {