From 042316ea51f6a3e719c51e80db727d54f5be31d7 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 16 Mar 2026 14:27:24 +0000 Subject: [PATCH 1/6] Initial plan From 5633a2679ef657e8f5adc008e3e8c0c84bb4fe2d Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 16 Mar 2026 15:01:21 +0000 Subject: [PATCH 2/6] feat: support github-app auth and per-dependency overrides for dependencies (APM) - Option B - Add APMPackageEntry struct with optional per-package github-app override - Update APMDependenciesInfo with Entries, GitHubApp fields and HasGitHubApp() helper - Update frontmatter parser to support: default github-app at top level + per-package object entries - Add groupAPMEntriesByApp: groups packages by effective app config for batch token minting - Add generateAPMAppTokenMintStep: generates GitHub App token step before APM pack - Add GenerateAPMPackStepForGroup: generates indexed pack steps with GITHUB_TOKEN env override - Update activation job compiler to generate N token+pack+upload steps per group - Update main job compiler to generate N download steps per group - Update JSON schema to allow new dependency formats - Add unit tests for new parsing, grouping, and step generation - Add integration tests for github-app compilation paths - Maintain full backward compatibility: no github-app = same output as before Closes # Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/parser/schemas/main_workflow_schema.json | 112 ++++++- pkg/workflow/apm_dependencies.go | 195 ++++++++++- .../apm_dependencies_compilation_test.go | 145 +++++++++ pkg/workflow/apm_dependencies_test.go | 302 +++++++++++++++++- pkg/workflow/compiler_activation_job.go | 59 ++-- pkg/workflow/compiler_yaml_main_job.go | 30 +- .../frontmatter_extraction_metadata.go | 73 ++++- pkg/workflow/frontmatter_types.go | 34 +- 8 files changed, 893 insertions(+), 57 deletions(-) diff --git a/pkg/parser/schemas/main_workflow_schema.json b/pkg/parser/schemas/main_workflow_schema.json index 21014dfdaed..a163a32725e 100644 --- a/pkg/parser/schemas/main_workflow_schema.json +++ b/pkg/parser/schemas/main_workflow_schema.json @@ -322,7 +322,7 @@ { "type": "array", "minItems": 1, - "description": "Array of label names — any of these labels will trigger the workflow.", + "description": "Array of label names \u2014 any of these labels will trigger the workflow.", "items": { "type": "string", "minLength": 1, @@ -343,7 +343,7 @@ { "type": "array", "minItems": 1, - "description": "Array of label names — any of these labels will trigger the workflow.", + "description": "Array of label names \u2014 any of these labels will trigger the workflow.", "items": { "type": "string", "minLength": 1, @@ -7845,18 +7845,41 @@ ] }, "dependencies": { - "description": "APM package references to install. Supports array format (list of package slugs) or object format with packages and isolated fields.", + "description": "APM package references to install. Supports array format (list of package slugs), object format with packages and optional isolated/github-app fields, and per-package github-app overrides for cross-org access.", "examples": [ ["microsoft/apm-sample-package", "acme/custom-tools"], { "packages": ["microsoft/apm-sample-package"], "isolated": true + }, + { + "github-app": { + "app-id": "${{ vars.APP_ID }}", + "private-key": "${{ secrets.APP_PRIVATE_KEY }}" + }, + "packages": ["acme-platform-org/acme-skills/plugins/dev-tools"] + }, + { + "github-app": { + "app-id": "${{ vars.APP_ID }}", + "private-key": "${{ secrets.APP_PRIVATE_KEY }}" + }, + "packages": [ + "acme-platform-org/acme-skills/plugins/dev-tools", + { + "source": "partner-org/partner-package", + "github-app": { + "app-id": "${{ vars.PARTNER_APP_ID }}", + "private-key": "${{ secrets.PARTNER_APP_PRIVATE_KEY }}" + } + } + ] } ], "oneOf": [ { "type": "array", - "description": "Simple array of APM package references.", + "description": "Simple array of APM package references (no auth required).", "items": { "type": "string", "pattern": "^[a-zA-Z0-9_-]+/[a-zA-Z0-9_./-]+$", @@ -7865,20 +7888,91 @@ }, { "type": "object", - "description": "Object format with packages and optional isolated flag.", + "description": "Object format with packages, optional isolated flag, and optional github-app for cross-org access.", "properties": { "packages": { "type": "array", - "description": "List of APM package references to install.", + "description": "List of APM package references. Each entry is either a string or an object with source and optional github-app override.", "items": { - "type": "string", - "pattern": "^[a-zA-Z0-9_-]+/[a-zA-Z0-9_./-]+$", - "description": "APM package reference in the format 'org/repo' or 'org/repo/path/to/skill'" + "oneOf": [ + { + "type": "string", + "pattern": "^[a-zA-Z0-9_-]+/[a-zA-Z0-9_./-]+$", + "description": "APM package reference in the format 'org/repo' or 'org/repo/path/to/skill'" + }, + { + "type": "object", + "description": "Object format for per-package github-app override.", + "properties": { + "source": { + "type": "string", + "pattern": "^[a-zA-Z0-9_-]+/[a-zA-Z0-9_./-]+$", + "description": "APM package reference in the format 'org/repo' or 'org/repo/path/to/skill'" + }, + "github-app": { + "type": "object", + "description": "GitHub App configuration for generating a token with cross-org access.", + "properties": { + "app-id": { + "type": "string", + "description": "GitHub App ID or reference (e.g. '${{ vars.APP_ID }}')." + }, + "private-key": { + "type": "string", + "description": "GitHub App private key or reference (e.g. '${{ secrets.APP_PRIVATE_KEY }}')." + }, + "owner": { + "type": "string", + "description": "Optional GitHub App installation owner. Defaults to the repository owner." + }, + "repositories": { + "type": "array", + "items": { + "type": "string" + }, + "description": "Optional list of repositories to grant access to. Use ['*'] for org-wide access." + } + }, + "required": ["app-id", "private-key"], + "additionalProperties": false + } + }, + "required": ["source", "github-app"], + "additionalProperties": false + } + ] } }, "isolated": { "type": "boolean", "description": "If true, agent restore step clears primitive dirs before unpacking." + }, + "github-app": { + "type": "object", + "description": "GitHub App configuration for generating a token with cross-org access.", + "properties": { + "app-id": { + "type": "string", + "description": "GitHub App ID or reference (e.g. '${{ vars.APP_ID }}')." + }, + "private-key": { + "type": "string", + "description": "GitHub App private key or reference (e.g. '${{ secrets.APP_PRIVATE_KEY }}')." + }, + "owner": { + "type": "string", + "description": "Optional GitHub App installation owner. Defaults to the repository owner." + }, + "repositories": { + "type": "array", + "items": { + "type": "string" + }, + "description": "Optional list of repositories to grant access to. Use ['*'] for org-wide access." + } + }, + "required": ["app-id", "private-key"], + "additionalProperties": false } }, "required": ["packages"], diff --git a/pkg/workflow/apm_dependencies.go b/pkg/workflow/apm_dependencies.go index f4b625be7c7..00d43226269 100644 --- a/pkg/workflow/apm_dependencies.go +++ b/pkg/workflow/apm_dependencies.go @@ -1,15 +1,153 @@ package workflow import ( + "fmt" + + "github.com/github/gh-aw/pkg/constants" "github.com/github/gh-aw/pkg/logger" ) var apmDepsLog = logger.New("workflow:apm_dependencies") +// APMPackageGroup represents a group of APM packages that share the same effective GitHub App config. +// Packages with per-package overrides are in their own group; packages without overrides share +// the default github-app group (or the "no-auth" group when no default is configured). +type APMPackageGroup struct { + GitHubApp *GitHubAppConfig // nil = use GITHUB_TOKEN; non-nil = mint a fresh token + Packages []string // Package sources in this group + Index int // Group index for deterministic step/artifact naming +} + +// GetPackageGroups returns the package groups derived from the dependency configuration. +// When no GitHub App is configured (simple case), a single group with no auth is returned. +// When a GitHub App is configured, entries are grouped by their effective app config so that +// packages sharing the same credentials are packed together in one APM step. +func (a *APMDependenciesInfo) GetPackageGroups() []APMPackageGroup { + return groupAPMEntriesByApp(a) +} + +// groupAPMEntriesByApp groups APM package entries by their effective GitHub App config. +// Entries without a per-package override use the default GitHubApp (or no app). +// Entries with identical AppID+PrivateKey combinations are placed in the same group. +func groupAPMEntriesByApp(deps *APMDependenciesInfo) []APMPackageGroup { + if !deps.HasGitHubApp() { + // Simple case: no GitHub App configured — single group, no token minting needed + return []APMPackageGroup{{Packages: deps.Packages, Index: 0}} + } + + type groupData struct { + app *GitHubAppConfig + packages []string + } + + groupMap := make(map[string]*groupData) // key = apmAppGroupKey + var groupOrder []string // preserves insertion order for determinism + + for _, entry := range deps.Entries { + effectiveApp := entry.GitHubApp + if effectiveApp == nil { + effectiveApp = deps.GitHubApp // fall back to the top-level default + } + key := apmAppGroupKey(effectiveApp) + if _, exists := groupMap[key]; !exists { + groupMap[key] = &groupData{app: effectiveApp} + groupOrder = append(groupOrder, key) + } + groupMap[key].packages = append(groupMap[key].packages, entry.Source) + } + + groups := make([]APMPackageGroup, len(groupOrder)) + for i, key := range groupOrder { + groups[i] = APMPackageGroup{ + GitHubApp: groupMap[key].app, + Packages: groupMap[key].packages, + Index: i, + } + } + return groups +} + +// apmAppGroupKey returns a stable string key for a GitHubAppConfig used to group packages. +// A nil config (= use GITHUB_TOKEN) maps to an empty string. +func apmAppGroupKey(app *GitHubAppConfig) string { + if app == nil { + return "" + } + return app.AppID + "|" + app.PrivateKey +} + +// apmArtifactBaseName returns the artifact base name for a given APMDependenciesInfo and group index. +// When GitHub App auth is NOT configured (simple case) the legacy name "apm" is used to +// preserve backward compatibility with previously compiled lock files. +// When GitHub App auth IS configured, artifacts are named "apm-N". +func apmArtifactBaseName(deps *APMDependenciesInfo, groupIndex int) string { + if !deps.HasGitHubApp() { + return constants.APMArtifactName // "apm" — backward compat + } + return fmt.Sprintf("%s-%d", constants.APMArtifactName, groupIndex) +} + +// apmPackStepID returns the step ID for the APM pack step of a given group. +// The simple-case (no GitHub App) keeps the legacy "apm_pack" step ID. +func apmPackStepID(deps *APMDependenciesInfo, groupIndex int) string { + if !deps.HasGitHubApp() { + return "apm_pack" // legacy + } + return fmt.Sprintf("apm_pack_%d", groupIndex) +} + +// apmAppTokenStepID returns the step ID for the GitHub App token mint step for a given group. +func apmAppTokenStepID(groupIndex int) string { + return fmt.Sprintf("apm-app-token-%d", groupIndex) +} + +// generateAPMAppTokenMintStep generates the step that mints a short-lived GitHub App +// installation token scoped for use in the APM pack step. +func (c *Compiler) generateAPMAppTokenMintStep(app *GitHubAppConfig, stepID string) []string { + effectiveOwner := app.Owner + if effectiveOwner == "" { + effectiveOwner = "${{ github.repository_owner }} (default)" + } + apmDepsLog.Printf("Generating APM GitHub App token mint step: id=%s, owner=%s", stepID, effectiveOwner) + var steps []string + + steps = append(steps, " - name: Generate GitHub App token for APM dependencies\n") + steps = append(steps, fmt.Sprintf(" id: %s\n", stepID)) + 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(" private-key: %s\n", app.PrivateKey)) + + owner := app.Owner + if owner == "" { + owner = "${{ github.repository_owner }}" + } + steps = append(steps, fmt.Sprintf(" owner: %s\n", owner)) + + if len(app.Repositories) == 1 && app.Repositories[0] == "*" { + // Org-wide access: omit repositories field + } else if len(app.Repositories) == 1 { + steps = append(steps, fmt.Sprintf(" repositories: %s\n", app.Repositories[0])) + } else if len(app.Repositories) > 1 { + steps = append(steps, " repositories: |-\n") + for _, repo := range app.Repositories { + steps = append(steps, fmt.Sprintf(" %s\n", repo)) + } + } else { + steps = append(steps, " repositories: ${{ github.event.repository.name }}\n") + } + + steps = append(steps, " github-api-url: ${{ github.api_url }}\n") + return steps +} + // GenerateAPMPackStep generates the GitHub Actions step that installs APM packages and // packs them into a bundle in the activation job. The step always uses isolated:true because // the activation job has no repo context to preserve. // +// This is the simple (backward-compatible) form: no GitHub App auth, single group. +// For the multi-group case use GenerateAPMPackStepForGroup. +// // Parameters: // - apmDeps: APM dependency configuration extracted from frontmatter // - target: APM target derived from the agentic engine (e.g. "copilot", "claude", "all") @@ -24,11 +162,12 @@ func GenerateAPMPackStep(apmDeps *APMDependenciesInfo, target string, data *Work apmDepsLog.Printf("Generating APM pack step: %d packages, target=%s", len(apmDeps.Packages), target) + stepID := apmPackStepID(apmDeps, 0) actionRef := GetActionPin("microsoft/apm-action") lines := []string{ " - name: Install and pack APM dependencies", - " id: apm_pack", + " id: " + stepID, " uses: " + actionRef, " with:", " dependencies: |", @@ -49,6 +188,60 @@ func GenerateAPMPackStep(apmDeps *APMDependenciesInfo, target string, data *Work return GitHubActionStep(lines) } +// GenerateAPMPackStepForGroup generates the APM pack step for a specific package group. +// Used when GitHub App auth is configured (Option B), where each group may use a different token. +// +// Parameters: +// - group: APMPackageGroup containing the packages and optional GitHub App config +// - target: APM target (e.g. "copilot", "claude", "all") +// - tokenStepID: Step ID of the preceding token-mint step (empty = use default GITHUB_TOKEN) +// - data: WorkflowData used for action pin resolution +func GenerateAPMPackStepForGroup(group APMPackageGroup, target string, tokenStepID string, data *WorkflowData) GitHubActionStep { + if len(group.Packages) == 0 { + apmDepsLog.Print("No APM packages in group to pack") + return GitHubActionStep{} + } + + apmDepsLog.Printf("Generating APM pack step for group %d: %d packages, target=%s, tokenStep=%s", + group.Index, len(group.Packages), target, tokenStepID) + + stepID := fmt.Sprintf("apm_pack_%d", group.Index) + workDir := fmt.Sprintf("/tmp/gh-aw/apm-workspace-%d", group.Index) + actionRef := GetActionPin("microsoft/apm-action") + + lines := []string{ + " - name: Install and pack APM dependencies", + " id: " + stepID, + " uses: " + actionRef, + } + + if tokenStepID != "" { + lines = append(lines, + " env:", + " GITHUB_TOKEN: ${{ steps."+tokenStepID+".outputs.token }}", + ) + } + + lines = append(lines, + " with:", + " dependencies: |", + ) + + for _, dep := range group.Packages { + lines = append(lines, " - "+dep) + } + + lines = append(lines, + " isolated: 'true'", + " pack: 'true'", + " archive: 'true'", + " target: "+target, + " working-directory: "+workDir, + ) + + return GitHubActionStep(lines) +} + // GenerateAPMRestoreStep generates the GitHub Actions step that restores APM packages // from a pre-packed bundle in the agent job. // diff --git a/pkg/workflow/apm_dependencies_compilation_test.go b/pkg/workflow/apm_dependencies_compilation_test.go index e9f0c5d9454..48a5b9717e7 100644 --- a/pkg/workflow/apm_dependencies_compilation_test.go +++ b/pkg/workflow/apm_dependencies_compilation_test.go @@ -232,3 +232,148 @@ Test with Claude engine target inference assert.Contains(t, lockContent, "target: claude", "Lock file should use claude target for claude engine") } + +func TestAPMDependenciesCompilationDefaultGitHubApp(t *testing.T) { + tmpDir := testutil.TempDir(t, "apm-deps-github-app-test") + + workflow := `--- +engine: claude +on: workflow_dispatch +permissions: + issues: read + pull-requests: read +dependencies: + github-app: + app-id: ${{ vars.APP_ID }} + private-key: ${{ secrets.APP_PRIVATE_KEY }} + packages: + - acme-platform-org/acme-skills/plugins/dev-tools + - acme-platform-org/another-package +--- + +Test with default github-app for cross-org APM access +` + + testFile := filepath.Join(tmpDir, "test-apm-github-app.md") + err := os.WriteFile(testFile, []byte(workflow), 0644) + require.NoError(t, err, "Failed to write test file") + + compiler := NewCompiler() + err = compiler.CompileWorkflow(testFile) + require.NoError(t, err, "Compilation should succeed") + + lockFile := strings.Replace(testFile, ".md", ".lock.yml", 1) + content, err := os.ReadFile(lockFile) + require.NoError(t, err, "Failed to read lock file") + + lockContent := string(content) + + // Activation job: token mint step before the pack step + assert.Contains(t, lockContent, "Generate GitHub App token for APM dependencies", + "Lock file should contain APM GitHub App token mint step") + assert.Contains(t, lockContent, "id: apm-app-token-0", + "Lock file should use indexed token step ID") + assert.Contains(t, lockContent, "${{ vars.APP_ID }}", + "Lock file should reference the app ID variable") + assert.Contains(t, lockContent, "${{ secrets.APP_PRIVATE_KEY }}", + "Lock file should reference the private key secret") + + // Activation job: pack step with GITHUB_TOKEN env override + assert.Contains(t, lockContent, "id: apm_pack_0", + "Lock file should use indexed pack step ID") + assert.Contains(t, lockContent, "GITHUB_TOKEN: ${{ steps.apm-app-token-0.outputs.token }}", + "Lock file should set GITHUB_TOKEN from app token mint step") + assert.Contains(t, lockContent, "- acme-platform-org/acme-skills/plugins/dev-tools", + "Lock file should list first dependency") + assert.Contains(t, lockContent, "- acme-platform-org/another-package", + "Lock file should list second dependency") + + // Activation job: artifact upload uses indexed name + assert.Contains(t, lockContent, "name: apm-0", + "Lock file should use indexed artifact name") + + // Agent job: download step uses indexed artifact name + assert.Contains(t, lockContent, "Download APM bundle artifact", + "Lock file should download APM bundle in agent job") + + // Agent job: single restore step handles all bundles + assert.Contains(t, lockContent, "Restore APM dependencies", + "Lock file should contain APM restore step") + assert.Contains(t, lockContent, "bundle: /tmp/gh-aw/apm-bundle/*.tar.gz", + "Lock file should restore from bundle path") +} + +func TestAPMDependenciesCompilationPerPackageGitHubApp(t *testing.T) { + tmpDir := testutil.TempDir(t, "apm-deps-per-pkg-app-test") + + workflow := `--- +engine: claude +on: workflow_dispatch +permissions: + issues: read + pull-requests: read +dependencies: + github-app: + app-id: ${{ vars.APP_ID }} + private-key: ${{ secrets.APP_PRIVATE_KEY }} + packages: + - acme-platform-org/acme-skills/plugins/dev-tools + - source: partner-org/partner-package + github-app: + app-id: ${{ vars.PARTNER_APP_ID }} + private-key: ${{ secrets.PARTNER_APP_PRIVATE_KEY }} +--- + +Test with per-package github-app overrides for multi-org APM access +` + + testFile := filepath.Join(tmpDir, "test-apm-per-pkg-app.md") + err := os.WriteFile(testFile, []byte(workflow), 0644) + require.NoError(t, err, "Failed to write test file") + + compiler := NewCompiler() + err = compiler.CompileWorkflow(testFile) + require.NoError(t, err, "Compilation should succeed") + + lockFile := strings.Replace(testFile, ".md", ".lock.yml", 1) + content, err := os.ReadFile(lockFile) + require.NoError(t, err, "Failed to read lock file") + + lockContent := string(content) + + // Group 0: default app for acme-platform-org package + assert.Contains(t, lockContent, "id: apm-app-token-0", + "Lock file should have token mint step for group 0") + assert.Contains(t, lockContent, "${{ vars.APP_ID }}", + "Lock file should reference default app ID") + assert.Contains(t, lockContent, "id: apm_pack_0", + "Lock file should have pack step for group 0") + assert.Contains(t, lockContent, "GITHUB_TOKEN: ${{ steps.apm-app-token-0.outputs.token }}", + "Pack step 0 should use token from group 0 mint") + assert.Contains(t, lockContent, "- acme-platform-org/acme-skills/plugins/dev-tools", + "Pack step 0 should list the acme package") + assert.Contains(t, lockContent, "name: apm-0", + "Artifact upload for group 0 should use indexed name") + + // Group 1: per-package app for partner-org package + assert.Contains(t, lockContent, "id: apm-app-token-1", + "Lock file should have token mint step for group 1") + assert.Contains(t, lockContent, "${{ vars.PARTNER_APP_ID }}", + "Lock file should reference partner app ID") + assert.Contains(t, lockContent, "id: apm_pack_1", + "Lock file should have pack step for group 1") + assert.Contains(t, lockContent, "GITHUB_TOKEN: ${{ steps.apm-app-token-1.outputs.token }}", + "Pack step 1 should use token from group 1 mint") + assert.Contains(t, lockContent, "- partner-org/partner-package", + "Pack step 1 should list the partner package") + assert.Contains(t, lockContent, "name: apm-1", + "Artifact upload for group 1 should use indexed name") + + // Agent job: two download steps (one per group) + downloadCount := strings.Count(lockContent, "Download APM bundle artifact") + assert.Equal(t, 2, downloadCount, "Agent job should have 2 APM bundle download steps") + + // Agent job: single restore step + assert.Contains(t, lockContent, "Restore APM dependencies", + "Lock file should contain APM restore step") +} diff --git a/pkg/workflow/apm_dependencies_test.go b/pkg/workflow/apm_dependencies_test.go index 77ab336218c..ace7b0f6516 100644 --- a/pkg/workflow/apm_dependencies_test.go +++ b/pkg/workflow/apm_dependencies_test.go @@ -16,6 +16,8 @@ func TestExtractAPMDependenciesFromFrontmatter(t *testing.T) { frontmatter map[string]any expectedDeps []string expectedIsolated bool + hasDefaultApp bool + perPackageApps int // number of entries that have their own github-app }{ { name: "No dependencies field", @@ -111,6 +113,100 @@ func TestExtractAPMDependenciesFromFrontmatter(t *testing.T) { }, expectedDeps: nil, }, + // New: github-app support + { + name: "Object format with default github-app", + frontmatter: map[string]any{ + "dependencies": map[string]any{ + "github-app": map[string]any{ + "app-id": "${{ vars.APP_ID }}", + "private-key": "${{ secrets.APP_PRIVATE_KEY }}", + }, + "packages": []any{ + "acme-platform-org/acme-skills/plugins/dev-tools", + }, + }, + }, + expectedDeps: []string{"acme-platform-org/acme-skills/plugins/dev-tools"}, + hasDefaultApp: true, + }, + { + name: "Object format with default github-app and repositories", + frontmatter: map[string]any{ + "dependencies": map[string]any{ + "github-app": map[string]any{ + "app-id": "${{ vars.APP_ID }}", + "private-key": "${{ secrets.APP_PRIVATE_KEY }}", + "repositories": []any{"*"}, + }, + "packages": []any{ + "acme-platform-org/acme-skills/plugins/dev-tools", + "acme-platform-org/another-package", + }, + }, + }, + expectedDeps: []string{"acme-platform-org/acme-skills/plugins/dev-tools", "acme-platform-org/another-package"}, + hasDefaultApp: true, + }, + { + name: "Object format with per-package github-app override", + frontmatter: map[string]any{ + "dependencies": map[string]any{ + "github-app": map[string]any{ + "app-id": "${{ vars.APP_ID }}", + "private-key": "${{ secrets.APP_PRIVATE_KEY }}", + }, + "packages": []any{ + "acme-platform-org/acme-skills/plugins/dev-tools", + map[string]any{ + "source": "partner-org/partner-package", + "github-app": map[string]any{ + "app-id": "${{ vars.PARTNER_APP_ID }}", + "private-key": "${{ secrets.PARTNER_APP_PRIVATE_KEY }}", + }, + }, + }, + }, + }, + expectedDeps: []string{"acme-platform-org/acme-skills/plugins/dev-tools", "partner-org/partner-package"}, + hasDefaultApp: true, + perPackageApps: 1, + }, + { + name: "Object format with only per-package github-app (no default)", + frontmatter: map[string]any{ + "dependencies": map[string]any{ + "packages": []any{ + map[string]any{ + "source": "partner-org/partner-package", + "github-app": map[string]any{ + "app-id": "${{ vars.PARTNER_APP_ID }}", + "private-key": "${{ secrets.PARTNER_APP_PRIVATE_KEY }}", + }, + }, + }, + }, + }, + expectedDeps: []string{"partner-org/partner-package"}, + hasDefaultApp: false, + perPackageApps: 1, + }, + { + name: "Object entry without source is skipped", + frontmatter: map[string]any{ + "dependencies": map[string]any{ + "packages": []any{ + map[string]any{ + "github-app": map[string]any{ + "app-id": "${{ vars.APP_ID }}", + "private-key": "${{ secrets.APP_PRIVATE_KEY }}", + }, + }, + }, + }, + }, + expectedDeps: nil, // empty source → skipped + }, } for _, tt := range tests { @@ -122,6 +218,136 @@ func TestExtractAPMDependenciesFromFrontmatter(t *testing.T) { require.NotNil(t, result, "Should return non-nil APMDependenciesInfo") assert.Equal(t, tt.expectedDeps, result.Packages, "Extracted packages should match expected") assert.Equal(t, tt.expectedIsolated, result.Isolated, "Isolated flag should match expected") + if tt.hasDefaultApp { + assert.NotNil(t, result.GitHubApp, "Default github-app should be present") + } else { + assert.Nil(t, result.GitHubApp, "Default github-app should be absent") + } + // Count entries with a per-package GitHubApp + perPkgCount := 0 + for _, e := range result.Entries { + if e.GitHubApp != nil { + perPkgCount++ + } + } + assert.Equal(t, tt.perPackageApps, perPkgCount, "Per-package github-app count should match") + } + }) + } +} + +func TestAPMDependenciesHasGitHubApp(t *testing.T) { + tests := []struct { + name string + deps *APMDependenciesInfo + expected bool + }{ + { + name: "No github-app configured", + deps: &APMDependenciesInfo{Packages: []string{"pkg1"}, Entries: []APMPackageEntry{{Source: "pkg1"}}}, + expected: false, + }, + { + name: "Default github-app configured", + deps: &APMDependenciesInfo{ + Packages: []string{"pkg1"}, + Entries: []APMPackageEntry{{Source: "pkg1"}}, + GitHubApp: &GitHubAppConfig{AppID: "123", PrivateKey: "key"}, + }, + expected: true, + }, + { + name: "Per-package github-app configured", + deps: &APMDependenciesInfo{ + Packages: []string{"pkg1"}, + Entries: []APMPackageEntry{ + {Source: "pkg1", GitHubApp: &GitHubAppConfig{AppID: "123", PrivateKey: "key"}}, + }, + }, + expected: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.expected, tt.deps.HasGitHubApp(), "HasGitHubApp should match expected") + }) + } +} + +func TestGroupAPMEntriesByApp(t *testing.T) { + tests := []struct { + name string + deps *APMDependenciesInfo + expectedGroups int + groupSizes []int // expected number of packages per group (in order) + }{ + { + name: "Simple case - no github-app gives single group", + deps: &APMDependenciesInfo{ + Packages: []string{"pkg1", "pkg2"}, + Entries: []APMPackageEntry{{Source: "pkg1"}, {Source: "pkg2"}}, + }, + expectedGroups: 1, + groupSizes: []int{2}, + }, + { + name: "Default github-app only - single group", + deps: &APMDependenciesInfo{ + Packages: []string{"pkg1", "pkg2"}, + Entries: []APMPackageEntry{{Source: "pkg1"}, {Source: "pkg2"}}, + GitHubApp: &GitHubAppConfig{AppID: "123", PrivateKey: "key"}, + }, + expectedGroups: 1, + groupSizes: []int{2}, + }, + { + name: "Default github-app + per-package override with different app - two groups", + deps: &APMDependenciesInfo{ + Packages: []string{"pkg1", "pkg2"}, + Entries: []APMPackageEntry{ + {Source: "pkg1"}, // uses default app + {Source: "pkg2", GitHubApp: &GitHubAppConfig{AppID: "456", PrivateKey: "key2"}}, + }, + GitHubApp: &GitHubAppConfig{AppID: "123", PrivateKey: "key1"}, + }, + expectedGroups: 2, + groupSizes: []int{1, 1}, + }, + { + name: "Two packages with same per-package github-app - merged into one group", + deps: &APMDependenciesInfo{ + Packages: []string{"pkg1", "pkg2"}, + Entries: []APMPackageEntry{ + {Source: "pkg1", GitHubApp: &GitHubAppConfig{AppID: "123", PrivateKey: "key"}}, + {Source: "pkg2", GitHubApp: &GitHubAppConfig{AppID: "123", PrivateKey: "key"}}, + }, + }, + expectedGroups: 1, + groupSizes: []int{2}, + }, + { + name: "Three packages across three different apps", + deps: &APMDependenciesInfo{ + Packages: []string{"pkg1", "pkg2", "pkg3"}, + Entries: []APMPackageEntry{ + {Source: "pkg1"}, // uses default app + {Source: "pkg2", GitHubApp: &GitHubAppConfig{AppID: "456", PrivateKey: "key2"}}, + {Source: "pkg3", GitHubApp: &GitHubAppConfig{AppID: "789", PrivateKey: "key3"}}, + }, + GitHubApp: &GitHubAppConfig{AppID: "123", PrivateKey: "key1"}, + }, + expectedGroups: 3, + groupSizes: []int{1, 1, 1}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + groups := groupAPMEntriesByApp(tt.deps) + require.Len(t, groups, tt.expectedGroups, "Number of groups should match") + for i, g := range groups { + assert.Equal(t, i, g.Index, "Group index should match position") + assert.Len(t, g.Packages, tt.groupSizes[i], "Group %d package count should match", i) } }) } @@ -169,7 +395,7 @@ func TestGenerateAPMPackStep(t *testing.T) { }, { name: "Single dependency with copilot target", - apmDeps: &APMDependenciesInfo{Packages: []string{"microsoft/apm-sample-package"}}, + apmDeps: &APMDependenciesInfo{Packages: []string{"microsoft/apm-sample-package"}, Entries: []APMPackageEntry{{Source: "microsoft/apm-sample-package"}}}, target: "copilot", expectedContains: []string{ "Install and pack APM dependencies", @@ -186,7 +412,7 @@ func TestGenerateAPMPackStep(t *testing.T) { }, { name: "Multiple dependencies with claude target", - apmDeps: &APMDependenciesInfo{Packages: []string{"microsoft/apm-sample-package", "github/skills/review"}}, + apmDeps: &APMDependenciesInfo{Packages: []string{"microsoft/apm-sample-package", "github/skills/review"}, Entries: []APMPackageEntry{{Source: "microsoft/apm-sample-package"}, {Source: "github/skills/review"}}}, target: "claude", expectedContains: []string{ "Install and pack APM dependencies", @@ -199,7 +425,7 @@ func TestGenerateAPMPackStep(t *testing.T) { }, { name: "All target for non-copilot/claude engine", - apmDeps: &APMDependenciesInfo{Packages: []string{"microsoft/apm-sample-package"}}, + apmDeps: &APMDependenciesInfo{Packages: []string{"microsoft/apm-sample-package"}, Entries: []APMPackageEntry{{Source: "microsoft/apm-sample-package"}}}, target: "all", expectedContains: []string{ "target: all", @@ -232,6 +458,76 @@ func TestGenerateAPMPackStep(t *testing.T) { } } +func TestGenerateAPMPackStepForGroup(t *testing.T) { + tests := []struct { + name string + group APMPackageGroup + target string + tokenStepID string + expectedContains []string + expectedEmpty bool + }{ + { + name: "Empty group returns empty step", + group: APMPackageGroup{Packages: []string{}, Index: 0}, + target: "copilot", + expectedEmpty: true, + }, + { + name: "Single package group without token", + group: APMPackageGroup{Packages: []string{"microsoft/apm-sample-package"}, Index: 0}, + target: "copilot", + tokenStepID: "", + expectedContains: []string{ + "Install and pack APM dependencies", + "id: apm_pack_0", + "microsoft/apm-action", + "- microsoft/apm-sample-package", + "target: copilot", + "working-directory: /tmp/gh-aw/apm-workspace-0", + }, + }, + { + name: "Group with GitHub App token", + group: APMPackageGroup{Packages: []string{"acme-org/acme-skills"}, Index: 1}, + target: "claude", + tokenStepID: "apm-app-token-1", + expectedContains: []string{ + "id: apm_pack_1", + "env:", + "GITHUB_TOKEN: ${{ steps.apm-app-token-1.outputs.token }}", + "- acme-org/acme-skills", + "target: claude", + "working-directory: /tmp/gh-aw/apm-workspace-1", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + data := &WorkflowData{Name: "test-workflow"} + step := GenerateAPMPackStepForGroup(tt.group, tt.target, tt.tokenStepID, data) + + if tt.expectedEmpty { + assert.Empty(t, step, "Step should be empty for empty group") + return + } + + require.NotEmpty(t, step, "Step should not be empty") + + var sb strings.Builder + for _, line := range step { + sb.WriteString(line + "\n") + } + combined := sb.String() + + for _, expected := range tt.expectedContains { + assert.Contains(t, combined, expected, "Step should contain: %s", expected) + } + }) + } +} + func TestGenerateAPMRestoreStep(t *testing.T) { tests := []struct { name string diff --git a/pkg/workflow/compiler_activation_job.go b/pkg/workflow/compiler_activation_job.go index a2b0a77f9f7..c2a539d9fbe 100644 --- a/pkg/workflow/compiler_activation_job.go +++ b/pkg/workflow/compiler_activation_job.go @@ -406,27 +406,50 @@ func (c *Compiler) buildActivationJob(data *WorkflowData, preActivationJobCreate compilerActivationJobLog.Print("Generating prompt in activation job") c.generatePromptInActivationJob(&steps, data, preActivationJobCreated, customJobsBeforeActivation) - // Generate APM pack step if dependencies are specified. - // The pack step runs after prompt generation and uploads as a separate "apm" artifact. + // Generate APM pack step(s) if dependencies are specified. + // When no GitHub App is configured: single pack step (backward compat). + // When a GitHub App is configured: one token-mint + pack + upload step per package group. if data.APMDependencies != nil && len(data.APMDependencies.Packages) > 0 { - compilerActivationJobLog.Printf("Adding APM pack step: %d packages", len(data.APMDependencies.Packages)) apmTarget := engine.GetAPMTarget() - apmPackStep := GenerateAPMPackStep(data.APMDependencies, apmTarget, data) - for _, line := range apmPackStep { - steps = append(steps, line+"\n") + apmGroups := data.APMDependencies.GetPackageGroups() + compilerActivationJobLog.Printf("Adding %d APM pack group(s): %d packages total", + len(apmGroups), len(data.APMDependencies.Packages)) + + for _, group := range apmGroups { + // Mint a GitHub App token before the pack step when the group requires one. + tokenStepID := "" + if group.GitHubApp != nil { + tokenStepID = apmAppTokenStepID(group.Index) + compilerActivationJobLog.Printf("Adding APM GitHub App token mint step for group %d", group.Index) + tokenSteps := c.generateAPMAppTokenMintStep(group.GitHubApp, tokenStepID) + steps = append(steps, tokenSteps...) + } + + // Generate the pack step for this group. + var packStep GitHubActionStep + if data.APMDependencies.HasGitHubApp() { + packStep = GenerateAPMPackStepForGroup(group, apmTarget, tokenStepID, data) + } else { + packStep = GenerateAPMPackStep(data.APMDependencies, apmTarget, data) + } + for _, line := range packStep { + steps = append(steps, line+"\n") + } + + // Upload the packed APM bundle as a separate artifact for the agent job to download. + // The path comes from the apm_pack / apm_pack_N step output `bundle-path`. + packID := apmPackStepID(data.APMDependencies, group.Index) + artifactBaseName := apmArtifactBaseName(data.APMDependencies, group.Index) + artifactName := artifactPrefixExprForActivationJob(data) + artifactBaseName + compilerActivationJobLog.Printf("Adding APM bundle artifact upload step for group %d (artifact=%s)", group.Index, artifactBaseName) + steps = append(steps, " - name: Upload APM bundle artifact\n") + steps = append(steps, " if: success()\n") + steps = append(steps, fmt.Sprintf(" uses: %s\n", GetActionPin("actions/upload-artifact"))) + steps = append(steps, " with:\n") + steps = append(steps, fmt.Sprintf(" name: %s\n", artifactName)) + steps = append(steps, fmt.Sprintf(" path: ${{ steps.%s.outputs.bundle-path }}\n", packID)) + steps = append(steps, " retention-days: 1\n") } - // Upload the packed APM bundle as a separate artifact for the agent job to download. - // The path comes from the apm_pack step output `bundle-path`, which microsoft/apm-action - // sets to the location of the packed .tar.gz archive. - compilerActivationJobLog.Print("Adding APM bundle artifact upload step") - apmArtifactName := artifactPrefixExprForActivationJob(data) + constants.APMArtifactName - steps = append(steps, " - name: Upload APM bundle artifact\n") - steps = append(steps, " if: success()\n") - steps = append(steps, fmt.Sprintf(" uses: %s\n", GetActionPin("actions/upload-artifact"))) - steps = append(steps, " with:\n") - steps = append(steps, fmt.Sprintf(" name: %s\n", apmArtifactName)) - steps = append(steps, " path: ${{ steps.apm_pack.outputs.bundle-path }}\n") - steps = append(steps, " retention-days: 1\n") } // Upload aw_info.json and prompt.txt as the activation artifact for the agent job to download. diff --git a/pkg/workflow/compiler_yaml_main_job.go b/pkg/workflow/compiler_yaml_main_job.go index 48fdbe037aa..086095ab382 100644 --- a/pkg/workflow/compiler_yaml_main_job.go +++ b/pkg/workflow/compiler_yaml_main_job.go @@ -237,19 +237,27 @@ func (c *Compiler) generateMainJobSteps(yaml *strings.Builder, data *WorkflowDat } } - // Add APM (Agent Package Manager) setup step if dependencies are specified + // Add APM (Agent Package Manager) setup step if dependencies are specified. + // When no GitHub App is configured: single download step (backward compat). + // When a GitHub App is configured: one download step per artifact group, all going to + // /tmp/gh-aw/apm-bundle so the single restore step can glob *.tar.gz across all groups. if data.APMDependencies != nil && len(data.APMDependencies.Packages) > 0 { - // Download the pre-packed APM bundle from the separate "apm" artifact. - // In workflow_call context, apply the per-invocation prefix to avoid name clashes. - compilerYamlLog.Printf("Adding APM bundle download step: %d packages", len(data.APMDependencies.Packages)) - apmArtifactName := artifactPrefixExprForDownstreamJob(data) + constants.APMArtifactName - yaml.WriteString(" - name: Download APM bundle artifact\n") - fmt.Fprintf(yaml, " uses: %s\n", GetActionPin("actions/download-artifact")) - yaml.WriteString(" with:\n") - fmt.Fprintf(yaml, " name: %s\n", apmArtifactName) - yaml.WriteString(" path: /tmp/gh-aw/apm-bundle\n") + apmGroups := data.APMDependencies.GetPackageGroups() + compilerYamlLog.Printf("Adding %d APM bundle download step(s): %d packages total", + len(apmGroups), len(data.APMDependencies.Packages)) + prefix := artifactPrefixExprForDownstreamJob(data) + + for _, group := range apmGroups { + artifactBaseName := apmArtifactBaseName(data.APMDependencies, group.Index) + artifactName := prefix + artifactBaseName + yaml.WriteString(" - name: Download APM bundle artifact\n") + fmt.Fprintf(yaml, " uses: %s\n", GetActionPin("actions/download-artifact")) + yaml.WriteString(" with:\n") + fmt.Fprintf(yaml, " name: %s\n", artifactName) + yaml.WriteString(" path: /tmp/gh-aw/apm-bundle\n") + } - // Restore APM dependencies from bundle + // Restore APM dependencies from all bundles in the shared directory compilerYamlLog.Printf("Adding APM restore step") apmStep := GenerateAPMRestoreStep(data.APMDependencies, data) for _, line := range apmStep { diff --git a/pkg/workflow/frontmatter_extraction_metadata.go b/pkg/workflow/frontmatter_extraction_metadata.go index 2f4ecb8aa9a..0662882719a 100644 --- a/pkg/workflow/frontmatter_extraction_metadata.go +++ b/pkg/workflow/frontmatter_extraction_metadata.go @@ -352,9 +352,13 @@ func extractPluginsFromFrontmatter(frontmatter map[string]any) *PluginInfo { } // extractAPMDependenciesFromFrontmatter extracts APM (Agent Package Manager) dependency -// configuration from frontmatter. Supports two formats: +// configuration from frontmatter. Supports the following formats: +// // - Array format: ["org/pkg1", "org/pkg2"] -// - Object format: {packages: ["org/pkg1", "org/pkg2"], isolated: true} +// - Object format with packages and isolated: {packages: [...], isolated: true} +// - Object format with a default github-app: {github-app: {...}, packages: [...]} +// - Object format with per-package github-app overrides: +// {github-app: {...}, packages: ["org/pkg1", {source: "org/pkg2", github-app: {...}}]} // // Returns nil if no dependencies field is present or if the field contains no packages. func extractAPMDependenciesFromFrontmatter(frontmatter map[string]any) *APMDependenciesInfo { @@ -364,7 +368,9 @@ func extractAPMDependenciesFromFrontmatter(frontmatter map[string]any) *APMDepen } var packages []string + var entries []APMPackageEntry var isolated bool + var defaultApp *GitHubAppConfig switch v := value.(type) { case []any: @@ -372,24 +378,61 @@ func extractAPMDependenciesFromFrontmatter(frontmatter map[string]any) *APMDepen for _, item := range v { if s, ok := item.(string); ok && s != "" { packages = append(packages, s) + entries = append(entries, APMPackageEntry{Source: s}) } } case map[string]any: - // Object format: dependencies: {packages: [...], isolated: true} - if pkgsAny, ok := v["packages"]; ok { - if pkgsArray, ok := pkgsAny.([]any); ok { - for _, item := range pkgsArray { - if s, ok := item.(string); ok && s != "" { - packages = append(packages, s) - } - } + // Object format: dependencies: {github-app: {...}, packages: [...], isolated: true} + + // Parse optional default github-app at the top level + if appAny, ok := v["github-app"]; ok { + if appMap, ok := appAny.(map[string]any); ok { + defaultApp = parseAppConfig(appMap) } } + + // Parse isolated flag if iso, ok := v["isolated"]; ok { if isoBool, ok := iso.(bool); ok { isolated = isoBool } } + + // Parse packages: each item is either a string or an object {source: "...", github-app: {...}} + if pkgsAny, ok := v["packages"]; ok { + if pkgsArray, ok := pkgsAny.([]any); ok { + for _, item := range pkgsArray { + switch pkg := item.(type) { + case string: + // Simple string entry + if pkg != "" { + packages = append(packages, pkg) + entries = append(entries, APMPackageEntry{Source: pkg}) + } + case map[string]any: + // Object entry: {source: "org/repo", github-app: {...}} + sourceAny, hasSource := pkg["source"] + if !hasSource { + frontmatterMetadataLog.Print("Skipping APM package entry: missing 'source' field") + continue + } + source, ok := sourceAny.(string) + if !ok || source == "" { + frontmatterMetadataLog.Print("Skipping APM package entry: 'source' field must be a non-empty string") + continue + } + entry := APMPackageEntry{Source: source} + if appAny, ok := pkg["github-app"]; ok { + if appMap, ok := appAny.(map[string]any); ok { + entry.GitHubApp = parseAppConfig(appMap) + } + } + packages = append(packages, source) + entries = append(entries, entry) + } + } + } + } default: return nil } @@ -398,6 +441,12 @@ func extractAPMDependenciesFromFrontmatter(frontmatter map[string]any) *APMDepen return nil } - frontmatterMetadataLog.Printf("Extracted %d APM dependency packages from frontmatter (isolated=%v)", len(packages), isolated) - return &APMDependenciesInfo{Packages: packages, Isolated: isolated} + frontmatterMetadataLog.Printf("Extracted %d APM dependency packages from frontmatter (isolated=%v, hasDefaultApp=%v)", + len(packages), isolated, defaultApp != nil) + return &APMDependenciesInfo{ + Packages: packages, + Entries: entries, + Isolated: isolated, + GitHubApp: defaultApp, + } } diff --git a/pkg/workflow/frontmatter_types.go b/pkg/workflow/frontmatter_types.go index 0395514511b..33a2b08eae6 100644 --- a/pkg/workflow/frontmatter_types.go +++ b/pkg/workflow/frontmatter_types.go @@ -86,12 +86,40 @@ type PluginsConfig struct { GitHubToken string `json:"github-token,omitempty"` // Custom GitHub token for plugin installation } +// APMPackageEntry represents a single APM package entry. +// Supports both simple string format (just the package source) and object format with an +// optional per-package GitHub App override (source: + github-app:). +type APMPackageEntry struct { + Source string // Package source (e.g., "org/repo" or "org/repo/path/to/skill") + GitHubApp *GitHubAppConfig // Optional per-package GitHub App override; nil means use the default +} + // APMDependenciesInfo encapsulates APM (Agent Package Manager) dependency configuration. -// Supports simple array format and object format with packages and isolated fields. +// Supports: +// - Simple array format: dependencies: [pkg1, pkg2] +// - Object format with packages and isolated: dependencies: {packages: [...], isolated: true} +// - Object format with a default github-app for cross-org access +// - Per-package github-app overrides for multi-org scenarios +// // When present, a pack step is emitted in the activation job and a restore step in the agent job. type APMDependenciesInfo struct { - Packages []string // APM package slugs to install (e.g., "org/package") - Isolated bool // If true, agent restore step clears primitive dirs before unpacking + Packages []string // Flat list of all APM package sources (for backward compat) + Entries []APMPackageEntry // Package entries with optional per-package GitHub App overrides + Isolated bool // If true, agent restore step clears primitive dirs before unpacking + GitHubApp *GitHubAppConfig // Default GitHub App for all packages that don't override it +} + +// HasGitHubApp returns true if any GitHub App is configured (default or per-package override). +func (a *APMDependenciesInfo) HasGitHubApp() bool { + if a.GitHubApp != nil { + return true + } + for _, e := range a.Entries { + if e.GitHubApp != nil { + return true + } + } + return false } // RateLimitConfig represents rate limiting configuration for workflow triggers From b1e92ed4380a82a6440f1b91cd6abcd95e855d15 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 16 Mar 2026 19:16:38 +0000 Subject: [PATCH 3/6] fix: use unique step names per group to pass duplicate-step validation When multiple APM groups are generated (e.g. two packages with different github-app configs), all steps in the activation job must have unique names or the duplicate-step validator rejects the workflow. Append the group index to each generated step name: - "Generate GitHub App token for APM dependencies (N)" - "Install and pack APM dependencies (N)" [ForGroup path only] - "Upload APM bundle artifact (N)" - "Download APM bundle artifact (N)" The backward-compat single-group pack step (GenerateAPMPackStep) keeps its original name "Install and pack APM dependencies" since it is only ever emitted once. All existing test assertions use assert.Contains with substrings so they continue to pass. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/apm_dependencies.go | 8 +++++--- pkg/workflow/compiler_activation_job.go | 4 ++-- pkg/workflow/compiler_yaml_main_job.go | 2 +- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/pkg/workflow/apm_dependencies.go b/pkg/workflow/apm_dependencies.go index 00d43226269..82243bf0db5 100644 --- a/pkg/workflow/apm_dependencies.go +++ b/pkg/workflow/apm_dependencies.go @@ -103,7 +103,9 @@ func apmAppTokenStepID(groupIndex int) string { // generateAPMAppTokenMintStep generates the step that mints a short-lived GitHub App // installation token scoped for use in the APM pack step. -func (c *Compiler) generateAPMAppTokenMintStep(app *GitHubAppConfig, stepID string) []string { +// groupIndex is included in the step name to avoid duplicate-step validation errors when +// multiple groups each require their own token. +func (c *Compiler) generateAPMAppTokenMintStep(app *GitHubAppConfig, stepID string, groupIndex int) []string { effectiveOwner := app.Owner if effectiveOwner == "" { effectiveOwner = "${{ github.repository_owner }} (default)" @@ -111,7 +113,7 @@ func (c *Compiler) generateAPMAppTokenMintStep(app *GitHubAppConfig, stepID stri apmDepsLog.Printf("Generating APM GitHub App token mint step: id=%s, owner=%s", stepID, effectiveOwner) var steps []string - steps = append(steps, " - name: Generate GitHub App token for APM dependencies\n") + steps = append(steps, fmt.Sprintf(" - name: Generate GitHub App token for APM dependencies (%d)\n", groupIndex)) steps = append(steps, fmt.Sprintf(" id: %s\n", stepID)) steps = append(steps, fmt.Sprintf(" uses: %s\n", GetActionPin("actions/create-github-app-token"))) steps = append(steps, " with:\n") @@ -210,7 +212,7 @@ func GenerateAPMPackStepForGroup(group APMPackageGroup, target string, tokenStep actionRef := GetActionPin("microsoft/apm-action") lines := []string{ - " - name: Install and pack APM dependencies", + fmt.Sprintf(" - name: Install and pack APM dependencies (%d)", group.Index), " id: " + stepID, " uses: " + actionRef, } diff --git a/pkg/workflow/compiler_activation_job.go b/pkg/workflow/compiler_activation_job.go index c2a539d9fbe..e46dd74690a 100644 --- a/pkg/workflow/compiler_activation_job.go +++ b/pkg/workflow/compiler_activation_job.go @@ -421,7 +421,7 @@ func (c *Compiler) buildActivationJob(data *WorkflowData, preActivationJobCreate if group.GitHubApp != nil { tokenStepID = apmAppTokenStepID(group.Index) compilerActivationJobLog.Printf("Adding APM GitHub App token mint step for group %d", group.Index) - tokenSteps := c.generateAPMAppTokenMintStep(group.GitHubApp, tokenStepID) + tokenSteps := c.generateAPMAppTokenMintStep(group.GitHubApp, tokenStepID, group.Index) steps = append(steps, tokenSteps...) } @@ -442,7 +442,7 @@ func (c *Compiler) buildActivationJob(data *WorkflowData, preActivationJobCreate artifactBaseName := apmArtifactBaseName(data.APMDependencies, group.Index) artifactName := artifactPrefixExprForActivationJob(data) + artifactBaseName compilerActivationJobLog.Printf("Adding APM bundle artifact upload step for group %d (artifact=%s)", group.Index, artifactBaseName) - steps = append(steps, " - name: Upload APM bundle artifact\n") + steps = append(steps, fmt.Sprintf(" - name: Upload APM bundle artifact (%d)\n", group.Index)) steps = append(steps, " if: success()\n") steps = append(steps, fmt.Sprintf(" uses: %s\n", GetActionPin("actions/upload-artifact"))) steps = append(steps, " with:\n") diff --git a/pkg/workflow/compiler_yaml_main_job.go b/pkg/workflow/compiler_yaml_main_job.go index 086095ab382..83830f1b366 100644 --- a/pkg/workflow/compiler_yaml_main_job.go +++ b/pkg/workflow/compiler_yaml_main_job.go @@ -250,7 +250,7 @@ func (c *Compiler) generateMainJobSteps(yaml *strings.Builder, data *WorkflowDat for _, group := range apmGroups { artifactBaseName := apmArtifactBaseName(data.APMDependencies, group.Index) artifactName := prefix + artifactBaseName - yaml.WriteString(" - name: Download APM bundle artifact\n") + fmt.Fprintf(yaml, " - name: Download APM bundle artifact (%d)\n", group.Index) fmt.Fprintf(yaml, " uses: %s\n", GetActionPin("actions/download-artifact")) yaml.WriteString(" with:\n") fmt.Fprintf(yaml, " name: %s\n", artifactName) From 8ed63c8fe8689309d073b14c3905c063f899b363 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 16 Mar 2026 19:26:49 +0000 Subject: [PATCH 4/6] Changes before error encountered Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .github/workflows/smoke-claude.lock.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/smoke-claude.lock.yml b/.github/workflows/smoke-claude.lock.yml index e95fce61e66..bad7d7cdc92 100644 --- a/.github/workflows/smoke-claude.lock.yml +++ b/.github/workflows/smoke-claude.lock.yml @@ -649,7 +649,7 @@ jobs: archive: 'true' target: claude working-directory: /tmp/gh-aw/apm-workspace - - name: Upload APM bundle artifact + - name: Upload APM bundle artifact (0) if: success() uses: actions/upload-artifact@bbbca2ddaa5d8feaa63e36b76fdaad77386f024f # v7.0.0 with: @@ -793,7 +793,7 @@ jobs: run: bash /opt/gh-aw/actions/install_awf_binary.sh v0.24.2 - name: Install Claude Code CLI run: npm install -g @anthropic-ai/claude-code@latest - - name: Download APM bundle artifact + - name: Download APM bundle artifact (0) uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 with: name: apm From 8793380f9de954ccbd339b6f0b04976f55e55b0d Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 16 Mar 2026 20:06:27 +0000 Subject: [PATCH 5/6] refactor: remove per-package github-app support for APM dependencies Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .github/workflows/smoke-claude.lock.yml | 4 +- pkg/parser/schemas/main_workflow_schema.json | 70 +----- pkg/workflow/apm_dependencies.go | 131 +++------- .../apm_dependencies_compilation_test.go | 75 ------ pkg/workflow/apm_dependencies_test.go | 229 +----------------- pkg/workflow/compiler_activation_job.go | 73 +++--- pkg/workflow/compiler_yaml_main_job.go | 26 +- .../frontmatter_extraction_metadata.go | 19 +- pkg/workflow/frontmatter_types.go | 28 +-- 9 files changed, 95 insertions(+), 560 deletions(-) diff --git a/.github/workflows/smoke-claude.lock.yml b/.github/workflows/smoke-claude.lock.yml index bad7d7cdc92..e95fce61e66 100644 --- a/.github/workflows/smoke-claude.lock.yml +++ b/.github/workflows/smoke-claude.lock.yml @@ -649,7 +649,7 @@ jobs: archive: 'true' target: claude working-directory: /tmp/gh-aw/apm-workspace - - name: Upload APM bundle artifact (0) + - name: Upload APM bundle artifact if: success() uses: actions/upload-artifact@bbbca2ddaa5d8feaa63e36b76fdaad77386f024f # v7.0.0 with: @@ -793,7 +793,7 @@ jobs: run: bash /opt/gh-aw/actions/install_awf_binary.sh v0.24.2 - name: Install Claude Code CLI run: npm install -g @anthropic-ai/claude-code@latest - - name: Download APM bundle artifact (0) + - name: Download APM bundle artifact uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 with: name: apm diff --git a/pkg/parser/schemas/main_workflow_schema.json b/pkg/parser/schemas/main_workflow_schema.json index 0bedfadb16a..ca5f8a0d6be 100644 --- a/pkg/parser/schemas/main_workflow_schema.json +++ b/pkg/parser/schemas/main_workflow_schema.json @@ -7970,7 +7970,7 @@ ] }, "dependencies": { - "description": "APM package references to install. Supports array format (list of package slugs), object format with packages and optional isolated/github-app fields, and per-package github-app overrides for cross-org access.", + "description": "APM package references to install. Supports array format (list of package slugs) or object format with packages and optional isolated/github-app fields.", "examples": [ ["microsoft/apm-sample-package", "acme/custom-tools"], { @@ -7983,22 +7983,6 @@ "private-key": "${{ secrets.APP_PRIVATE_KEY }}" }, "packages": ["acme-platform-org/acme-skills/plugins/dev-tools"] - }, - { - "github-app": { - "app-id": "${{ vars.APP_ID }}", - "private-key": "${{ secrets.APP_PRIVATE_KEY }}" - }, - "packages": [ - "acme-platform-org/acme-skills/plugins/dev-tools", - { - "source": "partner-org/partner-package", - "github-app": { - "app-id": "${{ vars.PARTNER_APP_ID }}", - "private-key": "${{ secrets.PARTNER_APP_PRIVATE_KEY }}" - } - } - ] } ], "oneOf": [ @@ -8017,55 +8001,11 @@ "properties": { "packages": { "type": "array", - "description": "List of APM package references. Each entry is either a string or an object with source and optional github-app override.", + "description": "List of APM package references.", "items": { - "oneOf": [ - { - "type": "string", - "pattern": "^[a-zA-Z0-9_-]+/[a-zA-Z0-9_./-]+$", - "description": "APM package reference in the format 'org/repo' or 'org/repo/path/to/skill'" - }, - { - "type": "object", - "description": "Object format for per-package github-app override.", - "properties": { - "source": { - "type": "string", - "pattern": "^[a-zA-Z0-9_-]+/[a-zA-Z0-9_./-]+$", - "description": "APM package reference in the format 'org/repo' or 'org/repo/path/to/skill'" - }, - "github-app": { - "type": "object", - "description": "GitHub App configuration for generating a token with cross-org access.", - "properties": { - "app-id": { - "type": "string", - "description": "GitHub App ID or reference (e.g. '${{ vars.APP_ID }}')." - }, - "private-key": { - "type": "string", - "description": "GitHub App private key or reference (e.g. '${{ secrets.APP_PRIVATE_KEY }}')." - }, - "owner": { - "type": "string", - "description": "Optional GitHub App installation owner. Defaults to the repository owner." - }, - "repositories": { - "type": "array", - "items": { - "type": "string" - }, - "description": "Optional list of repositories to grant access to. Use ['*'] for org-wide access." - } - }, - "required": ["app-id", "private-key"], - "additionalProperties": false - } - }, - "required": ["source", "github-app"], - "additionalProperties": false - } - ] + "type": "string", + "pattern": "^[a-zA-Z0-9_-]+/[a-zA-Z0-9_./-]+$", + "description": "APM package reference in the format 'org/repo' or 'org/repo/path/to/skill'" } }, "isolated": { diff --git a/pkg/workflow/apm_dependencies.go b/pkg/workflow/apm_dependencies.go index 82243bf0db5..9b5559fe626 100644 --- a/pkg/workflow/apm_dependencies.go +++ b/pkg/workflow/apm_dependencies.go @@ -9,103 +9,34 @@ import ( var apmDepsLog = logger.New("workflow:apm_dependencies") -// APMPackageGroup represents a group of APM packages that share the same effective GitHub App config. -// Packages with per-package overrides are in their own group; packages without overrides share -// the default github-app group (or the "no-auth" group when no default is configured). -type APMPackageGroup struct { - GitHubApp *GitHubAppConfig // nil = use GITHUB_TOKEN; non-nil = mint a fresh token - Packages []string // Package sources in this group - Index int // Group index for deterministic step/artifact naming -} - -// GetPackageGroups returns the package groups derived from the dependency configuration. -// When no GitHub App is configured (simple case), a single group with no auth is returned. -// When a GitHub App is configured, entries are grouped by their effective app config so that -// packages sharing the same credentials are packed together in one APM step. -func (a *APMDependenciesInfo) GetPackageGroups() []APMPackageGroup { - return groupAPMEntriesByApp(a) -} - -// groupAPMEntriesByApp groups APM package entries by their effective GitHub App config. -// Entries without a per-package override use the default GitHubApp (or no app). -// Entries with identical AppID+PrivateKey combinations are placed in the same group. -func groupAPMEntriesByApp(deps *APMDependenciesInfo) []APMPackageGroup { - if !deps.HasGitHubApp() { - // Simple case: no GitHub App configured — single group, no token minting needed - return []APMPackageGroup{{Packages: deps.Packages, Index: 0}} - } - - type groupData struct { - app *GitHubAppConfig - packages []string - } - - groupMap := make(map[string]*groupData) // key = apmAppGroupKey - var groupOrder []string // preserves insertion order for determinism - - for _, entry := range deps.Entries { - effectiveApp := entry.GitHubApp - if effectiveApp == nil { - effectiveApp = deps.GitHubApp // fall back to the top-level default - } - key := apmAppGroupKey(effectiveApp) - if _, exists := groupMap[key]; !exists { - groupMap[key] = &groupData{app: effectiveApp} - groupOrder = append(groupOrder, key) - } - groupMap[key].packages = append(groupMap[key].packages, entry.Source) - } - - groups := make([]APMPackageGroup, len(groupOrder)) - for i, key := range groupOrder { - groups[i] = APMPackageGroup{ - GitHubApp: groupMap[key].app, - Packages: groupMap[key].packages, - Index: i, - } - } - return groups -} - -// apmAppGroupKey returns a stable string key for a GitHubAppConfig used to group packages. -// A nil config (= use GITHUB_TOKEN) maps to an empty string. -func apmAppGroupKey(app *GitHubAppConfig) string { - if app == nil { - return "" - } - return app.AppID + "|" + app.PrivateKey -} - -// apmArtifactBaseName returns the artifact base name for a given APMDependenciesInfo and group index. +// apmArtifactBaseName returns the artifact base name for a given APMDependenciesInfo. // When GitHub App auth is NOT configured (simple case) the legacy name "apm" is used to // preserve backward compatibility with previously compiled lock files. -// When GitHub App auth IS configured, artifacts are named "apm-N". -func apmArtifactBaseName(deps *APMDependenciesInfo, groupIndex int) string { +// When GitHub App auth IS configured, the artifact is named "apm-0". +func apmArtifactBaseName(deps *APMDependenciesInfo) string { if !deps.HasGitHubApp() { return constants.APMArtifactName // "apm" — backward compat } - return fmt.Sprintf("%s-%d", constants.APMArtifactName, groupIndex) + return constants.APMArtifactName + "-0" } -// apmPackStepID returns the step ID for the APM pack step of a given group. +// apmPackStepID returns the step ID for the APM pack step. // The simple-case (no GitHub App) keeps the legacy "apm_pack" step ID. -func apmPackStepID(deps *APMDependenciesInfo, groupIndex int) string { +func apmPackStepID(deps *APMDependenciesInfo) string { if !deps.HasGitHubApp() { return "apm_pack" // legacy } - return fmt.Sprintf("apm_pack_%d", groupIndex) + return "apm_pack_0" } -// apmAppTokenStepID returns the step ID for the GitHub App token mint step for a given group. -func apmAppTokenStepID(groupIndex int) string { - return fmt.Sprintf("apm-app-token-%d", groupIndex) +// apmAppTokenStepID returns the step ID for the GitHub App token mint step. +func apmAppTokenStepID() string { + return "apm-app-token-0" } // generateAPMAppTokenMintStep generates the step that mints a short-lived GitHub App // installation token scoped for use in the APM pack step. -// groupIndex is included in the step name to avoid duplicate-step validation errors when -// multiple groups each require their own token. -func (c *Compiler) generateAPMAppTokenMintStep(app *GitHubAppConfig, stepID string, groupIndex int) []string { +func (c *Compiler) generateAPMAppTokenMintStep(app *GitHubAppConfig, stepID string) []string { effectiveOwner := app.Owner if effectiveOwner == "" { effectiveOwner = "${{ github.repository_owner }} (default)" @@ -113,7 +44,7 @@ func (c *Compiler) generateAPMAppTokenMintStep(app *GitHubAppConfig, stepID stri apmDepsLog.Printf("Generating APM GitHub App token mint step: id=%s, owner=%s", stepID, effectiveOwner) var steps []string - steps = append(steps, fmt.Sprintf(" - name: Generate GitHub App token for APM dependencies (%d)\n", groupIndex)) + steps = append(steps, " - name: Generate GitHub App token for APM dependencies\n") steps = append(steps, fmt.Sprintf(" id: %s\n", stepID)) steps = append(steps, fmt.Sprintf(" uses: %s\n", GetActionPin("actions/create-github-app-token"))) steps = append(steps, " with:\n") @@ -147,8 +78,8 @@ func (c *Compiler) generateAPMAppTokenMintStep(app *GitHubAppConfig, stepID stri // packs them into a bundle in the activation job. The step always uses isolated:true because // the activation job has no repo context to preserve. // -// This is the simple (backward-compatible) form: no GitHub App auth, single group. -// For the multi-group case use GenerateAPMPackStepForGroup. +// This is the simple (backward-compatible) form: no GitHub App auth. +// For the GitHub App case use generateAPMPackStepWithToken. // // Parameters: // - apmDeps: APM dependency configuration extracted from frontmatter @@ -164,7 +95,7 @@ func GenerateAPMPackStep(apmDeps *APMDependenciesInfo, target string, data *Work apmDepsLog.Printf("Generating APM pack step: %d packages, target=%s", len(apmDeps.Packages), target) - stepID := apmPackStepID(apmDeps, 0) + stepID := apmPackStepID(apmDeps) actionRef := GetActionPin("microsoft/apm-action") lines := []string{ @@ -190,30 +121,24 @@ func GenerateAPMPackStep(apmDeps *APMDependenciesInfo, target string, data *Work return GitHubActionStep(lines) } -// GenerateAPMPackStepForGroup generates the APM pack step for a specific package group. -// Used when GitHub App auth is configured (Option B), where each group may use a different token. -// -// Parameters: -// - group: APMPackageGroup containing the packages and optional GitHub App config -// - target: APM target (e.g. "copilot", "claude", "all") -// - tokenStepID: Step ID of the preceding token-mint step (empty = use default GITHUB_TOKEN) -// - data: WorkflowData used for action pin resolution -func GenerateAPMPackStepForGroup(group APMPackageGroup, target string, tokenStepID string, data *WorkflowData) GitHubActionStep { - if len(group.Packages) == 0 { - apmDepsLog.Print("No APM packages in group to pack") +// generateAPMPackStepWithToken generates the APM pack step for the GitHub App auth case. +// It sets the GITHUB_TOKEN env to the token minted by the preceding token-mint step. +// Uses step ID "apm_pack_0" (distinguished from the no-auth "apm_pack") so artifact +// references remain consistent when workflows have a github-app configured. +func generateAPMPackStepWithToken(apmDeps *APMDependenciesInfo, target string, tokenStepID string, data *WorkflowData) GitHubActionStep { + if len(apmDeps.Packages) == 0 { + apmDepsLog.Print("No APM packages to pack") return GitHubActionStep{} } - apmDepsLog.Printf("Generating APM pack step for group %d: %d packages, target=%s, tokenStep=%s", - group.Index, len(group.Packages), target, tokenStepID) + apmDepsLog.Printf("Generating APM pack step with token: %d packages, target=%s, tokenStep=%s", + len(apmDeps.Packages), target, tokenStepID) - stepID := fmt.Sprintf("apm_pack_%d", group.Index) - workDir := fmt.Sprintf("/tmp/gh-aw/apm-workspace-%d", group.Index) actionRef := GetActionPin("microsoft/apm-action") lines := []string{ - fmt.Sprintf(" - name: Install and pack APM dependencies (%d)", group.Index), - " id: " + stepID, + " - name: Install and pack APM dependencies", + " id: " + apmPackStepID(apmDeps), " uses: " + actionRef, } @@ -229,7 +154,7 @@ func GenerateAPMPackStepForGroup(group APMPackageGroup, target string, tokenStep " dependencies: |", ) - for _, dep := range group.Packages { + for _, dep := range apmDeps.Packages { lines = append(lines, " - "+dep) } @@ -238,7 +163,7 @@ func GenerateAPMPackStepForGroup(group APMPackageGroup, target string, tokenStep " pack: 'true'", " archive: 'true'", " target: "+target, - " working-directory: "+workDir, + " working-directory: /tmp/gh-aw/apm-workspace", ) return GitHubActionStep(lines) diff --git a/pkg/workflow/apm_dependencies_compilation_test.go b/pkg/workflow/apm_dependencies_compilation_test.go index 48a5b9717e7..b89e43d6881 100644 --- a/pkg/workflow/apm_dependencies_compilation_test.go +++ b/pkg/workflow/apm_dependencies_compilation_test.go @@ -302,78 +302,3 @@ Test with default github-app for cross-org APM access assert.Contains(t, lockContent, "bundle: /tmp/gh-aw/apm-bundle/*.tar.gz", "Lock file should restore from bundle path") } - -func TestAPMDependenciesCompilationPerPackageGitHubApp(t *testing.T) { - tmpDir := testutil.TempDir(t, "apm-deps-per-pkg-app-test") - - workflow := `--- -engine: claude -on: workflow_dispatch -permissions: - issues: read - pull-requests: read -dependencies: - github-app: - app-id: ${{ vars.APP_ID }} - private-key: ${{ secrets.APP_PRIVATE_KEY }} - packages: - - acme-platform-org/acme-skills/plugins/dev-tools - - source: partner-org/partner-package - github-app: - app-id: ${{ vars.PARTNER_APP_ID }} - private-key: ${{ secrets.PARTNER_APP_PRIVATE_KEY }} ---- - -Test with per-package github-app overrides for multi-org APM access -` - - testFile := filepath.Join(tmpDir, "test-apm-per-pkg-app.md") - err := os.WriteFile(testFile, []byte(workflow), 0644) - require.NoError(t, err, "Failed to write test file") - - compiler := NewCompiler() - err = compiler.CompileWorkflow(testFile) - require.NoError(t, err, "Compilation should succeed") - - lockFile := strings.Replace(testFile, ".md", ".lock.yml", 1) - content, err := os.ReadFile(lockFile) - require.NoError(t, err, "Failed to read lock file") - - lockContent := string(content) - - // Group 0: default app for acme-platform-org package - assert.Contains(t, lockContent, "id: apm-app-token-0", - "Lock file should have token mint step for group 0") - assert.Contains(t, lockContent, "${{ vars.APP_ID }}", - "Lock file should reference default app ID") - assert.Contains(t, lockContent, "id: apm_pack_0", - "Lock file should have pack step for group 0") - assert.Contains(t, lockContent, "GITHUB_TOKEN: ${{ steps.apm-app-token-0.outputs.token }}", - "Pack step 0 should use token from group 0 mint") - assert.Contains(t, lockContent, "- acme-platform-org/acme-skills/plugins/dev-tools", - "Pack step 0 should list the acme package") - assert.Contains(t, lockContent, "name: apm-0", - "Artifact upload for group 0 should use indexed name") - - // Group 1: per-package app for partner-org package - assert.Contains(t, lockContent, "id: apm-app-token-1", - "Lock file should have token mint step for group 1") - assert.Contains(t, lockContent, "${{ vars.PARTNER_APP_ID }}", - "Lock file should reference partner app ID") - assert.Contains(t, lockContent, "id: apm_pack_1", - "Lock file should have pack step for group 1") - assert.Contains(t, lockContent, "GITHUB_TOKEN: ${{ steps.apm-app-token-1.outputs.token }}", - "Pack step 1 should use token from group 1 mint") - assert.Contains(t, lockContent, "- partner-org/partner-package", - "Pack step 1 should list the partner package") - assert.Contains(t, lockContent, "name: apm-1", - "Artifact upload for group 1 should use indexed name") - - // Agent job: two download steps (one per group) - downloadCount := strings.Count(lockContent, "Download APM bundle artifact") - assert.Equal(t, 2, downloadCount, "Agent job should have 2 APM bundle download steps") - - // Agent job: single restore step - assert.Contains(t, lockContent, "Restore APM dependencies", - "Lock file should contain APM restore step") -} diff --git a/pkg/workflow/apm_dependencies_test.go b/pkg/workflow/apm_dependencies_test.go index ace7b0f6516..4ce9014a251 100644 --- a/pkg/workflow/apm_dependencies_test.go +++ b/pkg/workflow/apm_dependencies_test.go @@ -17,7 +17,6 @@ func TestExtractAPMDependenciesFromFrontmatter(t *testing.T) { expectedDeps []string expectedIsolated bool hasDefaultApp bool - perPackageApps int // number of entries that have their own github-app }{ { name: "No dependencies field", @@ -113,7 +112,7 @@ func TestExtractAPMDependenciesFromFrontmatter(t *testing.T) { }, expectedDeps: nil, }, - // New: github-app support + // github-app support { name: "Object format with default github-app", frontmatter: map[string]any{ @@ -148,64 +147,18 @@ func TestExtractAPMDependenciesFromFrontmatter(t *testing.T) { expectedDeps: []string{"acme-platform-org/acme-skills/plugins/dev-tools", "acme-platform-org/another-package"}, hasDefaultApp: true, }, - { - name: "Object format with per-package github-app override", - frontmatter: map[string]any{ - "dependencies": map[string]any{ - "github-app": map[string]any{ - "app-id": "${{ vars.APP_ID }}", - "private-key": "${{ secrets.APP_PRIVATE_KEY }}", - }, - "packages": []any{ - "acme-platform-org/acme-skills/plugins/dev-tools", - map[string]any{ - "source": "partner-org/partner-package", - "github-app": map[string]any{ - "app-id": "${{ vars.PARTNER_APP_ID }}", - "private-key": "${{ secrets.PARTNER_APP_PRIVATE_KEY }}", - }, - }, - }, - }, - }, - expectedDeps: []string{"acme-platform-org/acme-skills/plugins/dev-tools", "partner-org/partner-package"}, - hasDefaultApp: true, - perPackageApps: 1, - }, - { - name: "Object format with only per-package github-app (no default)", - frontmatter: map[string]any{ - "dependencies": map[string]any{ - "packages": []any{ - map[string]any{ - "source": "partner-org/partner-package", - "github-app": map[string]any{ - "app-id": "${{ vars.PARTNER_APP_ID }}", - "private-key": "${{ secrets.PARTNER_APP_PRIVATE_KEY }}", - }, - }, - }, - }, - }, - expectedDeps: []string{"partner-org/partner-package"}, - hasDefaultApp: false, - perPackageApps: 1, - }, { name: "Object entry without source is skipped", frontmatter: map[string]any{ "dependencies": map[string]any{ "packages": []any{ map[string]any{ - "github-app": map[string]any{ - "app-id": "${{ vars.APP_ID }}", - "private-key": "${{ secrets.APP_PRIVATE_KEY }}", - }, + "foo": "bar", // no source field }, }, }, }, - expectedDeps: nil, // empty source → skipped + expectedDeps: nil, // no valid source → skipped }, } @@ -223,14 +176,6 @@ func TestExtractAPMDependenciesFromFrontmatter(t *testing.T) { } else { assert.Nil(t, result.GitHubApp, "Default github-app should be absent") } - // Count entries with a per-package GitHubApp - perPkgCount := 0 - for _, e := range result.Entries { - if e.GitHubApp != nil { - perPkgCount++ - } - } - assert.Equal(t, tt.perPackageApps, perPkgCount, "Per-package github-app count should match") } }) } @@ -244,28 +189,17 @@ func TestAPMDependenciesHasGitHubApp(t *testing.T) { }{ { name: "No github-app configured", - deps: &APMDependenciesInfo{Packages: []string{"pkg1"}, Entries: []APMPackageEntry{{Source: "pkg1"}}}, + deps: &APMDependenciesInfo{Packages: []string{"pkg1"}}, expected: false, }, { name: "Default github-app configured", deps: &APMDependenciesInfo{ Packages: []string{"pkg1"}, - Entries: []APMPackageEntry{{Source: "pkg1"}}, GitHubApp: &GitHubAppConfig{AppID: "123", PrivateKey: "key"}, }, expected: true, }, - { - name: "Per-package github-app configured", - deps: &APMDependenciesInfo{ - Packages: []string{"pkg1"}, - Entries: []APMPackageEntry{ - {Source: "pkg1", GitHubApp: &GitHubAppConfig{AppID: "123", PrivateKey: "key"}}, - }, - }, - expected: true, - }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -274,85 +208,6 @@ func TestAPMDependenciesHasGitHubApp(t *testing.T) { } } -func TestGroupAPMEntriesByApp(t *testing.T) { - tests := []struct { - name string - deps *APMDependenciesInfo - expectedGroups int - groupSizes []int // expected number of packages per group (in order) - }{ - { - name: "Simple case - no github-app gives single group", - deps: &APMDependenciesInfo{ - Packages: []string{"pkg1", "pkg2"}, - Entries: []APMPackageEntry{{Source: "pkg1"}, {Source: "pkg2"}}, - }, - expectedGroups: 1, - groupSizes: []int{2}, - }, - { - name: "Default github-app only - single group", - deps: &APMDependenciesInfo{ - Packages: []string{"pkg1", "pkg2"}, - Entries: []APMPackageEntry{{Source: "pkg1"}, {Source: "pkg2"}}, - GitHubApp: &GitHubAppConfig{AppID: "123", PrivateKey: "key"}, - }, - expectedGroups: 1, - groupSizes: []int{2}, - }, - { - name: "Default github-app + per-package override with different app - two groups", - deps: &APMDependenciesInfo{ - Packages: []string{"pkg1", "pkg2"}, - Entries: []APMPackageEntry{ - {Source: "pkg1"}, // uses default app - {Source: "pkg2", GitHubApp: &GitHubAppConfig{AppID: "456", PrivateKey: "key2"}}, - }, - GitHubApp: &GitHubAppConfig{AppID: "123", PrivateKey: "key1"}, - }, - expectedGroups: 2, - groupSizes: []int{1, 1}, - }, - { - name: "Two packages with same per-package github-app - merged into one group", - deps: &APMDependenciesInfo{ - Packages: []string{"pkg1", "pkg2"}, - Entries: []APMPackageEntry{ - {Source: "pkg1", GitHubApp: &GitHubAppConfig{AppID: "123", PrivateKey: "key"}}, - {Source: "pkg2", GitHubApp: &GitHubAppConfig{AppID: "123", PrivateKey: "key"}}, - }, - }, - expectedGroups: 1, - groupSizes: []int{2}, - }, - { - name: "Three packages across three different apps", - deps: &APMDependenciesInfo{ - Packages: []string{"pkg1", "pkg2", "pkg3"}, - Entries: []APMPackageEntry{ - {Source: "pkg1"}, // uses default app - {Source: "pkg2", GitHubApp: &GitHubAppConfig{AppID: "456", PrivateKey: "key2"}}, - {Source: "pkg3", GitHubApp: &GitHubAppConfig{AppID: "789", PrivateKey: "key3"}}, - }, - GitHubApp: &GitHubAppConfig{AppID: "123", PrivateKey: "key1"}, - }, - expectedGroups: 3, - groupSizes: []int{1, 1, 1}, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - groups := groupAPMEntriesByApp(tt.deps) - require.Len(t, groups, tt.expectedGroups, "Number of groups should match") - for i, g := range groups { - assert.Equal(t, i, g.Index, "Group index should match position") - assert.Len(t, g.Packages, tt.groupSizes[i], "Group %d package count should match", i) - } - }) - } -} - func TestEngineGetAPMTarget(t *testing.T) { tests := []struct { name string @@ -395,7 +250,7 @@ func TestGenerateAPMPackStep(t *testing.T) { }, { name: "Single dependency with copilot target", - apmDeps: &APMDependenciesInfo{Packages: []string{"microsoft/apm-sample-package"}, Entries: []APMPackageEntry{{Source: "microsoft/apm-sample-package"}}}, + apmDeps: &APMDependenciesInfo{Packages: []string{"microsoft/apm-sample-package"}}, target: "copilot", expectedContains: []string{ "Install and pack APM dependencies", @@ -412,7 +267,7 @@ func TestGenerateAPMPackStep(t *testing.T) { }, { name: "Multiple dependencies with claude target", - apmDeps: &APMDependenciesInfo{Packages: []string{"microsoft/apm-sample-package", "github/skills/review"}, Entries: []APMPackageEntry{{Source: "microsoft/apm-sample-package"}, {Source: "github/skills/review"}}}, + apmDeps: &APMDependenciesInfo{Packages: []string{"microsoft/apm-sample-package", "github/skills/review"}}, target: "claude", expectedContains: []string{ "Install and pack APM dependencies", @@ -425,7 +280,7 @@ func TestGenerateAPMPackStep(t *testing.T) { }, { name: "All target for non-copilot/claude engine", - apmDeps: &APMDependenciesInfo{Packages: []string{"microsoft/apm-sample-package"}, Entries: []APMPackageEntry{{Source: "microsoft/apm-sample-package"}}}, + apmDeps: &APMDependenciesInfo{Packages: []string{"microsoft/apm-sample-package"}}, target: "all", expectedContains: []string{ "target: all", @@ -458,76 +313,6 @@ func TestGenerateAPMPackStep(t *testing.T) { } } -func TestGenerateAPMPackStepForGroup(t *testing.T) { - tests := []struct { - name string - group APMPackageGroup - target string - tokenStepID string - expectedContains []string - expectedEmpty bool - }{ - { - name: "Empty group returns empty step", - group: APMPackageGroup{Packages: []string{}, Index: 0}, - target: "copilot", - expectedEmpty: true, - }, - { - name: "Single package group without token", - group: APMPackageGroup{Packages: []string{"microsoft/apm-sample-package"}, Index: 0}, - target: "copilot", - tokenStepID: "", - expectedContains: []string{ - "Install and pack APM dependencies", - "id: apm_pack_0", - "microsoft/apm-action", - "- microsoft/apm-sample-package", - "target: copilot", - "working-directory: /tmp/gh-aw/apm-workspace-0", - }, - }, - { - name: "Group with GitHub App token", - group: APMPackageGroup{Packages: []string{"acme-org/acme-skills"}, Index: 1}, - target: "claude", - tokenStepID: "apm-app-token-1", - expectedContains: []string{ - "id: apm_pack_1", - "env:", - "GITHUB_TOKEN: ${{ steps.apm-app-token-1.outputs.token }}", - "- acme-org/acme-skills", - "target: claude", - "working-directory: /tmp/gh-aw/apm-workspace-1", - }, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - data := &WorkflowData{Name: "test-workflow"} - step := GenerateAPMPackStepForGroup(tt.group, tt.target, tt.tokenStepID, data) - - if tt.expectedEmpty { - assert.Empty(t, step, "Step should be empty for empty group") - return - } - - require.NotEmpty(t, step, "Step should not be empty") - - var sb strings.Builder - for _, line := range step { - sb.WriteString(line + "\n") - } - combined := sb.String() - - for _, expected := range tt.expectedContains { - assert.Contains(t, combined, expected, "Step should contain: %s", expected) - } - }) - } -} - func TestGenerateAPMRestoreStep(t *testing.T) { tests := []struct { name string diff --git a/pkg/workflow/compiler_activation_job.go b/pkg/workflow/compiler_activation_job.go index e46dd74690a..5dfbf63a33f 100644 --- a/pkg/workflow/compiler_activation_job.go +++ b/pkg/workflow/compiler_activation_job.go @@ -408,48 +408,45 @@ func (c *Compiler) buildActivationJob(data *WorkflowData, preActivationJobCreate // Generate APM pack step(s) if dependencies are specified. // When no GitHub App is configured: single pack step (backward compat). - // When a GitHub App is configured: one token-mint + pack + upload step per package group. + // When a GitHub App is configured: one token-mint + pack + upload step. if data.APMDependencies != nil && len(data.APMDependencies.Packages) > 0 { apmTarget := engine.GetAPMTarget() - apmGroups := data.APMDependencies.GetPackageGroups() - compilerActivationJobLog.Printf("Adding %d APM pack group(s): %d packages total", - len(apmGroups), len(data.APMDependencies.Packages)) - - for _, group := range apmGroups { - // Mint a GitHub App token before the pack step when the group requires one. - tokenStepID := "" - if group.GitHubApp != nil { - tokenStepID = apmAppTokenStepID(group.Index) - compilerActivationJobLog.Printf("Adding APM GitHub App token mint step for group %d", group.Index) - tokenSteps := c.generateAPMAppTokenMintStep(group.GitHubApp, tokenStepID, group.Index) - steps = append(steps, tokenSteps...) - } - - // Generate the pack step for this group. - var packStep GitHubActionStep - if data.APMDependencies.HasGitHubApp() { - packStep = GenerateAPMPackStepForGroup(group, apmTarget, tokenStepID, data) - } else { - packStep = GenerateAPMPackStep(data.APMDependencies, apmTarget, data) - } - for _, line := range packStep { - steps = append(steps, line+"\n") - } + compilerActivationJobLog.Printf("Adding APM pack step: %d packages total", + len(data.APMDependencies.Packages)) + + // Mint a GitHub App token before the pack step when configured. + tokenStepID := "" + if data.APMDependencies.GitHubApp != nil { + tokenStepID = apmAppTokenStepID() + compilerActivationJobLog.Print("Adding APM GitHub App token mint step") + tokenSteps := c.generateAPMAppTokenMintStep(data.APMDependencies.GitHubApp, tokenStepID) + steps = append(steps, tokenSteps...) + } - // Upload the packed APM bundle as a separate artifact for the agent job to download. - // The path comes from the apm_pack / apm_pack_N step output `bundle-path`. - packID := apmPackStepID(data.APMDependencies, group.Index) - artifactBaseName := apmArtifactBaseName(data.APMDependencies, group.Index) - artifactName := artifactPrefixExprForActivationJob(data) + artifactBaseName - compilerActivationJobLog.Printf("Adding APM bundle artifact upload step for group %d (artifact=%s)", group.Index, artifactBaseName) - steps = append(steps, fmt.Sprintf(" - name: Upload APM bundle artifact (%d)\n", group.Index)) - steps = append(steps, " if: success()\n") - steps = append(steps, fmt.Sprintf(" uses: %s\n", GetActionPin("actions/upload-artifact"))) - steps = append(steps, " with:\n") - steps = append(steps, fmt.Sprintf(" name: %s\n", artifactName)) - steps = append(steps, fmt.Sprintf(" path: ${{ steps.%s.outputs.bundle-path }}\n", packID)) - steps = append(steps, " retention-days: 1\n") + // Generate the pack step. + var packStep GitHubActionStep + if data.APMDependencies.GitHubApp != nil { + packStep = generateAPMPackStepWithToken(data.APMDependencies, apmTarget, tokenStepID, data) + } else { + packStep = GenerateAPMPackStep(data.APMDependencies, apmTarget, data) + } + for _, line := range packStep { + steps = append(steps, line+"\n") } + + // Upload the packed APM bundle as a separate artifact for the agent job to download. + // The path comes from the apm_pack / apm_pack_0 step output `bundle-path`. + packID := apmPackStepID(data.APMDependencies) + artifactBaseName := apmArtifactBaseName(data.APMDependencies) + artifactName := artifactPrefixExprForActivationJob(data) + artifactBaseName + compilerActivationJobLog.Printf("Adding APM bundle artifact upload step (artifact=%s)", artifactBaseName) + steps = append(steps, " - name: Upload APM bundle artifact\n") + steps = append(steps, " if: success()\n") + steps = append(steps, fmt.Sprintf(" uses: %s\n", GetActionPin("actions/upload-artifact"))) + steps = append(steps, " with:\n") + steps = append(steps, fmt.Sprintf(" name: %s\n", artifactName)) + steps = append(steps, fmt.Sprintf(" path: ${{ steps.%s.outputs.bundle-path }}\n", packID)) + steps = append(steps, " retention-days: 1\n") } // Upload aw_info.json and prompt.txt as the activation artifact for the agent job to download. diff --git a/pkg/workflow/compiler_yaml_main_job.go b/pkg/workflow/compiler_yaml_main_job.go index 83830f1b366..8325bcf44cf 100644 --- a/pkg/workflow/compiler_yaml_main_job.go +++ b/pkg/workflow/compiler_yaml_main_job.go @@ -238,26 +238,20 @@ func (c *Compiler) generateMainJobSteps(yaml *strings.Builder, data *WorkflowDat } // Add APM (Agent Package Manager) setup step if dependencies are specified. - // When no GitHub App is configured: single download step (backward compat). - // When a GitHub App is configured: one download step per artifact group, all going to - // /tmp/gh-aw/apm-bundle so the single restore step can glob *.tar.gz across all groups. if data.APMDependencies != nil && len(data.APMDependencies.Packages) > 0 { - apmGroups := data.APMDependencies.GetPackageGroups() - compilerYamlLog.Printf("Adding %d APM bundle download step(s): %d packages total", - len(apmGroups), len(data.APMDependencies.Packages)) + compilerYamlLog.Printf("Adding APM bundle download step: %d packages total", + len(data.APMDependencies.Packages)) prefix := artifactPrefixExprForDownstreamJob(data) - for _, group := range apmGroups { - artifactBaseName := apmArtifactBaseName(data.APMDependencies, group.Index) - artifactName := prefix + artifactBaseName - fmt.Fprintf(yaml, " - name: Download APM bundle artifact (%d)\n", group.Index) - fmt.Fprintf(yaml, " uses: %s\n", GetActionPin("actions/download-artifact")) - yaml.WriteString(" with:\n") - fmt.Fprintf(yaml, " name: %s\n", artifactName) - yaml.WriteString(" path: /tmp/gh-aw/apm-bundle\n") - } + artifactBaseName := apmArtifactBaseName(data.APMDependencies) + artifactName := prefix + artifactBaseName + fmt.Fprintf(yaml, " - name: Download APM bundle artifact\n") + fmt.Fprintf(yaml, " uses: %s\n", GetActionPin("actions/download-artifact")) + yaml.WriteString(" with:\n") + fmt.Fprintf(yaml, " name: %s\n", artifactName) + yaml.WriteString(" path: /tmp/gh-aw/apm-bundle\n") - // Restore APM dependencies from all bundles in the shared directory + // Restore APM dependencies from the bundle compilerYamlLog.Printf("Adding APM restore step") apmStep := GenerateAPMRestoreStep(data.APMDependencies, data) for _, line := range apmStep { diff --git a/pkg/workflow/frontmatter_extraction_metadata.go b/pkg/workflow/frontmatter_extraction_metadata.go index 0662882719a..65029fc687c 100644 --- a/pkg/workflow/frontmatter_extraction_metadata.go +++ b/pkg/workflow/frontmatter_extraction_metadata.go @@ -357,8 +357,6 @@ func extractPluginsFromFrontmatter(frontmatter map[string]any) *PluginInfo { // - Array format: ["org/pkg1", "org/pkg2"] // - Object format with packages and isolated: {packages: [...], isolated: true} // - Object format with a default github-app: {github-app: {...}, packages: [...]} -// - Object format with per-package github-app overrides: -// {github-app: {...}, packages: ["org/pkg1", {source: "org/pkg2", github-app: {...}}]} // // Returns nil if no dependencies field is present or if the field contains no packages. func extractAPMDependenciesFromFrontmatter(frontmatter map[string]any) *APMDependenciesInfo { @@ -368,7 +366,6 @@ func extractAPMDependenciesFromFrontmatter(frontmatter map[string]any) *APMDepen } var packages []string - var entries []APMPackageEntry var isolated bool var defaultApp *GitHubAppConfig @@ -378,7 +375,6 @@ func extractAPMDependenciesFromFrontmatter(frontmatter map[string]any) *APMDepen for _, item := range v { if s, ok := item.(string); ok && s != "" { packages = append(packages, s) - entries = append(entries, APMPackageEntry{Source: s}) } } case map[string]any: @@ -398,19 +394,18 @@ func extractAPMDependenciesFromFrontmatter(frontmatter map[string]any) *APMDepen } } - // Parse packages: each item is either a string or an object {source: "...", github-app: {...}} + // Parse packages: each item is a string; object entries are also accepted if they have a 'source' field if pkgsAny, ok := v["packages"]; ok { if pkgsArray, ok := pkgsAny.([]any); ok { for _, item := range pkgsArray { switch pkg := item.(type) { case string: - // Simple string entry if pkg != "" { packages = append(packages, pkg) - entries = append(entries, APMPackageEntry{Source: pkg}) } case map[string]any: - // Object entry: {source: "org/repo", github-app: {...}} + // Object entry: only the 'source' field is used. + // Any other fields (e.g. 'github-app') are silently ignored. sourceAny, hasSource := pkg["source"] if !hasSource { frontmatterMetadataLog.Print("Skipping APM package entry: missing 'source' field") @@ -421,14 +416,7 @@ func extractAPMDependenciesFromFrontmatter(frontmatter map[string]any) *APMDepen frontmatterMetadataLog.Print("Skipping APM package entry: 'source' field must be a non-empty string") continue } - entry := APMPackageEntry{Source: source} - if appAny, ok := pkg["github-app"]; ok { - if appMap, ok := appAny.(map[string]any); ok { - entry.GitHubApp = parseAppConfig(appMap) - } - } packages = append(packages, source) - entries = append(entries, entry) } } } @@ -445,7 +433,6 @@ func extractAPMDependenciesFromFrontmatter(frontmatter map[string]any) *APMDepen len(packages), isolated, defaultApp != nil) return &APMDependenciesInfo{ Packages: packages, - Entries: entries, Isolated: isolated, GitHubApp: defaultApp, } diff --git a/pkg/workflow/frontmatter_types.go b/pkg/workflow/frontmatter_types.go index 33a2b08eae6..771a53c6ba9 100644 --- a/pkg/workflow/frontmatter_types.go +++ b/pkg/workflow/frontmatter_types.go @@ -86,40 +86,22 @@ type PluginsConfig struct { GitHubToken string `json:"github-token,omitempty"` // Custom GitHub token for plugin installation } -// APMPackageEntry represents a single APM package entry. -// Supports both simple string format (just the package source) and object format with an -// optional per-package GitHub App override (source: + github-app:). -type APMPackageEntry struct { - Source string // Package source (e.g., "org/repo" or "org/repo/path/to/skill") - GitHubApp *GitHubAppConfig // Optional per-package GitHub App override; nil means use the default -} - // APMDependenciesInfo encapsulates APM (Agent Package Manager) dependency configuration. // Supports: // - Simple array format: dependencies: [pkg1, pkg2] // - Object format with packages and isolated: dependencies: {packages: [...], isolated: true} // - Object format with a default github-app for cross-org access -// - Per-package github-app overrides for multi-org scenarios // // When present, a pack step is emitted in the activation job and a restore step in the agent job. type APMDependenciesInfo struct { - Packages []string // Flat list of all APM package sources (for backward compat) - Entries []APMPackageEntry // Package entries with optional per-package GitHub App overrides - Isolated bool // If true, agent restore step clears primitive dirs before unpacking - GitHubApp *GitHubAppConfig // Default GitHub App for all packages that don't override it + Packages []string // List of APM package sources + Isolated bool // If true, agent restore step clears primitive dirs before unpacking + GitHubApp *GitHubAppConfig // Optional GitHub App for cross-org access (applied to all packages) } -// HasGitHubApp returns true if any GitHub App is configured (default or per-package override). +// HasGitHubApp returns true if a GitHub App is configured. func (a *APMDependenciesInfo) HasGitHubApp() bool { - if a.GitHubApp != nil { - return true - } - for _, e := range a.Entries { - if e.GitHubApp != nil { - return true - } - } - return false + return a.GitHubApp != nil } // RateLimitConfig represents rate limiting configuration for workflow triggers From e4e088e95b75056d85c6e1f801c6dc2a564f4910 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 16 Mar 2026 20:45:09 +0000 Subject: [PATCH 6/6] refactor: simplify APM code - merge duplicate functions, remove unused params Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/apm_dependencies.go | 151 ++++++------------ pkg/workflow/apm_dependencies_test.go | 36 +++-- pkg/workflow/compiler_activation_job.go | 28 ++-- pkg/workflow/compiler_yaml_main_job.go | 10 +- .../frontmatter_extraction_metadata.go | 23 +-- 5 files changed, 83 insertions(+), 165 deletions(-) diff --git a/pkg/workflow/apm_dependencies.go b/pkg/workflow/apm_dependencies.go index 9b5559fe626..0a305fd130e 100644 --- a/pkg/workflow/apm_dependencies.go +++ b/pkg/workflow/apm_dependencies.go @@ -9,64 +9,50 @@ import ( var apmDepsLog = logger.New("workflow:apm_dependencies") -// apmArtifactBaseName returns the artifact base name for a given APMDependenciesInfo. -// When GitHub App auth is NOT configured (simple case) the legacy name "apm" is used to -// preserve backward compatibility with previously compiled lock files. -// When GitHub App auth IS configured, the artifact is named "apm-0". -func apmArtifactBaseName(deps *APMDependenciesInfo) string { - if !deps.HasGitHubApp() { - return constants.APMArtifactName // "apm" — backward compat - } - return constants.APMArtifactName + "-0" -} +const ( + // apmAppTokenStepID is the step ID for the GitHub App token mint step. + apmAppTokenStepID = "apm-app-token-0" +) // apmPackStepID returns the step ID for the APM pack step. -// The simple-case (no GitHub App) keeps the legacy "apm_pack" step ID. +// The no-app case keeps the legacy "apm_pack" step ID for backward compatibility. func apmPackStepID(deps *APMDependenciesInfo) string { - if !deps.HasGitHubApp() { - return "apm_pack" // legacy + if deps.HasGitHubApp() { + return "apm_pack_0" } - return "apm_pack_0" -} - -// apmAppTokenStepID returns the step ID for the GitHub App token mint step. -func apmAppTokenStepID() string { - return "apm-app-token-0" + return "apm_pack" } // generateAPMAppTokenMintStep generates the step that mints a short-lived GitHub App // installation token scoped for use in the APM pack step. -func (c *Compiler) generateAPMAppTokenMintStep(app *GitHubAppConfig, stepID string) []string { - effectiveOwner := app.Owner - if effectiveOwner == "" { - effectiveOwner = "${{ github.repository_owner }} (default)" - } - apmDepsLog.Printf("Generating APM GitHub App token mint step: id=%s, owner=%s", stepID, effectiveOwner) - var steps []string - - steps = append(steps, " - name: Generate GitHub App token for APM dependencies\n") - steps = append(steps, fmt.Sprintf(" id: %s\n", stepID)) - 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(" private-key: %s\n", app.PrivateKey)) - +func (c *Compiler) generateAPMAppTokenMintStep(app *GitHubAppConfig) []string { owner := app.Owner if owner == "" { owner = "${{ github.repository_owner }}" } - steps = append(steps, fmt.Sprintf(" owner: %s\n", owner)) + apmDepsLog.Printf("Generating APM GitHub App token mint step: owner=%s", owner) + + steps := []string{ + " - name: Generate GitHub App token for APM dependencies\n", + " id: " + apmAppTokenStepID + "\n", + fmt.Sprintf(" uses: %s\n", GetActionPin("actions/create-github-app-token")), + " with:\n", + fmt.Sprintf(" app-id: %s\n", app.AppID), + fmt.Sprintf(" private-key: %s\n", app.PrivateKey), + fmt.Sprintf(" owner: %s\n", owner), + } - if len(app.Repositories) == 1 && app.Repositories[0] == "*" { + switch { + case len(app.Repositories) == 1 && app.Repositories[0] == "*": // Org-wide access: omit repositories field - } else if len(app.Repositories) == 1 { + case len(app.Repositories) == 1: steps = append(steps, fmt.Sprintf(" repositories: %s\n", app.Repositories[0])) - } else if len(app.Repositories) > 1 { + case len(app.Repositories) > 1: steps = append(steps, " repositories: |-\n") for _, repo := range app.Repositories { steps = append(steps, fmt.Sprintf(" %s\n", repo)) } - } else { + default: steps = append(steps, " repositories: ${{ github.event.repository.name }}\n") } @@ -74,72 +60,29 @@ func (c *Compiler) generateAPMAppTokenMintStep(app *GitHubAppConfig, stepID stri return steps } -// GenerateAPMPackStep generates the GitHub Actions step that installs APM packages and +// generateAPMPackStep generates the GitHub Actions step that installs APM packages and // packs them into a bundle in the activation job. The step always uses isolated:true because // the activation job has no repo context to preserve. // -// This is the simple (backward-compatible) form: no GitHub App auth. -// For the GitHub App case use generateAPMPackStepWithToken. -// -// Parameters: -// - apmDeps: APM dependency configuration extracted from frontmatter -// - target: APM target derived from the agentic engine (e.g. "copilot", "claude", "all") -// - data: WorkflowData used for action pin resolution -// -// Returns a GitHubActionStep, or an empty step if apmDeps is nil or has no packages. -func GenerateAPMPackStep(apmDeps *APMDependenciesInfo, target string, data *WorkflowData) GitHubActionStep { +// When tokenStepID is non-empty, a GITHUB_TOKEN env override is added so the pack step +// uses the freshly minted GitHub App token instead of the default workflow token. +// When a GitHub App is configured the step ID becomes "apm_pack_0" and the artifact is +// named "apm-0"; otherwise the legacy "apm_pack" / "apm" names are kept for backward compat. +func generateAPMPackStep(apmDeps *APMDependenciesInfo, target string, tokenStepID string) GitHubActionStep { if apmDeps == nil || len(apmDeps.Packages) == 0 { apmDepsLog.Print("No APM dependencies to pack") return GitHubActionStep{} } - apmDepsLog.Printf("Generating APM pack step: %d packages, target=%s", len(apmDeps.Packages), target) - + // Step ID differs between the legacy (no-app) and app-auth cases. stepID := apmPackStepID(apmDeps) - actionRef := GetActionPin("microsoft/apm-action") - lines := []string{ - " - name: Install and pack APM dependencies", - " id: " + stepID, - " uses: " + actionRef, - " with:", - " dependencies: |", - } - - for _, dep := range apmDeps.Packages { - lines = append(lines, " - "+dep) - } - - lines = append(lines, - " isolated: 'true'", - " pack: 'true'", - " archive: 'true'", - " target: "+target, - " working-directory: /tmp/gh-aw/apm-workspace", - ) - - return GitHubActionStep(lines) -} - -// generateAPMPackStepWithToken generates the APM pack step for the GitHub App auth case. -// It sets the GITHUB_TOKEN env to the token minted by the preceding token-mint step. -// Uses step ID "apm_pack_0" (distinguished from the no-auth "apm_pack") so artifact -// references remain consistent when workflows have a github-app configured. -func generateAPMPackStepWithToken(apmDeps *APMDependenciesInfo, target string, tokenStepID string, data *WorkflowData) GitHubActionStep { - if len(apmDeps.Packages) == 0 { - apmDepsLog.Print("No APM packages to pack") - return GitHubActionStep{} - } - - apmDepsLog.Printf("Generating APM pack step with token: %d packages, target=%s, tokenStep=%s", - len(apmDeps.Packages), target, tokenStepID) - - actionRef := GetActionPin("microsoft/apm-action") + apmDepsLog.Printf("Generating APM pack step: id=%s, %d packages, target=%s", stepID, len(apmDeps.Packages), target) lines := []string{ " - name: Install and pack APM dependencies", - " id: " + apmPackStepID(apmDeps), - " uses: " + actionRef, + " id: " + stepID, + " uses: " + GetActionPin("microsoft/apm-action"), } if tokenStepID != "" { @@ -153,11 +96,9 @@ func generateAPMPackStepWithToken(apmDeps *APMDependenciesInfo, target string, t " with:", " dependencies: |", ) - for _, dep := range apmDeps.Packages { lines = append(lines, " - "+dep) } - lines = append(lines, " isolated: 'true'", " pack: 'true'", @@ -169,15 +110,21 @@ func generateAPMPackStepWithToken(apmDeps *APMDependenciesInfo, target string, t return GitHubActionStep(lines) } +// apmArtifactName returns the full artifact name for the APM bundle. +// Legacy (no GitHub App): "apm". With GitHub App: "apm-0". +func apmArtifactName(deps *APMDependenciesInfo, prefix string) string { + base := constants.APMArtifactName // "apm" + if deps.HasGitHubApp() { + base += "-0" + } + return prefix + base +} + // GenerateAPMRestoreStep generates the GitHub Actions step that restores APM packages // from a pre-packed bundle in the agent job. // -// Parameters: -// - apmDeps: APM dependency configuration extracted from frontmatter -// - data: WorkflowData used for action pin resolution -// // Returns a GitHubActionStep, or an empty step if apmDeps is nil or has no packages. -func GenerateAPMRestoreStep(apmDeps *APMDependenciesInfo, data *WorkflowData) GitHubActionStep { +func GenerateAPMRestoreStep(apmDeps *APMDependenciesInfo) GitHubActionStep { if apmDeps == nil || len(apmDeps.Packages) == 0 { apmDepsLog.Print("No APM dependencies to restore") return GitHubActionStep{} @@ -185,18 +132,14 @@ func GenerateAPMRestoreStep(apmDeps *APMDependenciesInfo, data *WorkflowData) Gi apmDepsLog.Printf("Generating APM restore step (isolated=%v)", apmDeps.Isolated) - actionRef := GetActionPin("microsoft/apm-action") - lines := []string{ " - name: Restore APM dependencies", - " uses: " + actionRef, + " uses: " + GetActionPin("microsoft/apm-action"), " with:", " bundle: /tmp/gh-aw/apm-bundle/*.tar.gz", } - if apmDeps.Isolated { lines = append(lines, " isolated: 'true'") } - return GitHubActionStep(lines) } diff --git a/pkg/workflow/apm_dependencies_test.go b/pkg/workflow/apm_dependencies_test.go index 4ce9014a251..f616158e7a8 100644 --- a/pkg/workflow/apm_dependencies_test.go +++ b/pkg/workflow/apm_dependencies_test.go @@ -153,12 +153,12 @@ func TestExtractAPMDependenciesFromFrontmatter(t *testing.T) { "dependencies": map[string]any{ "packages": []any{ map[string]any{ - "foo": "bar", // no source field + "foo": "bar", // non-string entries are silently skipped }, }, }, }, - expectedDeps: nil, // no valid source → skipped + expectedDeps: nil, }, } @@ -233,6 +233,7 @@ func TestGenerateAPMPackStep(t *testing.T) { name string apmDeps *APMDependenciesInfo target string + tokenStepID string expectedContains []string expectedEmpty bool }{ @@ -249,7 +250,7 @@ func TestGenerateAPMPackStep(t *testing.T) { expectedEmpty: true, }, { - name: "Single dependency with copilot target", + name: "Single dependency with copilot target (no token)", apmDeps: &APMDependenciesInfo{Packages: []string{"microsoft/apm-sample-package"}}, target: "copilot", expectedContains: []string{ @@ -265,33 +266,35 @@ func TestGenerateAPMPackStep(t *testing.T) { "working-directory: /tmp/gh-aw/apm-workspace", }, }, + { + name: "With GitHub App token uses apm_pack_0 step ID", + apmDeps: &APMDependenciesInfo{Packages: []string{"acme-org/pkg"}, GitHubApp: &GitHubAppConfig{AppID: "123", PrivateKey: "key"}}, + target: "claude", + tokenStepID: "apm-app-token-0", + expectedContains: []string{ + "id: apm_pack_0", + "env:", + "GITHUB_TOKEN: ${{ steps.apm-app-token-0.outputs.token }}", + "- acme-org/pkg", + "target: claude", + }, + }, { name: "Multiple dependencies with claude target", apmDeps: &APMDependenciesInfo{Packages: []string{"microsoft/apm-sample-package", "github/skills/review"}}, target: "claude", expectedContains: []string{ - "Install and pack APM dependencies", "id: apm_pack", - "microsoft/apm-action", "- microsoft/apm-sample-package", "- github/skills/review", "target: claude", }, }, - { - name: "All target for non-copilot/claude engine", - apmDeps: &APMDependenciesInfo{Packages: []string{"microsoft/apm-sample-package"}}, - target: "all", - expectedContains: []string{ - "target: all", - }, - }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - data := &WorkflowData{Name: "test-workflow"} - step := GenerateAPMPackStep(tt.apmDeps, tt.target, data) + step := generateAPMPackStep(tt.apmDeps, tt.target, tt.tokenStepID) if tt.expectedEmpty { assert.Empty(t, step, "Step should be empty for empty/nil dependencies") @@ -355,8 +358,7 @@ func TestGenerateAPMRestoreStep(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - data := &WorkflowData{Name: "test-workflow"} - step := GenerateAPMRestoreStep(tt.apmDeps, data) + step := GenerateAPMRestoreStep(tt.apmDeps) if tt.expectedEmpty { assert.Empty(t, step, "Step should be empty for empty/nil dependencies") diff --git a/pkg/workflow/compiler_activation_job.go b/pkg/workflow/compiler_activation_job.go index 5dfbf63a33f..31edffb4946 100644 --- a/pkg/workflow/compiler_activation_job.go +++ b/pkg/workflow/compiler_activation_job.go @@ -406,9 +406,8 @@ func (c *Compiler) buildActivationJob(data *WorkflowData, preActivationJobCreate compilerActivationJobLog.Print("Generating prompt in activation job") c.generatePromptInActivationJob(&steps, data, preActivationJobCreated, customJobsBeforeActivation) - // Generate APM pack step(s) if dependencies are specified. - // When no GitHub App is configured: single pack step (backward compat). - // When a GitHub App is configured: one token-mint + pack + upload step. + // Generate APM pack steps if dependencies are specified. + // Optionally mints a GitHub App token first when a github-app is configured. if data.APMDependencies != nil && len(data.APMDependencies.Packages) > 0 { apmTarget := engine.GetAPMTarget() compilerActivationJobLog.Printf("Adding APM pack step: %d packages total", @@ -417,28 +416,23 @@ func (c *Compiler) buildActivationJob(data *WorkflowData, preActivationJobCreate // Mint a GitHub App token before the pack step when configured. tokenStepID := "" if data.APMDependencies.GitHubApp != nil { - tokenStepID = apmAppTokenStepID() + tokenStepID = apmAppTokenStepID compilerActivationJobLog.Print("Adding APM GitHub App token mint step") - tokenSteps := c.generateAPMAppTokenMintStep(data.APMDependencies.GitHubApp, tokenStepID) - steps = append(steps, tokenSteps...) + steps = append(steps, c.generateAPMAppTokenMintStep(data.APMDependencies.GitHubApp)...) } - // Generate the pack step. - var packStep GitHubActionStep - if data.APMDependencies.GitHubApp != nil { - packStep = generateAPMPackStepWithToken(data.APMDependencies, apmTarget, tokenStepID, data) - } else { - packStep = GenerateAPMPackStep(data.APMDependencies, apmTarget, data) - } - for _, line := range packStep { + // Generate the pack step (token is applied when tokenStepID is non-empty). + for _, line := range generateAPMPackStep(data.APMDependencies, apmTarget, tokenStepID) { steps = append(steps, line+"\n") } // Upload the packed APM bundle as a separate artifact for the agent job to download. - // The path comes from the apm_pack / apm_pack_0 step output `bundle-path`. packID := apmPackStepID(data.APMDependencies) - artifactBaseName := apmArtifactBaseName(data.APMDependencies) - artifactName := artifactPrefixExprForActivationJob(data) + artifactBaseName + artifactBaseName := constants.APMArtifactName + if data.APMDependencies.HasGitHubApp() { + artifactBaseName += "-0" + } + artifactName := apmArtifactName(data.APMDependencies, artifactPrefixExprForActivationJob(data)) compilerActivationJobLog.Printf("Adding APM bundle artifact upload step (artifact=%s)", artifactBaseName) steps = append(steps, " - name: Upload APM bundle artifact\n") steps = append(steps, " if: success()\n") diff --git a/pkg/workflow/compiler_yaml_main_job.go b/pkg/workflow/compiler_yaml_main_job.go index 8325bcf44cf..6bad8f8f290 100644 --- a/pkg/workflow/compiler_yaml_main_job.go +++ b/pkg/workflow/compiler_yaml_main_job.go @@ -241,20 +241,16 @@ func (c *Compiler) generateMainJobSteps(yaml *strings.Builder, data *WorkflowDat if data.APMDependencies != nil && len(data.APMDependencies.Packages) > 0 { compilerYamlLog.Printf("Adding APM bundle download step: %d packages total", len(data.APMDependencies.Packages)) - prefix := artifactPrefixExprForDownstreamJob(data) - artifactBaseName := apmArtifactBaseName(data.APMDependencies) - artifactName := prefix + artifactBaseName + artifactName := apmArtifactName(data.APMDependencies, artifactPrefixExprForDownstreamJob(data)) fmt.Fprintf(yaml, " - name: Download APM bundle artifact\n") fmt.Fprintf(yaml, " uses: %s\n", GetActionPin("actions/download-artifact")) yaml.WriteString(" with:\n") fmt.Fprintf(yaml, " name: %s\n", artifactName) yaml.WriteString(" path: /tmp/gh-aw/apm-bundle\n") - // Restore APM dependencies from the bundle - compilerYamlLog.Printf("Adding APM restore step") - apmStep := GenerateAPMRestoreStep(data.APMDependencies, data) - for _, line := range apmStep { + compilerYamlLog.Print("Adding APM restore step") + for _, line := range GenerateAPMRestoreStep(data.APMDependencies) { yaml.WriteString(line + "\n") } } diff --git a/pkg/workflow/frontmatter_extraction_metadata.go b/pkg/workflow/frontmatter_extraction_metadata.go index 65029fc687c..7fe697338b0 100644 --- a/pkg/workflow/frontmatter_extraction_metadata.go +++ b/pkg/workflow/frontmatter_extraction_metadata.go @@ -394,29 +394,12 @@ func extractAPMDependenciesFromFrontmatter(frontmatter map[string]any) *APMDepen } } - // Parse packages: each item is a string; object entries are also accepted if they have a 'source' field + // Parse packages: only string entries are supported. if pkgsAny, ok := v["packages"]; ok { if pkgsArray, ok := pkgsAny.([]any); ok { for _, item := range pkgsArray { - switch pkg := item.(type) { - case string: - if pkg != "" { - packages = append(packages, pkg) - } - case map[string]any: - // Object entry: only the 'source' field is used. - // Any other fields (e.g. 'github-app') are silently ignored. - sourceAny, hasSource := pkg["source"] - if !hasSource { - frontmatterMetadataLog.Print("Skipping APM package entry: missing 'source' field") - continue - } - source, ok := sourceAny.(string) - if !ok || source == "" { - frontmatterMetadataLog.Print("Skipping APM package entry: 'source' field must be a non-empty string") - continue - } - packages = append(packages, source) + if pkg, ok := item.(string); ok && pkg != "" { + packages = append(packages, pkg) } } }