diff --git a/pkg/workflow/compiler_cache_regression_test.go b/pkg/workflow/compiler_cache_regression_test.go new file mode 100644 index 00000000000..49eadd56d7b --- /dev/null +++ b/pkg/workflow/compiler_cache_regression_test.go @@ -0,0 +1,90 @@ +package workflow + +import ( + "os" + "path/filepath" + "testing" + + "github.com/github/gh-aw/pkg/testutil" + "github.com/stretchr/testify/require" +) + +func TestComputeAllowedDomainsForSanitizationCacheReplacesByPath(t *testing.T) { + compiler := NewCompiler() + compiler.markdownPath = "/tmp/workflow-a.md" + + data := &WorkflowData{ + FrontmatterHash: "hash-1", + AI: "copilot", + } + + first, err := compiler.computeAllowedDomainsForSanitization(data) + require.NoError(t, err) + require.NotEmpty(t, first) + require.Len(t, compiler.allowedDomainsCache, 1) + require.Equal(t, "hash-1", compiler.allowedDomainsCache["/tmp/workflow-a.md"].frontmatterHash) + + data2 := &WorkflowData{ + FrontmatterHash: "hash-2", + AI: "copilot", + } + second, err := compiler.computeAllowedDomainsForSanitization(data2) + require.NoError(t, err) + require.NotEmpty(t, second) + require.Len(t, compiler.allowedDomainsCache, 1) + require.Equal(t, "hash-2", compiler.allowedDomainsCache["/tmp/workflow-a.md"].frontmatterHash) + require.Equal(t, second, compiler.allowedDomainsCache["/tmp/workflow-a.md"].domains) + + compiler.markdownPath = "/tmp/workflow-b.md" + data3 := &WorkflowData{ + FrontmatterHash: "hash-3", + AI: "copilot", + } + _, err = compiler.computeAllowedDomainsForSanitization(data3) + require.NoError(t, err) + require.Len(t, compiler.allowedDomainsCache, 2) +} + +func TestPermissionWarningsCountAcrossCompilations(t *testing.T) { + tmpDir := testutil.TempDir(t, "permission-warning-hash") + testFile := filepath.Join(tmpDir, "workflow.md") + + content1 := `--- +on: push +permissions: + contents: read +tools: + github: + toolsets: [issues] +--- + +# Test workflow +` + + content2 := `--- +on: push +permissions: + contents: read +tools: + github: + toolsets: [pull_requests] +--- + +# Test workflow +` + + require.NoError(t, os.WriteFile(testFile, []byte(content1), 0o644)) + + compiler := NewCompiler() + require.NoError(t, compiler.CompileWorkflow(testFile)) + require.Positive(t, compiler.GetWarningCount()) + + compiler.ResetWarningCount() + require.NoError(t, compiler.CompileWorkflow(testFile)) + require.Positive(t, compiler.GetWarningCount()) + + require.NoError(t, os.WriteFile(testFile, []byte(content2), 0o644)) + compiler.ResetWarningCount() + require.NoError(t, compiler.CompileWorkflow(testFile)) + require.Positive(t, compiler.GetWarningCount()) +} diff --git a/pkg/workflow/compiler_performance_benchmark_test.go b/pkg/workflow/compiler_performance_benchmark_test.go index 6597923c248..64356d580f0 100644 --- a/pkg/workflow/compiler_performance_benchmark_test.go +++ b/pkg/workflow/compiler_performance_benchmark_test.go @@ -130,6 +130,7 @@ permissions: pull-requests: read actions: read issues: read + discussions: read engine: copilot tools: github: diff --git a/pkg/workflow/compiler_types.go b/pkg/workflow/compiler_types.go index 03e9557df45..105ae537f77 100644 --- a/pkg/workflow/compiler_types.go +++ b/pkg/workflow/compiler_types.go @@ -108,6 +108,8 @@ type Compiler struct { ghesArtifactCompat bool // If true, GHES compatibility mode is enabled; artifact actions still use latest non-v3 pins ownerTypeCache map[string]string // Cached GitHub owner type ("User"/"Organization"/"") keyed by owner login; not goroutine-safe (Compiler is used sequentially) copilotRequestsTipShown map[string]bool // Tracks markdown paths that already emitted the copilot-requests enable tip in this compiler instance + permissionWarningShown map[string]string // Tracks markdown paths and last warning fingerprint (frontmatter hash when available, otherwise formatted warning text) + allowedDomainsCache map[string]allowedDomain // Cached allowed-domains per markdown path with the frontmatter hash that produced it // modelPricingResolver is an optional callback for resolving per-token pricing of models that // are absent from the embedded models.json catalog. When non-nil it is called during // buildInitialWorkflowData for the workflow's configured model; any returned pricing is merged @@ -116,6 +118,11 @@ type Compiler struct { modelPricingResolver func(ctx context.Context, provider, model string) (map[string]float64, bool) } +type allowedDomain struct { + frontmatterHash string + domains string +} + // NewCompiler creates a new workflow compiler with functional options. // By default, it auto-detects the version and action mode. // Common options: WithVerbose, WithEngineOverride, WithNoEmit, WithSkipValidation @@ -142,9 +149,11 @@ func NewCompiler(opts ...CompilerOption) *Compiler { artifactManager: NewArtifactManager(), actionPinWarnings: make(map[string]bool), // Initialize warning cache priorManifests: make(map[string]*GHAWManifest), - ownerTypeCache: make(map[string]string), // Initialize owner-type cache (keyed by owner login) - copilotRequestsTipShown: make(map[string]bool), // Initialize one-time tip tracking (keyed by markdown path) - gitRoot: gitRoot, // Auto-detected git root + ownerTypeCache: make(map[string]string), // Initialize owner-type cache (keyed by owner login) + copilotRequestsTipShown: make(map[string]bool), // Initialize one-time tip tracking (keyed by markdown path) + permissionWarningShown: make(map[string]string), // Initialize one-time permission warning tracking (keyed by markdown path) + allowedDomainsCache: make(map[string]allowedDomain), // Initialize allowed-domains cache (keyed by markdown path) + gitRoot: gitRoot, // Auto-detected git root } // Apply functional options diff --git a/pkg/workflow/domains.go b/pkg/workflow/domains.go index 8e792d3fc7a..cfa785a15b3 100644 --- a/pkg/workflow/domains.go +++ b/pkg/workflow/domains.go @@ -880,6 +880,9 @@ func mergeAPITargetDomains(domainsStr string, apiTarget string) string { // The result is cached in data.CachedAllowedDomainsStr after the first call so that // repeated calls (e.g. from the activation job, safe-outputs steps, and agent run step) // do not recompute the same domain list. +// Additionally, results are cached on the Compiler keyed by markdown path with the +// current FrontmatterHash so repeated compilations of an unchanged workflow skip the +// full domain computation without unbounded hash-key growth in watch mode. // Returns an error if the engine's model is malformed (e.g. a leading slash). func (c *Compiler) computeAllowedDomainsForSanitization(data *WorkflowData) (string, error) { // Return cached result if available (engine/network/tools/runtimes do not change during compilation). @@ -889,6 +892,16 @@ func (c *Compiler) computeAllowedDomainsForSanitization(data *WorkflowData) (str return data.CachedAllowedDomainsStr, nil } + // Check the Compiler-level cache keyed by markdown path. + // A cached entry is reusable only when the current frontmatter hash matches. + if c.markdownPath != "" && data.FrontmatterHash != "" { + if cached, ok := c.allowedDomainsCache[c.markdownPath]; ok && cached.frontmatterHash == data.FrontmatterHash { + data.CachedAllowedDomainsStr = cached.domains + data.CachedAllowedDomainsComputed = true + return cached.domains, nil + } + } + // Determine which engine is being used var engineID string if data.EngineConfig != nil { @@ -941,6 +954,16 @@ func (c *Compiler) computeAllowedDomainsForSanitization(data *WorkflowData) (str // Set the boolean sentinel first so that an empty result is also treated as cached. data.CachedAllowedDomainsComputed = true data.CachedAllowedDomainsStr = base + + // Populate the Compiler-level cache so subsequent compilations of the same + // workflow path and unchanged frontmatter skip this computation entirely. + if c.markdownPath != "" && data.FrontmatterHash != "" { + c.allowedDomainsCache[c.markdownPath] = allowedDomain{ + frontmatterHash: data.FrontmatterHash, + domains: base, + } + } + return base, nil } diff --git a/pkg/workflow/permissions_compiler_validator.go b/pkg/workflow/permissions_compiler_validator.go index c9ed25532c7..bfa7888011c 100644 --- a/pkg/workflow/permissions_compiler_validator.go +++ b/pkg/workflow/permissions_compiler_validator.go @@ -143,9 +143,20 @@ func (c *Compiler) validatePermissions(workflowData *WorkflowData, markdownPath message += "\n\n" + missingPermissionsDefaultToolsetWarning } - // In non-strict mode, missing permissions are warnings. - // In strict mode with default-only toolsets, this is intentionally downgraded to warning. - fmt.Fprintln(os.Stderr, formatCompilerMessage(markdownPath, "warning", message)) + // Emit to stderr once per markdown path + warning fingerprint. + // Prefer frontmatter hash when available; otherwise use the formatted + // message as a fallback fingerprint for code paths/tests where the hash + // is not set. + warningFingerprint := workflowData.FrontmatterHash + if warningFingerprint == "" { + warningFingerprint = message + } + if c.permissionWarningShown[markdownPath] != warningFingerprint { + // In non-strict mode, missing permissions are warnings. + // In strict mode with default-only toolsets, this is intentionally downgraded to warning. + fmt.Fprintln(os.Stderr, formatCompilerMessage(markdownPath, "warning", message)) + c.permissionWarningShown[markdownPath] = warningFingerprint + } c.IncrementWarningCount() } } diff --git a/pkg/workflow/permissions_scope_validation_test.go b/pkg/workflow/permissions_scope_validation_test.go index 01e2cafbe91..174ee3c7918 100644 --- a/pkg/workflow/permissions_scope_validation_test.go +++ b/pkg/workflow/permissions_scope_validation_test.go @@ -91,3 +91,15 @@ pull-requests: read`, }) } } + +func TestGetAllPermissionScopeNamesReturnsCopy(t *testing.T) { + scopes := getAllPermissionScopeNames() + require.NotEmpty(t, scopes) + + original := scopes[0] + scopes[0] = "mutated-scope" + + fresh := getAllPermissionScopeNames() + require.NotEmpty(t, fresh) + require.Equal(t, original, fresh[0]) +} diff --git a/pkg/workflow/permissions_validation.go b/pkg/workflow/permissions_validation.go index 6bbc1c9c8ed..9b312d8a71c 100644 --- a/pkg/workflow/permissions_validation.go +++ b/pkg/workflow/permissions_validation.go @@ -8,6 +8,7 @@ import ( "slices" "sort" "strings" + "sync" "github.com/github/gh-aw/pkg/constants" "github.com/github/gh-aw/pkg/setutil" @@ -16,6 +17,37 @@ import ( "github.com/goccy/go-yaml" ) +var allPermissionScopeNames = sync.OnceValue(func() []string { + ghTokenScopes := GetAllPermissionScopes() + appOnlyScopes := GetAllGitHubAppOnlyScopes() + // +1 for copilot-requests which is not in GetAllPermissionScopes + all := make([]string, 0, safeAllocationCapacity(len(ghTokenScopes), len(appOnlyScopes), 1)) + for _, scope := range ghTokenScopes { + all = append(all, string(scope)) + } + for _, scope := range appOnlyScopes { + all = append(all, string(scope)) + } + // copilot-requests is valid even though not in GetAllPermissionScopes + all = append(all, string(PermissionCopilotRequests)) + return all +}) + +// getAllPermissionScopeNames returns all valid permission scope names for fuzzy matching. +// The values are computed once and cached across calls. +func getAllPermissionScopeNames() []string { + return slices.Clone(allPermissionScopeNames()) +} + +// validPermissionMetaKeys is the set of meta-keys accepted in permissions shorthand contexts. +// Defined once at package level to avoid per-call map allocation. +var validPermissionMetaKeys = map[string]struct{}{ + "all": {}, + "read-all": {}, + "write-all": {}, + "none": {}, +} + // PermissionsValidationResult contains the result of permissions validation type PermissionsValidationResult struct { MissingPermissions map[PermissionScope]PermissionLevel // Permissions required but not granted @@ -408,28 +440,6 @@ func ValidatePermissionScopeNames(permissionsYAML string) error { permissionsValidationLog.Print("Validating permission scope names") - // Collect all valid scope names for fuzzy matching - ghTokenScopes := GetAllPermissionScopes() - appOnlyScopes := GetAllGitHubAppOnlyScopes() - // +1 for copilot-requests which is not in GetAllPermissionScopes - allScopes := make([]string, 0, safeAllocationCapacity(len(ghTokenScopes), len(appOnlyScopes), 1)) - for _, scope := range ghTokenScopes { - allScopes = append(allScopes, string(scope)) - } - for _, scope := range appOnlyScopes { - allScopes = append(allScopes, string(scope)) - } - // copilot-requests is valid even though not in GetAllPermissionScopes - allScopes = append(allScopes, string(PermissionCopilotRequests)) - // "all" is a meta-key that is always valid in shorthand contexts - validMeta := map[string]struct { - }{ - "all": {}, - "read-all": {}, - "write-all": {}, - "none": {}, - } - // Strip optional "permissions:" prefix so we can parse just the map content content := strings.TrimSpace(permissionsYAML) if strings.HasPrefix(content, "permissions:") { @@ -450,14 +460,17 @@ func ValidatePermissionScopeNames(permissionsYAML string) error { } for scopeKey := range permsMap { - if setutil.Contains(validMeta, scopeKey) { + if setutil.Contains(validPermissionMetaKeys, scopeKey) { continue } if _, ok := validPermissionScopes[scopeKey]; ok { continue } - // Unknown scope key — check for a case-only difference first (e.g. "Contents" → "contents") + // Unknown scope key — retrieve all valid scopes lazily (only on the error path) + allScopes := getAllPermissionScopeNames() + + // Check for a case-only difference first (e.g. "Contents" → "contents") lowerScopeKey := strings.ToLower(scopeKey) if lowerScopeKey != scopeKey { if _, ok := validPermissionScopes[lowerScopeKey]; ok {