Fix tools.github fallback edge cases and add missing MCP integration test entry

Copilot · pelikhan · Copilot · commit 8ade6cbca817 · 2026-03-18T03:43:44.000Z
Co-authored-by: pelikhan &lt;4175913+pelikhan@users.noreply.github.com&gt;
diff --git a/pkg/workflow/compiler_orchestrator_workflow.go b/pkg/workflow/compiler_orchestrator_workflow.go
@@ -634,29 +634,37 @@ func applyTopLevelGitHubAppFallbacks(data *WorkflowData) {
 		}
 	}
 
-	// Fallback for tools.github (MCP GitHub token minting)
-	if data.ParsedTools != nil && data.ParsedTools.GitHub != nil && data.ParsedTools.GitHub.GitHubApp == nil {
+	// Fallback for tools.github (MCP GitHub token minting).
+	// Skip when a custom github-token is already configured for the MCP server (token takes priority).
+	if data.ParsedTools != nil && data.ParsedTools.GitHub != nil &&
+		data.ParsedTools.GitHub.GitHubApp == nil && data.ParsedTools.GitHub.GitHubToken == "" {
 		orchestratorWorkflowLog.Print("Applying top-level github-app fallback for tools.github")
 		data.ParsedTools.GitHub.GitHubApp = fallback
 		// Also update the raw tools map so applyDefaultTools (called from applyDefaults in
 		// processOnSectionAndFilters) does not lose the fallback when it rebuilds ParsedTools
 		// from the map.
-		if github, ok := data.Tools["github"].(map[string]any); ok {
-			appMap := map[string]any{
-				"app-id":      fallback.AppID,
-				"private-key": fallback.PrivateKey,
-			}
-			if fallback.Owner != "" {
-				appMap["owner"] = fallback.Owner
-			}
-			if len(fallback.Repositories) > 0 {
-				repos := make([]any, len(fallback.Repositories))
-				for i, r := range fallback.Repositories {
-					repos[i] = r
-				}
-				appMap["repositories"] = repos
+		appMap := map[string]any{
+			"app-id":      fallback.AppID,
+			"private-key": fallback.PrivateKey,
+		}
+		if fallback.Owner != "" {
+			appMap["owner"] = fallback.Owner
+		}
+		if len(fallback.Repositories) > 0 {
+			repos := make([]any, len(fallback.Repositories))
+			for i, r := range fallback.Repositories {
+				repos[i] = r
 			}
+			appMap["repositories"] = repos
+		}
+		// Normalize data.Tools["github"] to a map so the github-app survives re-parsing.
+		// Configurations like `github: true` are normalized here rather than losing the fallback.
+		if github, ok := data.Tools["github"].(map[string]any); ok {
+			// Already a map; inject into existing settings.
 			github["github-app"] = appMap
+		} else {
+			// Non-map value (e.g. true/false) — create a fresh map preserving the enabled signal.
+			data.Tools["github"] = map[string]any{"github-app": appMap}
 		}
 	}
 
diff --git a/pkg/workflow/top_level_github_app_import_test.go b/pkg/workflow/top_level_github_app_import_test.go
@@ -137,7 +137,59 @@ This workflow's own top-level github-app takes precedence over the imported one.
 		"Current workflow's github-app should take precedence over the imported one")
 }
 
-// TestTopLevelGitHubAppImportActivation tests that a top-level github-app imported from a shared
+// TestTopLevelGitHubAppToolsGitHubTokenSkip tests that the fallback is NOT applied
+// to tools.github when a custom github-token is already configured for the MCP server.
+func TestTopLevelGitHubAppToolsGitHubTokenSkip(t *testing.T) {
+	compiler := NewCompilerWithVersion("1.0.0")
+
+	// Use ParseWorkflowFile directly with inline frontmatter
+	tmpDir := t.TempDir()
+	workflowsDir := filepath.Join(tmpDir, ".github", "workflows")
+	require.NoError(t, os.MkdirAll(workflowsDir, 0755))
+
+	// Workflow with top-level github-app but tools.github uses an explicit github-token
+	workflowContent := `---
+on: issues
+permissions:
+  contents: read
+github-app:
+  app-id: ${{ vars.APP_ID }}
+  private-key: ${{ secrets.APP_PRIVATE_KEY }}
+tools:
+  github:
+    mode: remote
+    toolsets: [default]
+    github-token: ${{ secrets.CUSTOM_PAT }}
+engine: copilot
+---
+
+# Workflow With Explicit GitHub Token for MCP
+
+When tools.github.github-token is set, the top-level github-app fallback should NOT override it.
+`
+	mdPath := filepath.Join(workflowsDir, "main.md")
+	require.NoError(t, os.WriteFile(mdPath, []byte(workflowContent), 0644))
+
+	origDir, err := os.Getwd()
+	require.NoError(t, err)
+	require.NoError(t, os.Chdir(workflowsDir))
+	defer func() { _ = os.Chdir(origDir) }()
+
+	data, err := compiler.ParseWorkflowFile("main.md")
+	require.NoError(t, err)
+
+	// The top-level github-app should be resolved at the top level
+	require.NotNil(t, data.TopLevelGitHubApp, "TopLevelGitHubApp should be populated")
+
+	// But it must NOT be injected into tools.github because github-token takes precedence
+	require.NotNil(t, data.ParsedTools, "ParsedTools should be populated")
+	require.NotNil(t, data.ParsedTools.GitHub, "ParsedTools.GitHub should be populated")
+	assert.Equal(t, "${{ secrets.CUSTOM_PAT }}", data.ParsedTools.GitHub.GitHubToken,
+		"tools.github.github-token should be preserved")
+	assert.Nil(t, data.ParsedTools.GitHub.GitHubApp,
+		"tools.github.github-app should NOT be set when github-token is configured")
+}
+
 // workflow is propagated to the activation job (reactions/status comments).
 func TestTopLevelGitHubAppImportActivation(t *testing.T) {
 	compiler := NewCompilerWithVersion("1.0.0")
diff --git a/pkg/workflow/top_level_github_app_integration_test.go b/pkg/workflow/top_level_github_app_integration_test.go
@@ -366,6 +366,14 @@ func TestTopLevelGitHubAppWorkflowFiles(t *testing.T) {
 				"app-id: ${{ vars.ACTIVATION_APP_ID }}",
 			},
 		},
+		{
+			name:         "tools.github MCP fallback workflow file",
+			workflowFile: "../cli/workflows/test-top-level-github-app-mcp.md",
+			expectContains: []string{
+				"id: github-mcp-app-token",
+				"app-id: ${{ vars.APP_ID }}",
+			},
+		},
 	}
 
 	for _, tt := range tests {
