diff --git a/.github/workflows/smoke-call-workflow.lock.yml b/.github/workflows/smoke-call-workflow.lock.yml index ee338952a0a..00e961a3681 100644 --- a/.github/workflows/smoke-call-workflow.lock.yml +++ b/.github/workflows/smoke-call-workflow.lock.yml @@ -1131,6 +1131,8 @@ jobs: call-smoke-workflow-call: needs: safe_outputs if: needs.safe_outputs.outputs.call_workflow_name == 'smoke-workflow-call' + # Imported from called workflow "smoke-workflow-call" because GitHub requires the caller job to grant permissions requested by reusable workflow jobs. + # Review the called workflow's job-level permissions in ./.github/workflows/smoke-workflow-call.lock.yml. permissions: actions: read contents: read diff --git a/pkg/workflow/call_workflow_permissions.go b/pkg/workflow/call_workflow_permissions.go index 609581df7fd..1c2c90bd931 100644 --- a/pkg/workflow/call_workflow_permissions.go +++ b/pkg/workflow/call_workflow_permissions.go @@ -3,14 +3,31 @@ package workflow import ( "fmt" "os" + "path/filepath" "sort" + "strings" + "github.com/github/gh-aw/pkg/constants" "github.com/github/gh-aw/pkg/logger" "github.com/github/gh-aw/pkg/parser" ) var callWorkflowPermissionsLog = logger.New("workflow:call_workflow_permissions") +type workflowSourceKind string + +const ( + workflowSourceKindLock workflowSourceKind = "lock" + workflowSourceKindYAML workflowSourceKind = "yaml" + workflowSourceKindMarkdown workflowSourceKind = "markdown" +) + +type callWorkflowPermissionImport struct { + permissions *Permissions + sourcePath string + sourceKind workflowSourceKind +} + // permissionLevelRank maps a permission level to a comparable rank where a higher // number grants strictly more access (none < read < write). Used to determine // whether one permission set covers another. Unknown or empty levels rank as 0. @@ -101,6 +118,10 @@ func extractJobPermissionsFromParsedWorkflow(workflow map[string]any) *Permissio return merged } +// extractCallWorkflowPermissions is a compatibility helper used by existing tests. +// New production code should prefer extractCallWorkflowPermissionImport when it +// needs both the permissions and their review source metadata. +// // extractCallWorkflowPermissions returns the permission superset required by the worker // workflow identified by workflowName. It resolves the file in priority order: // .lock.yml > .yml > .md (same-batch compilation target). @@ -121,6 +142,14 @@ func extractJobPermissionsFromParsedWorkflow(workflow map[string]any) *Permissio // extractJobPermissionsFromParsedWorkflow initialises a fresh Permissions map // regardless of whether any jobs declare a permissions block. func extractCallWorkflowPermissions(workflowName, markdownPath string) (*Permissions, error) { + imported, err := extractCallWorkflowPermissionImport(workflowName, markdownPath) + if err != nil || imported == nil { + return nil, err + } + return imported.permissions, nil +} + +func extractCallWorkflowPermissionImport(workflowName, markdownPath string) (*callWorkflowPermissionImport, error) { fileResult, err := findWorkflowFile(workflowName, markdownPath) if err != nil { return nil, fmt.Errorf("failed to find workflow file for '%s': %w", workflowName, err) @@ -128,15 +157,42 @@ func extractCallWorkflowPermissions(workflowName, markdownPath string) (*Permiss // Priority: .lock.yml > .yml > .md if fileResult.lockExists { - return extractPermissionsFromYAMLFile(fileResult.lockPath) + perms, err := extractPermissionsFromYAMLFile(fileResult.lockPath) + if err != nil { + return nil, err + } + return &callWorkflowPermissionImport{ + permissions: perms, + sourcePath: fileResult.lockPath, + sourceKind: workflowSourceKindLock, + }, nil } if fileResult.ymlExists { - return extractPermissionsFromYAMLFile(fileResult.ymlPath) + perms, err := extractPermissionsFromYAMLFile(fileResult.ymlPath) + if err != nil { + return nil, err + } + return &callWorkflowPermissionImport{ + permissions: perms, + sourcePath: fileResult.ymlPath, + sourceKind: workflowSourceKindYAML, + }, nil } if fileResult.mdExists { - return extractPermissionsFromMDFile(fileResult.mdPath) + perms, err := extractPermissionsFromMDFile(fileResult.mdPath) + if err != nil { + return nil, err + } + if perms == nil { + return nil, nil + } + return &callWorkflowPermissionImport{ + permissions: perms, + sourcePath: fileResult.mdPath, + sourceKind: workflowSourceKindMarkdown, + }, nil } // No file found — return nil so the caller omits the permissions block. @@ -144,6 +200,32 @@ func extractCallWorkflowPermissions(workflowName, markdownPath string) (*Permiss return nil, nil } +func buildCallWorkflowPermissionsComment(workflowName string, imported *callWorkflowPermissionImport) string { + if imported == nil || imported.permissions == nil { + return "" + } + if imported.permissions.RenderToYAML() == "" { + return "" + } + + reviewWhat := "job-level permissions" + if imported.sourceKind == workflowSourceKindMarkdown { + reviewWhat = "frontmatter permissions" + } + + return strings.Join([]string{ + fmt.Sprintf("# Imported from called workflow %q because GitHub requires the caller job to grant permissions requested by reusable workflow jobs.", workflowName), + fmt.Sprintf("# Review the called workflow's %s in %s.", reviewWhat, renderWorkflowReviewPath(imported.sourcePath)), + }, "\n") +} + +// renderWorkflowReviewPath converts an absolute workflow path to the canonical +// repo-relative display path used in generated review comments. This assumes +// workflow files live directly in constants.GetWorkflowDir(). +func renderWorkflowReviewPath(sourcePath string) string { + return "./" + filepath.ToSlash(filepath.Join(constants.GetWorkflowDir(), filepath.Base(sourcePath))) +} + // extractPermissionsFromYAMLFile reads a .lock.yml or .yml workflow file, parses it, // and returns the merged permissions from all its jobs. func extractPermissionsFromYAMLFile(filePath string) (*Permissions, error) { diff --git a/pkg/workflow/call_workflow_permissions_test.go b/pkg/workflow/call_workflow_permissions_test.go index 37bf2c480be..febf4c429e7 100644 --- a/pkg/workflow/call_workflow_permissions_test.go +++ b/pkg/workflow/call_workflow_permissions_test.go @@ -268,11 +268,75 @@ func TestExtractCallWorkflowPermissions_FileNotFound(t *testing.T) { assert.Nil(t, perms, "Should return nil when no file exists") } +func TestExtractCallWorkflowPermissionImport_MDWithoutPermissionsReturnsNil(t *testing.T) { + tmpDir := t.TempDir() + workflowsDir := filepath.Join(tmpDir, ".github", "workflows") + require.NoError(t, os.MkdirAll(workflowsDir, 0755), "Failed to create workflows directory") + + mdContent := `--- +on: + workflow_call: {} +engine: copilot +--- + +# Worker Without Permissions +` + require.NoError(t, os.WriteFile(filepath.Join(workflowsDir, "worker-no-perms.md"), []byte(mdContent), 0644), "Failed to write worker-no-perms.md") + + markdownPath := filepath.Join(workflowsDir, "gateway.md") + + imported, err := extractCallWorkflowPermissionImport("worker-no-perms", markdownPath) + require.NoError(t, err, "Should not error when markdown worker has no permissions") + assert.Nil(t, imported, "Should treat markdown workers with no permissions like other missing-import cases") +} + +func TestExtractCallWorkflowPermissionImport_TracksReviewSource(t *testing.T) { + t.Setenv("GH_AW_WORKFLOWS_DIR", "") + + tmpDir := t.TempDir() + workflowsDir := filepath.Join(tmpDir, ".github", "workflows") + require.NoError(t, os.MkdirAll(workflowsDir, 0755), "Failed to create workflows directory") + + lockContent := `name: Worker Lock +on: + workflow_call: {} +jobs: + work: + permissions: + contents: write + runs-on: ubuntu-latest + steps: + - run: echo "lock" +` + require.NoError(t, os.WriteFile(filepath.Join(workflowsDir, "worker-review.lock.yml"), []byte(lockContent), 0644), "Failed to write worker-review.lock.yml") + + markdownPath := filepath.Join(workflowsDir, "gateway.md") + + imported, err := extractCallWorkflowPermissionImport("worker-review", markdownPath) + require.NoError(t, err, "Should extract imported permissions without error") + require.NotNil(t, imported, "Should return import metadata") + require.NotNil(t, imported.permissions, "Should include permissions") + assert.Equal(t, workflowSourceKindLock, imported.sourceKind, "Should track lock workflow source kind") + assert.Equal(t, "./.github/workflows/worker-review.lock.yml", renderWorkflowReviewPath(imported.sourcePath), + "Should render a repo-relative review path for help comments") +} + +func TestBuildCallWorkflowPermissionsComment_NilInputs(t *testing.T) { + assert.Empty(t, buildCallWorkflowPermissionsComment("worker", nil), "Nil import should not emit a comment") + assert.Empty(t, buildCallWorkflowPermissionsComment("worker", &callWorkflowPermissionImport{}), "Nil permissions should not emit a comment") + assert.Empty(t, buildCallWorkflowPermissionsComment("worker", &callWorkflowPermissionImport{ + permissions: NewPermissions(), + sourceKind: workflowSourceKindLock, + }), "Empty permissions should not emit a comment") +} + // TestBuildCallWorkflowJobs_SetsPermissionsFromLockYML tests that call-workflow jobs // carry the union of caller + worker permissions when a .lock.yml worker file is present. // When the caller already covers all of the worker's needs, the effective permissions // equal the caller's declared permissions. func TestBuildCallWorkflowJobs_SetsPermissionsFromLockYML(t *testing.T) { + t.Setenv("GH_AW_WORKFLOWS_DIR", "") + compiler := NewCompiler(WithVersion("1.0.0")) tmpDir := t.TempDir() @@ -326,6 +390,12 @@ jobs: job, exists := compiler.jobManager.GetJob("call-worker-docs") require.True(t, exists, "Job should exist in job manager") + assert.Contains(t, job.PermissionsComment, + `Imported from called workflow "worker-docs" because GitHub requires the caller job to grant permissions requested by reusable workflow jobs.`, + "Job should explain why worker permissions are imported") + assert.Contains(t, job.PermissionsComment, + "Review the called workflow's job-level permissions in ./.github/workflows/worker-docs.lock.yml.", + "Job should point reviewers to the compiled worker workflow") assert.NotEmpty(t, job.Permissions, "Job should have permissions set") assert.Contains(t, job.Permissions, "contents: write", "Permissions should include contents: write") assert.Contains(t, job.Permissions, "issues: write", "Permissions should include issues: write") @@ -337,6 +407,8 @@ jobs: // target. When caller and worker declare the same permissions, the effective permissions // equal the caller's declared permissions. func TestBuildCallWorkflowJobs_SetsPermissionsFromMD(t *testing.T) { + t.Setenv("GH_AW_WORKFLOWS_DIR", "") + compiler := NewCompiler(WithVersion("1.0.0")) tmpDir := t.TempDir() @@ -379,6 +451,9 @@ permissions: job, exists := compiler.jobManager.GetJob("call-worker-e") require.True(t, exists, "Job should exist in job manager") + assert.Contains(t, job.PermissionsComment, + "Review the called workflow's frontmatter permissions in ./.github/workflows/worker-e.md.", + "Job should point reviewers to the markdown worker when no compiled file exists yet") assert.NotEmpty(t, job.Permissions, "Job should have permissions") assert.Contains(t, job.Permissions, "contents: read", "Permissions should include contents: read") assert.Contains(t, job.Permissions, "issues: write", "Permissions should include issues: write") @@ -547,6 +622,12 @@ jobs: assert.Contains(t, yamlOutput, "uses: ./.github/workflows/worker-a.lock.yml", "Should contain uses directive") assert.Contains(t, yamlOutput, "secrets: inherit", "Should inherit secrets") assert.Contains(t, yamlOutput, "permissions:", "Should include permissions block") + assert.Contains(t, yamlOutput, + `# Imported from called workflow "worker-a" because GitHub requires the caller job to grant permissions requested by reusable workflow jobs.`, + "Rendered YAML should explain imported workflow_call permissions") + assert.Contains(t, yamlOutput, + "# Review the called workflow's job-level permissions in ./.github/workflows/worker-a.lock.yml.", + "Rendered YAML should point to the worker workflow for review") // The call-* job gets the union of caller + worker permissions. Since the caller // already covers all of the worker's needs, the effective permissions equal the // caller's declared permissions. diff --git a/pkg/workflow/compiler_safe_output_jobs.go b/pkg/workflow/compiler_safe_output_jobs.go index 3698a04ee7e..12c73afbebb 100644 --- a/pkg/workflow/compiler_safe_output_jobs.go +++ b/pkg/workflow/compiler_safe_output_jobs.go @@ -169,10 +169,10 @@ func (c *Compiler) buildSafeOutputsJobs(data *WorkflowData, jobName, markdownPat // - `payload` is forwarded as the raw transport only when the worker declares it // (GitHub Actions rejects undeclared inputs) // - inherits all caller secrets via `secrets: inherit` -// - includes a job-level `permissions:` block derived from the CALLER's own -// declared permissions (not the worker's). The caller controls its own -// permission surface; the compiler validates that the declared permissions -// cover what the worker requires and warns if they do not. +// - includes a job-level `permissions:` block equal to the union of the +// caller's declared permissions and the called worker's required permissions +// - adds a help comment explaining why imported worker permissions appear on +// the call job and where to review them in the worker workflow source // // Returns the names of all generated jobs so they can be added to the conclusion // job's `needs` list. @@ -305,14 +305,16 @@ func (c *Compiler) buildCallWorkflowJobs(data *WorkflowData, markdownPath string } effectivePerms := callerPerms + var importedPerms *callWorkflowPermissionImport + var permErr error if markdownPath != "" { - workerPerms, permErr := extractCallWorkflowPermissions(workflowName, markdownPath) + importedPerms, permErr = extractCallWorkflowPermissionImport(workflowName, markdownPath) if permErr != nil { // Non-fatal: log and continue. The worker file may not exist yet (it may be // compiled in the same batch), in which case we fall back to the caller's // own declared permissions. compilerSafeOutputJobsLog.Printf("Could not extract worker permissions for call-workflow job '%s' (falling back to caller-only permissions): %v", jobName, permErr) - } else if workerPerms != nil { + } else if importedPerms != nil && importedPerms.permissions != nil { // Compute the union by merging caller and worker permissions into a // fresh map-based Permissions. Starting from a blank slate (rather // than a clone of callerPerms) ensures shorthand values like @@ -322,7 +324,7 @@ func (c *Compiler) buildCallWorkflowJobs(data *WorkflowData, markdownPath string // expanding it, silently dropping the caller's baseline grant. merged := NewPermissions() merged.Merge(callerPerms) - merged.Merge(workerPerms) + merged.Merge(importedPerms.permissions) effectivePerms = merged compilerSafeOutputJobsLog.Printf("Merged caller and worker permissions for call-workflow job '%s'", jobName) } @@ -331,6 +333,7 @@ func (c *Compiler) buildCallWorkflowJobs(data *WorkflowData, markdownPath string if effectivePerms != nil { rendered := effectivePerms.RenderToYAML() if rendered != "" { + callJob.PermissionsComment = buildCallWorkflowPermissionsComment(workflowName, importedPerms) callJob.Permissions = rendered compilerSafeOutputJobsLog.Printf("Set permissions on call-workflow job '%s': %s", jobName, rendered) } diff --git a/pkg/workflow/jobs.go b/pkg/workflow/jobs.go index 06d73b5960d..55976c0d9a4 100644 --- a/pkg/workflow/jobs.go +++ b/pkg/workflow/jobs.go @@ -42,6 +42,7 @@ type Job struct { RunsOn string If string HasWorkflowRunSafetyChecks bool // If true, the job's if condition includes workflow_run safety checks + PermissionsComment string Permissions string TimeoutMinutes int TimeoutMinutesExpression string @@ -292,6 +293,11 @@ func (jm *JobManager) renderJobTo(b *strings.Builder, job *Job) { } // Add permissions section + if job.PermissionsComment != "" { + for line := range strings.SplitSeq(strings.TrimRight(job.PermissionsComment, "\n"), "\n") { + fmt.Fprintf(b, " %s\n", line) + } + } if job.Permissions != "" { fmt.Fprintf(b, " %s\n", job.Permissions) }