-
Notifications
You must be signed in to change notification settings - Fork 295
feat: Reimplement APM artifact pack/unpack support (#20385) #20564
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -151,23 +151,35 @@ Each plugin repository must be specified in `org/repo` format. The compiler gene | |
|
|
||
| ### APM Dependencies (`dependencies:`) | ||
|
|
||
| Specifies [microsoft/apm](https://github.com/microsoft/apm) packages to install before workflow execution. When present, the compiler emits a step using the `microsoft/apm-action` action to install the listed packages. | ||
| Specifies [microsoft/apm](https://github.com/microsoft/apm) packages to install before workflow execution. When present, the compiler resolves and packs dependencies in the activation job, then unpacks them in the agent job for faster, deterministic startup. | ||
|
|
||
| APM (Agent Package Manager) manages AI agent primitives such as skills, prompts, instructions, agents, and hooks. Packages can depend on other packages and APM resolves the full dependency tree. | ||
|
|
||
| **Simple array format** (most common): | ||
|
|
||
| ```yaml wrap | ||
| dependencies: | ||
| - microsoft/apm-sample-package | ||
| - github/awesome-copilot/skills/review-and-refactor | ||
| - anthropics/skills/skills/frontend-design | ||
| ``` | ||
|
|
||
| **Object format** with options: | ||
|
|
||
| ```yaml wrap | ||
| dependencies: | ||
| packages: | ||
| - microsoft/apm-sample-package | ||
| - github/awesome-copilot/skills/review-and-refactor | ||
| isolated: true # clear repo primitives before unpack (default: false) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice addition of the |
||
| ``` | ||
|
|
||
| Each entry is an APM package reference. Supported formats: | ||
|
|
||
| - `owner/repo` — full APM package | ||
| - `owner/repo/path/to/skill` — individual skill or primitive from a repository | ||
|
|
||
| The compiler generates an `Install APM dependencies` step that runs after the engine CLI installation steps. | ||
| The compiler emits a pack step in the activation job and a restore step in the agent job. The APM target is automatically inferred from the configured engine (`copilot`, `claude`, or `all` for other engines). The `isolated` flag controls whether existing `.github/` primitive directories are cleared before the bundle is unpacked in the agent job. | ||
|
|
||
| ### Runtimes (`runtimes:`) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -148,6 +148,11 @@ type WorkflowExecutor interface { | |
| // before secret redaction runs. Engines that copy session or firewall state files should | ||
| // override this; the default implementation returns an empty slice. | ||
| GetFirewallLogsCollectionStep(workflowData *WorkflowData) []GitHubActionStep | ||
|
|
||
| // GetAPMTarget returns the APM target value to use when packing dependencies with | ||
| // microsoft/apm-action. Supported values are "copilot", "claude", and "all". | ||
| // The default implementation returns "all" (packs all primitive types). | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
| GetAPMTarget() string | ||
| } | ||
|
|
||
| // MCPConfigProvider handles MCP (Model Context Protocol) configuration | ||
|
|
@@ -337,6 +342,12 @@ func (e *BaseEngine) GetFirewallLogsCollectionStep(workflowData *WorkflowData) [ | |
| return []GitHubActionStep{} | ||
| } | ||
|
|
||
| // GetAPMTarget returns "all" by default (packs all primitive types). | ||
| // CopilotEngine overrides this to return "copilot"; ClaudeEngine overrides to return "claude". | ||
| func (e *BaseEngine) GetAPMTarget() string { | ||
| return "all" | ||
| } | ||
|
|
||
| // ParseLogMetrics provides a default no-op implementation for log parsing | ||
| // Engines can override this to provide detailed log parsing and metrics extraction | ||
| func (e *BaseEngine) ParseLogMetrics(logContent string, verbose bool) LogMetrics { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,30 +6,29 @@ import ( | |
|
|
||
| var apmDepsLog = logger.New("workflow:apm_dependencies") | ||
|
|
||
| // GenerateAPMDependenciesStep generates a GitHub Actions step that installs APM packages | ||
| // using the microsoft/apm-action action. The step is emitted when the workflow frontmatter | ||
| // contains a non-empty `dependencies` list in microsoft/apm format. | ||
| // 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. | ||
| // | ||
| // Parameters: | ||
| // - apmDeps: APM dependency configuration extracted from frontmatter | ||
| // - data: WorkflowData used for action pin resolution | ||
| // - 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 GenerateAPMDependenciesStep(apmDeps *APMDependenciesInfo, data *WorkflowData) GitHubActionStep { | ||
| func GenerateAPMPackStep(apmDeps *APMDependenciesInfo, target string, data *WorkflowData) GitHubActionStep { | ||
| if apmDeps == nil || len(apmDeps.Packages) == 0 { | ||
| apmDepsLog.Print("No APM dependencies to install") | ||
| apmDepsLog.Print("No APM dependencies to pack") | ||
| return GitHubActionStep{} | ||
| } | ||
|
|
||
| apmDepsLog.Printf("Generating APM dependencies step: %d packages", len(apmDeps.Packages)) | ||
| apmDepsLog.Printf("Generating APM pack step: %d packages, target=%s", len(apmDeps.Packages), target) | ||
|
|
||
| // Resolve the pinned action reference for microsoft/apm-action. | ||
| actionRef := GetActionPin("microsoft/apm-action") | ||
|
|
||
| // Build step lines. The `dependencies` input uses a YAML block scalar (`|`) | ||
| // so each package is written as an indented list item on its own line. | ||
| lines := []string{ | ||
| " - name: Install APM dependencies", | ||
| " - name: Install and pack APM dependencies", | ||
| " id: apm_pack", | ||
| " uses: " + actionRef, | ||
| " with:", | ||
| " dependencies: |", | ||
|
|
@@ -39,5 +38,45 @@ func GenerateAPMDependenciesStep(apmDeps *APMDependenciesInfo, data *WorkflowDat | |
| 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) | ||
| } | ||
|
|
||
| // GenerateAPMRestoreStep generates the GitHub Actions step that restores APM packages | ||
| // from a pre-packed bundle in the agent job. | ||
| // | ||
| // Parameters: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
| // - 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 { | ||
| if apmDeps == nil || len(apmDeps.Packages) == 0 { | ||
| apmDepsLog.Print("No APM dependencies to restore") | ||
| return GitHubActionStep{} | ||
| } | ||
|
|
||
| apmDepsLog.Printf("Generating APM restore step (isolated=%v)", apmDeps.Isolated) | ||
|
|
||
| actionRef := GetActionPin("microsoft/apm-action") | ||
|
|
||
| lines := []string{ | ||
| " - name: Restore APM dependencies", | ||
| " uses: " + actionRef, | ||
| " with:", | ||
| " bundle: /tmp/gh-aw/apm-bundle/*.tar.gz", | ||
| } | ||
|
|
||
| if apmDeps.Isolated { | ||
| lines = append(lines, " isolated: 'true'") | ||
| } | ||
|
|
||
| return GitHubActionStep(lines) | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -43,14 +43,37 @@ Test with a single APM dependency | |||||||||||||||
|
|
||||||||||||||||
| lockContent := string(content) | ||||||||||||||||
|
|
||||||||||||||||
| assert.Contains(t, lockContent, "Install APM dependencies", | ||||||||||||||||
| "Lock file should contain APM dependencies step name") | ||||||||||||||||
| // Activation job should have the pack step | ||||||||||||||||
| assert.Contains(t, lockContent, "Install and pack APM dependencies", | ||||||||||||||||
| "Lock file should contain APM pack step in activation job") | ||||||||||||||||
| assert.Contains(t, lockContent, "microsoft/apm-action", | ||||||||||||||||
| "Lock file should reference the microsoft/apm-action action") | ||||||||||||||||
| assert.Contains(t, lockContent, "dependencies: |", | ||||||||||||||||
| "Lock file should use block scalar for dependencies input") | ||||||||||||||||
| assert.Contains(t, lockContent, "- microsoft/apm-sample-package", | ||||||||||||||||
| "Lock file should list the dependency package") | ||||||||||||||||
| assert.Contains(t, lockContent, "id: apm_pack", | ||||||||||||||||
| "Lock file should have apm_pack step ID") | ||||||||||||||||
| assert.Contains(t, lockContent, "pack: 'true'", | ||||||||||||||||
| "Lock file should include pack input") | ||||||||||||||||
| assert.Contains(t, lockContent, "target: copilot", | ||||||||||||||||
| "Lock file should include target inferred from copilot engine") | ||||||||||||||||
|
|
||||||||||||||||
| // Separate APM artifact upload in activation job | ||||||||||||||||
| assert.Contains(t, lockContent, "Upload APM bundle artifact", | ||||||||||||||||
| "Lock file should upload APM bundle as separate artifact") | ||||||||||||||||
| assert.Contains(t, lockContent, "name: apm", | ||||||||||||||||
| "Lock file should name the APM artifact 'apm'") | ||||||||||||||||
|
|
||||||||||||||||
| // Agent job should have download + restore steps | ||||||||||||||||
| assert.Contains(t, lockContent, "Download APM bundle artifact", | ||||||||||||||||
| "Lock file should download APM bundle in agent job") | ||||||||||||||||
| assert.Contains(t, lockContent, "Restore APM dependencies", | ||||||||||||||||
| "Lock file should contain APM restore step in agent job") | ||||||||||||||||
| assert.Contains(t, lockContent, "bundle: /tmp/gh-aw/apm-bundle/*.tar.gz", | ||||||||||||||||
| "Lock file should restore from bundle path") | ||||||||||||||||
|
|
||||||||||||||||
| // Old install step should NOT appear | ||||||||||||||||
| assert.NotContains(t, lockContent, "Install APM dependencies", | ||||||||||||||||
| "Lock file should not contain the old install step name") | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| func TestAPMDependenciesCompilationMultiplePackages(t *testing.T) { | ||||||||||||||||
|
|
@@ -85,8 +108,8 @@ Test with multiple APM dependencies | |||||||||||||||
|
|
||||||||||||||||
| lockContent := string(content) | ||||||||||||||||
|
|
||||||||||||||||
| assert.Contains(t, lockContent, "Install APM dependencies", | ||||||||||||||||
| "Lock file should contain APM dependencies step name") | ||||||||||||||||
| assert.Contains(t, lockContent, "Install and pack APM dependencies", | ||||||||||||||||
| "Lock file should contain APM pack step") | ||||||||||||||||
| assert.Contains(t, lockContent, "microsoft/apm-action", | ||||||||||||||||
| "Lock file should reference the microsoft/apm-action action") | ||||||||||||||||
| assert.Contains(t, lockContent, "- microsoft/apm-sample-package", | ||||||||||||||||
|
|
@@ -95,6 +118,8 @@ Test with multiple APM dependencies | |||||||||||||||
| "Lock file should include second dependency") | ||||||||||||||||
| assert.Contains(t, lockContent, "- anthropics/skills/skills/frontend-design", | ||||||||||||||||
| "Lock file should include third dependency") | ||||||||||||||||
| assert.Contains(t, lockContent, "Restore APM dependencies", | ||||||||||||||||
| "Lock file should contain APM restore step") | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| func TestAPMDependenciesCompilationNoDependencies(t *testing.T) { | ||||||||||||||||
|
|
@@ -125,8 +150,85 @@ Test without APM dependencies | |||||||||||||||
|
|
||||||||||||||||
| lockContent := string(content) | ||||||||||||||||
|
|
||||||||||||||||
| assert.NotContains(t, lockContent, "Install APM dependencies", | ||||||||||||||||
| "Lock file should not contain APM dependencies step when no dependencies specified") | ||||||||||||||||
| assert.NotContains(t, lockContent, "Install and pack APM dependencies", | ||||||||||||||||
| "Lock file should not contain APM pack step when no dependencies specified") | ||||||||||||||||
| assert.NotContains(t, lockContent, "Restore APM dependencies", | ||||||||||||||||
| "Lock file should not contain APM restore step when no dependencies specified") | ||||||||||||||||
| assert.NotContains(t, lockContent, "microsoft/apm-action", | ||||||||||||||||
| "Lock file should not reference microsoft/apm-action when no dependencies specified") | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| func TestAPMDependenciesCompilationObjectFormatIsolated(t *testing.T) { | ||||||||||||||||
| tmpDir := testutil.TempDir(t, "apm-deps-isolated-test") | ||||||||||||||||
|
|
||||||||||||||||
| workflow := `--- | ||||||||||||||||
| engine: copilot | ||||||||||||||||
| on: workflow_dispatch | ||||||||||||||||
| permissions: | ||||||||||||||||
| issues: read | ||||||||||||||||
| pull-requests: read | ||||||||||||||||
| dependencies: | ||||||||||||||||
| packages: | ||||||||||||||||
| - microsoft/apm-sample-package | ||||||||||||||||
| isolated: true | ||||||||||||||||
| --- | ||||||||||||||||
|
|
||||||||||||||||
| Test with isolated APM dependencies | ||||||||||||||||
| ` | ||||||||||||||||
|
|
||||||||||||||||
| testFile := filepath.Join(tmpDir, "test-apm-isolated.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) | ||||||||||||||||
|
|
||||||||||||||||
| assert.Contains(t, lockContent, "Install and pack APM dependencies", | ||||||||||||||||
| "Lock file should contain APM pack step") | ||||||||||||||||
| assert.Contains(t, lockContent, "Restore APM dependencies", | ||||||||||||||||
| "Lock file should contain APM restore step") | ||||||||||||||||
| // Restore step should include isolated: true because frontmatter says so | ||||||||||||||||
| assert.Contains(t, lockContent, "isolated: 'true'", | ||||||||||||||||
| "Lock file restore step should include isolated flag") | ||||||||||||||||
|
Comment on lines
+197
to
+199
|
||||||||||||||||
| // Restore step should include isolated: true because frontmatter says so | |
| assert.Contains(t, lockContent, "isolated: 'true'", | |
| "Lock file restore step should include isolated flag") | |
| // Both pack and restore steps should include isolated: true because frontmatter says so | |
| isolatedCount := strings.Count(lockContent, "isolated: 'true'") | |
| assert.Equal(t, 2, isolatedCount, | |
| "Lock file should include isolated flag in both pack and restore steps") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
dependenciesfield looks good here — using the simple array format to add the APM sample package. This will trigger the pack/unpack lifecycle in the activation and agent jobs respectively.