Skip to content

perf(validation): remove YAML parse hot path in permission scope validation #34797

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.

Sign up for GitHub

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

Closed
Copilot wants to merge 4 commits into from
+229 −50
Closed

perf(validation): remove YAML parse hot path in permission scope validation #34797

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4694f8f
Initial plan
Copilot May 25, 2026
aa6d9bd
perf: speed up permission scope-name validation
Copilot May 25, 2026
56918ed
Merge branch 'main' into copilot/fix-performance-regression-validation
pelikhan May 26, 2026
c958179
docs(adr): add draft ADR-34797 for permission scope validation fast path
github-actions[bot] May 26, 2026
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
11 changes: 11 additions & 0 deletions pkg/workflow/permissions_scope_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,17 @@ pull-requests: read`,
issues: write`,
wantErr: false,
},
{
name: "quoted scope keys are valid",
yaml: `"contents": read
"pull-requests": write`,
wantErr: false,
},
{
name: "flow mapping shorthand remains valid",
yaml: `{contents: read, issues: write}`,
wantErr: false,
},
{
name: "typo in scope name suggests correction",
yaml: `contnts: read`,
Expand Down
177 changes: 127 additions & 50 deletions pkg/workflow/permissions_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,20 @@ import (
"github.com/goccy/go-yaml"
)

var (
permissionScopeValidationAllScopes = buildPermissionScopeValidationAllScopes()
permissionScopeValidationMeta = map[string]struct{}{
"all": {},
"read-all": {},
"write-all": {},
"none": {},
}
permissionScopeValidationValidScopesPreview = strings.Join(
permissionScopeValidationAllScopes[:min(10, len(permissionScopeValidationAllScopes))],
", ",
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot May 26, 2026

Choose a reason for hiding this comment

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

"..." is unconditionally appended to the scopes preview even when all scopes fit within the 10-item cap.

💡 Details and fix

If buildPermissionScopeValidationAllScopes() ever returns ≤ 10 entries — possible in tests or if the scope list is trimmed — the error message claims there are more valid scopes than shown, which is misleading.

// Fix:
if len(permissionScopeValidationAllScopes) > 10 {
    permissionScopeValidationValidScopesPreview = strings.Join(
        permissionScopeValidationAllScopes[:10], ", ",
    ) + "..."
} else {
    permissionScopeValidationValidScopesPreview = strings.Join(
        permissionScopeValidationAllScopes, ", ",
    )
}

) + "..."
)

// PermissionsValidationResult contains the result of permissions validation
type PermissionsValidationResult struct {
MissingPermissions map[PermissionScope]PermissionLevel // Permissions required but not granted
Expand Down Expand Up @@ -402,27 +416,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]bool{
"all": true,
"read-all": true,
"write-all": true,
"none": true,
}

// Strip optional "permissions:" prefix so we can parse just the map content
content := strings.TrimSpace(permissionsYAML)
if strings.HasPrefix(content, "permissions:") {
Expand All @@ -435,50 +428,134 @@ func ValidatePermissionScopeNames(permissionsYAML string) error {
}
}

// Try to parse the content as a YAML map of scope → level
scopeKeys, parsedFast := extractTopLevelPermissionScopeKeys(content)
if parsedFast {
for _, scopeKey := range scopeKeys {
if err := validateSinglePermissionScopeKey(scopeKey); err != nil {
return err
}
}
return nil
}

// Fallback to YAML map parsing for non-standard formats (flow mappings, etc.).
var permsMap map[string]any
if err := yaml.Unmarshal([]byte(content), &permsMap); err != nil {
// Not a map (e.g., a shorthand like "read-all"); nothing to validate
return nil
}

for scopeKey := range permsMap {
if validMeta[scopeKey] {
continue
if err := validateSinglePermissionScopeKey(scopeKey); err != nil {
return err
}
}

return nil
}

func validateSinglePermissionScopeKey(scopeKey string) error {
if _, ok := permissionScopeValidationMeta[scopeKey]; ok {
return nil
}
if _, ok := validPermissionScopes[scopeKey]; ok {
return nil
}

// Unknown scope key — check for a case-only difference first (e.g. "Contents" → "contents")
lowerScopeKey := strings.ToLower(scopeKey)
if lowerScopeKey != scopeKey {
if _, ok := validPermissionScopes[lowerScopeKey]; ok {
return fmt.Errorf(
"unknown permission scope %q.\n\nDid you mean: %s?\n\nValid permission scopes include: %s\n\nSee: %s",
scopeKey,
lowerScopeKey,
permissionScopeValidationValidScopesPreview,
constants.DocsPermissionsURL,
)
}
if _, ok := validPermissionScopes[scopeKey]; ok {
}

// Check for a close fuzzy match
permissionsValidationLog.Printf("Unknown permission scope key: %q", scopeKey)
suggestions := stringutil.FindClosestMatches(scopeKey, permissionScopeValidationAllScopes, 3)
if len(suggestions) == 0 {
return nil // too different to be a typo, ignore silently
}

return fmt.Errorf(
"unknown permission scope %q.\n\nDid you mean: %s?\n\nValid permission scopes include: %s\n\nSee: %s",
scopeKey,
strings.Join(suggestions, ", "),
permissionScopeValidationValidScopesPreview,
constants.DocsPermissionsURL,
)
}

func extractTopLevelPermissionScopeKeys(content string) ([]string, bool) {
if content == "" {
return nil, false
}
// Flow-style mappings ("{contents: read}") and sequence values are uncommon here.
// Let the YAML decoder handle those to preserve behavior.
if strings.Contains(content, "{") || strings.Contains(content, "}") || strings.Contains(content, "[") || strings.Contains(content, "]") {
return nil, false
}

lines := strings.Split(content, "\n")
keys := make([]string, 0, 4)
baseIndent := -1

for _, line := range lines {
trimmed := strings.TrimSpace(line)
if trimmed == "" || strings.HasPrefix(trimmed, "#") {
continue
}
colonIdx := strings.IndexByte(trimmed, ':')
if colonIdx <= 0 {
return nil, false
}

// Unknown scope key — check for a case-only difference first (e.g. "Contents" → "contents")
lowerScopeKey := strings.ToLower(scopeKey)
if lowerScopeKey != scopeKey {
if _, ok := validPermissionScopes[lowerScopeKey]; ok {
return fmt.Errorf(
"unknown permission scope %q.\n\nDid you mean: %s?\n\nValid permission scopes include: %s\n\nSee: %s",
scopeKey,
lowerScopeKey,
strings.Join(allScopes[:min(10, len(allScopes))], ", ")+"...",
constants.DocsPermissionsURL,
)
}
indent := countLeadingSpacesAndTabs(line)
if baseIndent == -1 {
baseIndent = indent
}
if indent != baseIndent {
// Nested block content (value continuation), not a top-level scope key.
continue
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot May 26, 2026

Choose a reason for hiding this comment

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

Fast path silently skips deeper-indented key-value lines instead of falling back, diverging from YAML semantics.

💡 Details and fix

When indent != baseIndent, the code does continue. This is fine for plain scalar continuations, but if a deeper-indented line is itself a key: value pair (e.g. from an accidentally-indented permission or a nested map), the fast path silently drops it and returns parsedFast=true. The YAML fallback — which would surface the nested key or return a parse error — never runs.

Example: contents: read\n badscope: write is invalid YAML (the YAML parser would error → ValidatePermissionScopeNames returns nil). The fast path returns ["contents"] and parsedFast=true, silently accepting the document.

Fix: when a deeper-indented line contains a colon, bail out to the YAML parser rather than skipping:

if indent != baseIndent {
    if colonIdx > 0 {
        // Looks like a nested map key — hand off to YAML parser.
        return nil, false
    }
    continue // plain scalar value continuation, safe to skip
}

}

// Check for a close fuzzy match
permissionsValidationLog.Printf("Unknown permission scope key: %q", scopeKey)
suggestions := stringutil.FindClosestMatches(scopeKey, allScopes, 3)
if len(suggestions) == 0 {
continue // too different to be a typo, ignore silently
key := strings.TrimSpace(trimmed[:colonIdx])
if key == "" || strings.HasPrefix(key, "-") {
return nil, false
}
key = strings.Trim(key, `"'`)
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot May 26, 2026

Choose a reason for hiding this comment

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

strings.Trim strips all leading/trailing quote characters rather than one balanced pair, silently normalising malformed keys.

💡 Details and fix

strings.Trim(key, "\"'") removes every leading and trailing " or ' character. A malformed key like ""contents"" becomes contents and validates successfully. A key like "write-all (unbalanced quote, meaning a workflow authoring error) loses its leading quote and is validated as write-all — a valid meta key — masking the typo.

The intent is to unquote YAML keys that were written with explicit quotes, but only a single balanced pair should be removed:

if len(key) >= 2 {
    q := key[0]
    if (q == '"' || q == '\'' ) && key[len(key)-1] == q {
        key = key[1 : len(key)-1]
    }
}

If the quotes are unbalanced, leaving the key unchanged (or returning nil, false) lets the YAML fallback handle it correctly.


return fmt.Errorf(
"unknown permission scope %q.\n\nDid you mean: %s?\n\nValid permission scopes include: %s\n\nSee: %s",
scopeKey,
strings.Join(suggestions, ", "),
strings.Join(allScopes[:min(10, len(allScopes))], ", ")+"...",
constants.DocsPermissionsURL,
)
keys = append(keys, key)
}

return nil
return keys, len(keys) > 0
}

func countLeadingSpacesAndTabs(s string) int {
i := 0
for i < len(s) && (s[i] == ' ' || s[i] == '\t') {
i++
}
return i
}

func buildPermissionScopeValidationAllScopes() []string {
ghTokenScopes := GetAllPermissionScopes()
appOnlyScopes := GetAllGitHubAppOnlyScopes()
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))
return allScopes
}
Loading