-
Notifications
You must be signed in to change notification settings - Fork 475
fix: restore WIF auth mapping support for imported engine definitions (v0.82.10 regression) #47572
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
7257393
4e88a19
67db630
e376282
1e13ca5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -786,3 +786,122 @@ imports: | |
| assert.Contains(t, lockStr, `GH_AW_INFO_ENGINE_ID: "auggie"`, "lock file should set engine ID to the imported definition") | ||
| assert.Contains(t, lockStr, "AUGMENT_SESSION_AUTH: ${{ secrets.AUGMENT_SESSION_AUTH }}", "lock file should bind custom auth secrets from engine.auth") | ||
| } | ||
|
|
||
| // TestImportedEngineWithAnthropicWIFAuth is a regression test for the v0.82.10 regression | ||
| // where an imported engine definition with a mapping-style auth (Anthropic/Azure WIF) caused | ||
| // "mapping was used where sequence is expected" because EngineDefinition.Auth is []AuthBinding. | ||
| // The WIF auth mapping must be stripped before EngineDefinition unmarshaling and handled via | ||
| // the EngineConfig path (applyEngineAuthField), matching the behaviour of inline engine blocks. | ||
| func TestImportedEngineWithAnthropicWIFAuth(t *testing.T) { | ||
| tmpDir := testutil.TempDir(t, "test-wif-auth-import-*") | ||
| workflowsDir := filepath.Join(tmpDir, constants.GetWorkflowDir()) | ||
| sharedDir := filepath.Join(workflowsDir, "shared") | ||
| require.NoError(t, os.MkdirAll(sharedDir, 0755)) | ||
|
|
||
| sharedContent := `--- | ||
| engine: | ||
| id: claude | ||
| auth: | ||
| type: github-oidc | ||
| provider: anthropic | ||
| federation-rule-id: fr_01ABC | ||
| organization-id: org_01XYZ | ||
| service-account-id: sa_01DEF | ||
| workspace-id: ws_01GHI | ||
| --- | ||
|
|
||
| # Shared Anthropic WIF engine config | ||
| ` | ||
| sharedFile := filepath.Join(sharedDir, "wif-engine.md") | ||
| require.NoError(t, os.WriteFile(sharedFile, []byte(sharedContent), 0644)) | ||
|
|
||
| mainContent := `--- | ||
| name: Test Imported WIF Engine | ||
| on: | ||
| workflow_dispatch: | ||
| permissions: | ||
| contents: read | ||
| id-token: write | ||
| imports: | ||
| - shared/wif-engine.md | ||
| --- | ||
|
|
||
| # Test Workflow | ||
| ` | ||
| mainFile := filepath.Join(workflowsDir, "test-wif.md") | ||
| require.NoError(t, os.WriteFile(mainFile, []byte(mainContent), 0644)) | ||
|
|
||
| compiler := NewCompiler() | ||
| err := compiler.CompileWorkflow(mainFile) | ||
| require.NoError(t, err, "compilation must succeed for imported engine definition with Anthropic WIF auth mapping") | ||
|
|
||
| lockFile := filepath.Join(workflowsDir, "test-wif.lock.yml") | ||
| lockContent, err := os.ReadFile(lockFile) | ||
| require.NoError(t, err, "lock file should be created") | ||
|
|
||
| lockStr := string(lockContent) | ||
| assert.Contains(t, lockStr, "AWF_AUTH_TYPE: github-oidc", "lock file must contain WIF auth type") | ||
| assert.Contains(t, lockStr, "AWF_AUTH_PROVIDER: anthropic", "lock file must contain WIF auth provider") | ||
| assert.Contains(t, lockStr, "AWF_AUTH_ANTHROPIC_FEDERATION_RULE_ID: fr_01ABC", "lock file must contain federation rule ID") | ||
| assert.Contains(t, lockStr, "AWF_AUTH_ANTHROPIC_ORGANIZATION_ID: org_01XYZ", "lock file must contain organization ID") | ||
| assert.Contains(t, lockStr, "AWF_AUTH_ANTHROPIC_SERVICE_ACCOUNT_ID: sa_01DEF", "lock file must contain service account ID") | ||
| assert.Contains(t, lockStr, "AWF_AUTH_ANTHROPIC_WORKSPACE_ID: ws_01GHI", "lock file must contain workspace ID") | ||
| } | ||
|
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. Missing regression coverage for sequence-style 💡 Suggested additionAdd a test importing a fragment with sequence-style auth: ---
engine:
id: my-engine
auth:
- role: session
secret: MY_SECRET
---and assert the compiled lock contains the secret binding. The type-assertion guard |
||
|
|
||
| func TestImportedEngineWithAzureWIFAuth(t *testing.T) { | ||
| tmpDir := testutil.TempDir(t, "test-azure-wif-auth-import-*") | ||
| workflowsDir := filepath.Join(tmpDir, constants.GetWorkflowDir()) | ||
| sharedDir := filepath.Join(workflowsDir, "shared") | ||
| require.NoError(t, os.MkdirAll(sharedDir, 0755)) | ||
|
|
||
| sharedContent := `--- | ||
| engine: | ||
| id: copilot | ||
| auth: | ||
| type: github-oidc | ||
| provider: azure | ||
| audience: https://cognitiveservices.azure.com | ||
| azure-tenant-id: tenant-id | ||
| azure-client-id: client-id | ||
| azure-scope: https://cognitiveservices.azure.com/.default | ||
| azure-cloud: public | ||
| --- | ||
|
|
||
| # Shared Azure WIF engine config | ||
| ` | ||
| sharedFile := filepath.Join(sharedDir, "azure-wif-engine.md") | ||
| require.NoError(t, os.WriteFile(sharedFile, []byte(sharedContent), 0644)) | ||
|
|
||
| mainContent := `--- | ||
| name: Test Imported Azure WIF Engine | ||
| on: | ||
| workflow_dispatch: | ||
| permissions: | ||
| contents: read | ||
| id-token: write | ||
| imports: | ||
| - shared/azure-wif-engine.md | ||
| --- | ||
|
|
||
| # Test Workflow | ||
| ` | ||
| mainFile := filepath.Join(workflowsDir, "test-azure-wif.md") | ||
| require.NoError(t, os.WriteFile(mainFile, []byte(mainContent), 0644)) | ||
|
|
||
| compiler := NewCompiler() | ||
| err := compiler.CompileWorkflow(mainFile) | ||
| require.NoError(t, err, "compilation must succeed for imported engine definition with Azure WIF auth mapping") | ||
|
|
||
| lockFile := filepath.Join(workflowsDir, "test-azure-wif.lock.yml") | ||
| lockContent, err := os.ReadFile(lockFile) | ||
| require.NoError(t, err, "lock file should be created") | ||
|
|
||
| lockStr := string(lockContent) | ||
| assert.Contains(t, lockStr, "AWF_AUTH_TYPE: github-oidc", "lock file must contain WIF auth type") | ||
| assert.Contains(t, lockStr, "AWF_AUTH_PROVIDER: azure", "lock file must contain WIF auth provider") | ||
| assert.Contains(t, lockStr, "AWF_AUTH_OIDC_AUDIENCE: https://cognitiveservices.azure.com", "lock file must contain OIDC audience") | ||
| assert.Contains(t, lockStr, "AWF_AUTH_AZURE_TENANT_ID: tenant-id", "lock file must contain Azure tenant ID") | ||
| assert.Contains(t, lockStr, "AWF_AUTH_AZURE_CLIENT_ID: client-id", "lock file must contain Azure client ID") | ||
| assert.Contains(t, lockStr, "AWF_AUTH_AZURE_SCOPE: https://cognitiveservices.azure.com/.default", "lock file must contain Azure scope") | ||
| assert.Contains(t, lockStr, "AWF_AUTH_AZURE_CLOUD: public", "lock file must contain Azure cloud") | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,109 @@ | ||
| //go:build integration | ||
|
|
||
| package workflow | ||
|
|
||
| import ( | ||
| "os" | ||
| "path/filepath" | ||
| "testing" | ||
|
|
||
| "github.com/github/gh-aw/pkg/constants" | ||
| "github.com/github/gh-aw/pkg/testutil" | ||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestImportedEngineWithAnthropicWIFAuthIntegration(t *testing.T) { | ||
| tmpDir := testutil.TempDir(t, "test-imported-wif-auth-*") | ||
| workflowsDir := filepath.Join(tmpDir, constants.GetWorkflowDir()) | ||
| sharedDir := filepath.Join(workflowsDir, "shared") | ||
| require.NoError(t, os.MkdirAll(sharedDir, 0755)) | ||
|
|
||
| sharedContent := `--- | ||
| engine: | ||
| id: claude | ||
| auth: | ||
| type: github-oidc | ||
| provider: anthropic | ||
| federation-rule-id: fr_01ABC | ||
| organization-id: org_01XYZ | ||
| service-account-id: sa_01DEF | ||
| workspace-id: ws_01GHI | ||
| --- | ||
|
|
||
| # Shared Anthropic WIF engine config | ||
| ` | ||
| sharedFile := filepath.Join(sharedDir, "wif-engine.md") | ||
| require.NoError(t, os.WriteFile(sharedFile, []byte(sharedContent), 0644)) | ||
|
|
||
| mainContent := `--- | ||
| name: Test Imported WIF Engine | ||
| on: | ||
| workflow_dispatch: | ||
| permissions: | ||
| contents: read | ||
| id-token: write | ||
| imports: | ||
| - shared/wif-engine.md | ||
| --- | ||
|
|
||
| # Test Workflow | ||
| ` | ||
| mainFile := filepath.Join(workflowsDir, "test-wif.md") | ||
| require.NoError(t, os.WriteFile(mainFile, []byte(mainContent), 0644)) | ||
|
|
||
| compiler := NewCompiler() | ||
| require.NoError(t, compiler.CompileWorkflow(mainFile)) | ||
|
|
||
| lockFile := filepath.Join(workflowsDir, "test-wif.lock.yml") | ||
| lockContent, err := os.ReadFile(lockFile) | ||
| require.NoError(t, err) | ||
|
|
||
| lockStr := string(lockContent) | ||
| assert.Contains(t, lockStr, "AWF_AUTH_TYPE: github-oidc") | ||
| assert.Contains(t, lockStr, "AWF_AUTH_PROVIDER: anthropic") | ||
| assert.Contains(t, lockStr, "AWF_AUTH_ANTHROPIC_FEDERATION_RULE_ID: fr_01ABC") | ||
| assert.Contains(t, lockStr, "AWF_AUTH_ANTHROPIC_ORGANIZATION_ID: org_01XYZ") | ||
| assert.Contains(t, lockStr, "AWF_AUTH_ANTHROPIC_SERVICE_ACCOUNT_ID: sa_01DEF") | ||
| assert.Contains(t, lockStr, "AWF_AUTH_ANTHROPIC_WORKSPACE_ID: ws_01GHI") | ||
| } | ||
|
|
||
| func TestImportedEngineWithMalformedAuthMappingStillFailsIntegration(t *testing.T) { | ||
| tmpDir := testutil.TempDir(t, "test-imported-malformed-auth-*") | ||
| workflowsDir := filepath.Join(tmpDir, constants.GetWorkflowDir()) | ||
| sharedDir := filepath.Join(workflowsDir, "shared") | ||
| require.NoError(t, os.MkdirAll(sharedDir, 0755)) | ||
|
|
||
| sharedContent := `--- | ||
| engine: | ||
| id: claude | ||
| auth: | ||
| role: session | ||
| secret: ANTHROPIC_API_KEY | ||
| --- | ||
|
|
||
| # Shared malformed auth config | ||
| ` | ||
| sharedFile := filepath.Join(sharedDir, "bad-auth-engine.md") | ||
| require.NoError(t, os.WriteFile(sharedFile, []byte(sharedContent), 0644)) | ||
|
|
||
| mainContent := `--- | ||
| name: Test Imported Invalid Engine Auth | ||
| on: | ||
| workflow_dispatch: | ||
| permissions: | ||
| contents: read | ||
| imports: | ||
| - shared/bad-auth-engine.md | ||
| --- | ||
|
|
||
| # Test Workflow | ||
| ` | ||
| mainFile := filepath.Join(workflowsDir, "test-invalid-auth.md") | ||
| require.NoError(t, os.WriteFile(mainFile, []byte(mainContent), 0644)) | ||
|
|
||
| compiler := NewCompiler() | ||
| err := compiler.CompileWorkflow(mainFile) | ||
| require.Error(t, err) | ||
| assert.Contains(t, err.Error(), "mapping was used where sequence is expected") | ||
| } |
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.
[/tdd] The PR title/description mention both Anthropic and Azure WIF, but only an Anthropic provider test was added — Azure WIF (provider: azure) is untested for the imported-engine path.
💡 Suggested addition
Add a parallel
TestImportedEngineWithAzureWIFAuth(or convert to a table-driven test) that setsprovider: azurewith its Azure-specific fields and asserts theAWF_AUTH_AZURE_*env vars appear in the lock file. This ensures the fix covers both providers and guards against future regressions on either.@copilot please address this.