Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/patch-add-dependencies-env-apm.md

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 3 additions & 3 deletions .github/aw/actions-lock.json
Original file line number Diff line number Diff line change
Expand Up @@ -155,10 +155,10 @@
"version": "v2.10.3",
"sha": "9cd1b7bf3f36d5a3c3b17abc3545bfb5481912ea"
},
"microsoft/apm-action@v1.3.1": {
"microsoft/apm-action@v1.3.4": {
"repo": "microsoft/apm-action",
"version": "v1.3.1",
"sha": "5eac264e08ed8db603fe2c40983794f94cab49d8"
"version": "v1.3.4",
"sha": "83d54a6c7941049210433b16c8dfac573665b12a"
},
"oven-sh/setup-bun@v2.1.3": {
"repo": "oven-sh/setup-bun",
Expand Down
10 changes: 5 additions & 5 deletions .github/workflows/smoke-claude.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion pkg/constants/constants.go
Original file line number Diff line number Diff line change
Expand Up @@ -407,7 +407,7 @@ var SerenaLanguageSupport = map[string][]string{
}

// DefaultAPMVersion is the default version of the microsoft/APM (Agent Package Manager) CLI
const DefaultAPMVersion Version = "v0.8.0"
const DefaultAPMVersion Version = "v0.8.2"

// DefaultPlaywrightMCPVersion is the default version of the @playwright/mcp package
const DefaultPlaywrightMCPVersion Version = "0.0.68"
Expand Down
32 changes: 26 additions & 6 deletions pkg/workflow/apm_dependencies.go
Original file line number Diff line number Diff line change
Expand Up @@ -124,12 +124,32 @@ func GenerateAPMPackStep(apmDeps *APMDependenciesInfo, target string, data *Work
" uses: " + actionRef,
}

// Inject the minted GitHub App token as GITHUB_TOKEN so APM can access cross-org private repos
if apmDeps.GitHubApp != nil {
lines = append(lines,
" env:",
fmt.Sprintf(" GITHUB_TOKEN: ${{ steps.%s.outputs.token }}", apmAppTokenStepID),
)
// Build env block: GitHub App token (if configured) + user-provided env vars.
// If github-app is configured, GITHUB_TOKEN is set from the minted app token, so any
// user-supplied GITHUB_TOKEN key is skipped to avoid a duplicate / conflicting entry.
hasGitHubAppToken := apmDeps.GitHubApp != nil
hasUserEnv := len(apmDeps.Env) > 0
if hasGitHubAppToken || hasUserEnv {
lines = append(lines, " env:")
if hasGitHubAppToken {
lines = append(lines,
fmt.Sprintf(" GITHUB_TOKEN: ${{ steps.%s.outputs.token }}", apmAppTokenStepID),
)
}
if hasUserEnv {
keys := make([]string, 0, len(apmDeps.Env))
for k := range apmDeps.Env {
// Skip GITHUB_TOKEN when github-app provides it to avoid duplicate keys
if hasGitHubAppToken && k == "GITHUB_TOKEN" {
continue
}
keys = append(keys, k)
}
sort.Strings(keys)
for _, k := range keys {
lines = append(lines, fmt.Sprintf(" %s: %s", k, apmDeps.Env[k]))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

YAML values are written here without quoting. If a user passes a value containing YAML special characters (e.g., a colon followed by a space, a #, or curly braces), the generated workflow file could be invalid. Consider wrapping the value in quotes or using a YAML-safe formatter: fmt.Sprintf(" %s: %q", k, apmDeps.Env[k]) or at least single-quoting values that contain special chars.

}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good defensive check — skipping GITHUB_TOKEN when a GitHub App token is already being injected prevents duplicate env key conflicts. The sort.Strings(keys) call ensures deterministic output ordering across Go map iterations. 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using sort.Strings(keys) ensures deterministic output ordering for env vars — great practice for generated YAML to keep diffs clean and avoid spurious recompilations.


lines = append(lines,
Expand Down
42 changes: 42 additions & 0 deletions pkg/workflow/apm_dependencies_compilation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -232,3 +232,45 @@ Test with Claude engine target inference
assert.Contains(t, lockContent, "target: claude",
"Lock file should use claude target for claude engine")
}

func TestAPMDependenciesCompilationWithEnv(t *testing.T) {
tmpDir := testutil.TempDir(t, "apm-deps-env-test")

workflow := `---
engine: copilot
on: workflow_dispatch
permissions:
issues: read
pull-requests: read
dependencies:
packages:
- microsoft/apm-sample-package
env:
MY_TOKEN: ${{ secrets.MY_TOKEN }}
REGISTRY: https://registry.example.com
---

Test with env vars on APM pack step
`

testFile := filepath.Join(tmpDir, "test-apm-env.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, "MY_TOKEN:",
"Lock file should include MY_TOKEN env var on pack step")
assert.Contains(t, lockContent, "REGISTRY: https://registry.example.com",
"Lock file should include REGISTRY env var on pack step")
}
152 changes: 152 additions & 0 deletions pkg/workflow/apm_dependencies_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -631,3 +631,155 @@ func TestBuildAPMAppTokenInvalidationStep(t *testing.T) {
assert.Contains(t, combined, "always()", "Must run even if prior steps fail to ensure token cleanup")
})
}

func TestExtractAPMDependenciesEnv(t *testing.T) {
t.Run("Object format with env field extracts env vars", func(t *testing.T) {
frontmatter := map[string]any{
"dependencies": map[string]any{
"packages": []any{"microsoft/apm-sample-package"},
"env": map[string]any{
"MY_TOKEN": "${{ secrets.MY_TOKEN }}",
"REGISTRY": "https://registry.example.com",
},
},
}
result, err := extractAPMDependenciesFromFrontmatter(frontmatter)
require.NoError(t, err, "Should not return an error")
require.NotNil(t, result, "Should return non-nil APMDependenciesInfo")
require.NotNil(t, result.Env, "Env map should be set")
assert.Equal(t, "${{ secrets.MY_TOKEN }}", result.Env["MY_TOKEN"], "MY_TOKEN should be extracted")
assert.Equal(t, "https://registry.example.com", result.Env["REGISTRY"], "REGISTRY should be extracted")
})

t.Run("Object format without env field has nil Env", func(t *testing.T) {
frontmatter := map[string]any{
"dependencies": map[string]any{
"packages": []any{"microsoft/apm-sample-package"},
},
}
result, err := extractAPMDependenciesFromFrontmatter(frontmatter)
require.NoError(t, err, "Should not return an error")
require.NotNil(t, result, "Should return non-nil APMDependenciesInfo")
assert.Nil(t, result.Env, "Env should be nil when not specified")
})

t.Run("Array format has nil Env", func(t *testing.T) {
frontmatter := map[string]any{
"dependencies": []any{"microsoft/apm-sample-package"},
}
result, err := extractAPMDependenciesFromFrontmatter(frontmatter)
require.NoError(t, err, "Should not return an error")
require.NotNil(t, result, "Should return non-nil APMDependenciesInfo")
assert.Nil(t, result.Env, "Env should be nil for array format")
})

t.Run("Non-string env values are skipped", func(t *testing.T) {
frontmatter := map[string]any{
"dependencies": map[string]any{
"packages": []any{"microsoft/apm-sample-package"},
"env": map[string]any{
"VALID": "value",
"INVALID": 42,
},
},
}
result, err := extractAPMDependenciesFromFrontmatter(frontmatter)
require.NoError(t, err, "Should not return an error")
require.NotNil(t, result, "Should return non-nil APMDependenciesInfo")
require.NotNil(t, result.Env, "Env map should be set")
assert.Equal(t, "value", result.Env["VALID"], "String value should be extracted")
assert.NotContains(t, result.Env, "INVALID", "Non-string value should be skipped")
})
}

func TestGenerateAPMPackStepWithEnv(t *testing.T) {
t.Run("Pack step includes user env vars", func(t *testing.T) {
apmDeps := &APMDependenciesInfo{
Packages: []string{"microsoft/apm-sample-package"},
Env: map[string]string{
"MY_TOKEN": "${{ secrets.MY_TOKEN }}",
"REGISTRY": "https://registry.example.com",
},
}
data := &WorkflowData{Name: "test-workflow"}
step := GenerateAPMPackStep(apmDeps, "copilot", data)

require.NotEmpty(t, step, "Step should not be empty")
combined := combineStepLines(step)

assert.Contains(t, combined, "env:", "Should have env section")
assert.Contains(t, combined, "MY_TOKEN: ${{ secrets.MY_TOKEN }}", "Should include MY_TOKEN env var")
assert.Contains(t, combined, "REGISTRY: https://registry.example.com", "Should include REGISTRY env var")
assert.NotContains(t, combined, "GITHUB_TOKEN:", "Should not have GITHUB_TOKEN without github-app")
})

t.Run("Pack step with env vars and github-app includes both", func(t *testing.T) {
apmDeps := &APMDependenciesInfo{
Packages: []string{"acme-org/acme-skills"},
GitHubApp: &GitHubAppConfig{
AppID: "${{ vars.APP_ID }}",
PrivateKey: "${{ secrets.APP_PRIVATE_KEY }}",
},
Env: map[string]string{
"EXTRA": "value",
},
}
data := &WorkflowData{Name: "test-workflow"}
step := GenerateAPMPackStep(apmDeps, "copilot", data)

require.NotEmpty(t, step, "Step should not be empty")
combined := combineStepLines(step)

assert.Contains(t, combined, "GITHUB_TOKEN: ${{ steps.apm-app-token.outputs.token }}", "Should have GITHUB_TOKEN from app")
assert.Contains(t, combined, "EXTRA: value", "Should include user env var")
})

t.Run("Env vars are output in sorted order for determinism", func(t *testing.T) {
apmDeps := &APMDependenciesInfo{
Packages: []string{"microsoft/apm-sample-package"},
Env: map[string]string{
"Z_VAR": "z",
"A_VAR": "a",
"M_VAR": "m",
},
}
data := &WorkflowData{Name: "test-workflow"}
step := GenerateAPMPackStep(apmDeps, "copilot", data)

require.NotEmpty(t, step, "Step should not be empty")
combined := combineStepLines(step)

aPos := strings.Index(combined, "A_VAR:")
mPos := strings.Index(combined, "M_VAR:")
zPos := strings.Index(combined, "Z_VAR:")
require.NotEqual(t, -1, aPos, "A_VAR should be present in output")
require.NotEqual(t, -1, mPos, "M_VAR should be present in output")
require.NotEqual(t, -1, zPos, "Z_VAR should be present in output")
assert.True(t, aPos < mPos && mPos < zPos, "Env vars should be sorted alphabetically")
})

t.Run("GITHUB_TOKEN in user env is skipped when github-app is configured", func(t *testing.T) {
apmDeps := &APMDependenciesInfo{
Packages: []string{"acme-org/acme-skills"},
GitHubApp: &GitHubAppConfig{
AppID: "${{ vars.APP_ID }}",
PrivateKey: "${{ secrets.APP_PRIVATE_KEY }}",
},
Env: map[string]string{
"GITHUB_TOKEN": "should-be-skipped",
"OTHER_VAR": "kept",
},
}
data := &WorkflowData{Name: "test-workflow"}
step := GenerateAPMPackStep(apmDeps, "copilot", data)

require.NotEmpty(t, step, "Step should not be empty")
combined := combineStepLines(step)

assert.Contains(t, combined, "GITHUB_TOKEN: ${{ steps.apm-app-token.outputs.token }}", "Should have GITHUB_TOKEN from app token, not user env")
assert.NotContains(t, combined, "should-be-skipped", "User-supplied GITHUB_TOKEN value should be absent")
assert.Contains(t, combined, "OTHER_VAR: kept", "Other user env vars should be present")
count := strings.Count(combined, "GITHUB_TOKEN:")
assert.Equal(t, 1, count, "GITHUB_TOKEN should appear exactly once")
})
}
6 changes: 3 additions & 3 deletions pkg/workflow/data/action_pins.json
Original file line number Diff line number Diff line change
Expand Up @@ -155,10 +155,10 @@
"version": "v2.10.3",
"sha": "9cd1b7bf3f36d5a3c3b17abc3545bfb5481912ea"
},
"microsoft/apm-action@v1.3.1": {
"microsoft/apm-action@v1.3.4": {
"repo": "microsoft/apm-action",
"version": "v1.3.1",
"sha": "5eac264e08ed8db603fe2c40983794f94cab49d8"
"version": "v1.3.4",
"sha": "83d54a6c7941049210433b16c8dfac573665b12a"
},
"oven-sh/setup-bun@v2.1.3": {
"repo": "oven-sh/setup-bun",
Expand Down
17 changes: 15 additions & 2 deletions pkg/workflow/frontmatter_extraction_metadata.go
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,7 @@ func extractAPMDependenciesFromFrontmatter(frontmatter map[string]any) (*APMDepe
var isolated bool
var githubApp *GitHubAppConfig
var version string
var env map[string]string

switch v := value.(type) {
case []any:
Expand Down Expand Up @@ -403,6 +404,18 @@ func extractAPMDependenciesFromFrontmatter(frontmatter map[string]any) (*APMDepe
version = versionStr
}
}
if envAny, ok := v["env"]; ok {
if envMap, ok := envAny.(map[string]any); ok && len(envMap) > 0 {
env = make(map[string]string, len(envMap))
for k, val := range envMap {
if s, ok := val.(string); ok {
env[k] = s
} else {
frontmatterMetadataLog.Printf("Skipping non-string env value for key '%s'", k)
}
}
}
}
default:
return nil, nil
}
Expand All @@ -411,6 +424,6 @@ func extractAPMDependenciesFromFrontmatter(frontmatter map[string]any) (*APMDepe
return nil, nil
}

frontmatterMetadataLog.Printf("Extracted %d APM dependency packages from frontmatter (isolated=%v, github-app=%v, version=%s)", len(packages), isolated, githubApp != nil, version)
return &APMDependenciesInfo{Packages: packages, Isolated: isolated, GitHubApp: githubApp, Version: version}, nil
frontmatterMetadataLog.Printf("Extracted %d APM dependency packages from frontmatter (isolated=%v, github-app=%v, version=%s, env=%d)", len(packages), isolated, githubApp != nil, version, len(env))
return &APMDependenciesInfo{Packages: packages, Isolated: isolated, GitHubApp: githubApp, Version: version, Env: env}, nil
}
9 changes: 5 additions & 4 deletions pkg/workflow/frontmatter_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -140,10 +140,11 @@ type PluginsConfig struct {
// Supports simple array format and object format with packages, isolated, github-app, and version fields.
// 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
GitHubApp *GitHubAppConfig // Optional GitHub App for cross-org private package access
Version string // Optional APM CLI version override (e.g., "v0.8.0"); defaults to DefaultAPMVersion
Packages []string // APM package slugs to install (e.g., "org/package")
Isolated bool // If true, agent restore step clears primitive dirs before unpacking
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good addition of the Env field to APMDependenciesInfo. Using map[string]string is appropriate here since env vars are key-value pairs. The comment clearly explains its purpose.

GitHubApp *GitHubAppConfig // Optional GitHub App for cross-org private package access
Version string // Optional APM CLI version override (e.g., "v0.8.0"); defaults to DefaultAPMVersion
Env map[string]string // Optional environment variables to set on the APM pack step
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, clean addition. The sorted-key determinism in GenerateAPMPackStep ensures reproducible compiled output, which is great for lock file stability.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed! Deterministic ordering is essential for stable diffs in generated YAML. Great to see this covered by a test.

📰 BREAKING: Report filed by Smoke Copilot

}

// RateLimitConfig represents rate limiting configuration for workflow triggers
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new Env field is well-placed in the struct. Documenting it as "Optional environment variables to set on the APM pack step" makes intent clear. Consistent with the existing comment style in the struct.

Expand Down