-
Notifications
You must be signed in to change notification settings - Fork 141
Add ignored-roles field to rate-limit configuration with defaults #15025
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
Conversation
- Updated RateLimitConfig struct to include IgnoredRoles field - Updated JSON schema validation to support ignored-roles - Implemented role check in check_rate_limit.cjs to skip rate limiting for users with ignored roles - Added comprehensive tests for the new functionality - Updated documentation with examples and troubleshooting guide - Created test workflow demonstrating role-based exemptions Co-authored-by: pelikhan <[email protected]>
Co-authored-by: pelikhan <[email protected]>
|
@copilot default is admin, maintain, write |
Default behavior now exempts users with admin, maintain, or write permissions from rate limiting. Only triage and read users are subject to rate limiting by default. Users can override this by explicitly setting ignored-roles in frontmatter. Co-authored-by: pelikhan <[email protected]>
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.
@copilot move to pkg/cli/workflows
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.
Moved to pkg/cli/workflows/
Commit: fc0c190
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.
@copilot move to pkg/cli/workflows
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.
Moved to pkg/cli/workflows/
Commit: fc0c190
docs/RATE_LIMITING.md
Outdated
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.
@copilot delete file
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.
Deleted
Commit: fc0c190
…G.md - Moved test-rate-limit-defaults.md to pkg/cli/workflows/ - Moved test-rate-limit-ignored-roles.md to pkg/cli/workflows/ - Deleted docs/RATE_LIMITING.md (documentation exists elsewhere) - Lock files for test workflows are not tracked per .gitignore Co-authored-by: pelikhan <[email protected]>
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.
Pull request overview
Adds support for rate-limit.ignored-roles in workflow frontmatter so certain repository permission levels can bypass rate limiting (with intended defaults of admin, maintain, write), spanning compiler extraction, schema/docs, and the JS runtime check.
Changes:
- Extend rate-limit config type/schema/docs with
ignored-rolesand document default behavior. - Parse
ignored-rolesin Go and pass it to the pre-activation rate limit check via env var. - Update JS rate-limit check + tests, and add example/test workflows.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/role_checks.go | Extracts ignored-roles and threads it into generated workflow env vars. |
| pkg/workflow/frontmatter_types.go | Adds IgnoredRoles to the typed frontmatter config struct. |
| pkg/parser/schemas/main_workflow_schema.json | Validates ignored-roles values via enum and documents defaults. |
| actions/setup/js/check_rate_limit.cjs | Implements ignored-role exemption using repo permission lookup. |
| actions/setup/js/check_rate_limit.test.cjs | Adds unit tests for default/custom ignored-role behavior. |
| pkg/cli/workflows/test-rate-limit-ignored-roles.md | Adds an example workflow demonstrating ignored-roles. |
| pkg/cli/workflows/test-rate-limit-defaults.md | Adds an example workflow demonstrating default ignored roles. |
| docs/src/content/docs/reference/frontmatter-full.md | Documents the new ignored-roles field in the full frontmatter reference. |
| docs/src/content/docs/agent-factory-status.mdx | Adds a listing entry for the new test workflow. |
| docs/RATE_LIMITING.md | Removes the legacy rate limiting doc file. |
Comments suppressed due to low confidence (1)
actions/setup/js/check_rate_limit.cjs:47
ignoredRolesList.split(",")will produce[""]when the env var is an empty string. If you implement support forignored-roles: [], also normalize the parsed roles by filtering out empty strings so the log output andincludescheck behave as expected.
// Check if user has an ignored role (exempt from rate limiting)
const ignoredRoles = ignoredRolesList.split(",").map(r => r.trim());
core.info(` Ignored roles: ${ignoredRoles.join(", ")}`);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| | [Test Create PR Error Handling](https://github.com/github/gh-aw/blob/main/.github/workflows/test-create-pr-error-handling.md) | claude | [](https://github.com/github/gh-aw/actions/workflows/test-create-pr-error-handling.lock.yml) | - | - | | ||
| | [Test Dispatcher Workflow](https://github.com/github/gh-aw/blob/main/.github/workflows/test-dispatcher.md) | copilot | [](https://github.com/github/gh-aw/actions/workflows/test-dispatcher.lock.yml) | - | - | | ||
| | [Test Project URL Explicit Requirement](https://github.com/github/gh-aw/blob/main/.github/workflows/test-project-url-default.md) | copilot | [](https://github.com/github/gh-aw/actions/workflows/test-project-url-default.lock.yml) | - | - | | ||
| | [Test Rate Limiting with Ignored Roles](https://github.com/github/gh-aw/blob/main/.github/workflows/test-rate-limit-ignored-roles.md) | copilot | [](https://github.com/github/gh-aw/actions/workflows/test-rate-limit-ignored-roles.lock.yml) | - | - | |
Copilot
AI
Feb 11, 2026
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.
This table row links to .github/workflows/test-rate-limit-ignored-roles.md, but the workflow file in this PR is under pkg/cli/workflows/. As-is, the link (and likely the badge target) will be broken. Update the URL to the correct path, or omit this entry if pkg/cli/workflows files aren’t intended to be published as runnable Actions workflows.
| | [Test Rate Limiting with Ignored Roles](https://github.com/github/gh-aw/blob/main/.github/workflows/test-rate-limit-ignored-roles.md) | copilot | [](https://github.com/github/gh-aw/actions/workflows/test-rate-limit-ignored-roles.lock.yml) | - | - | | |
| | [Test Rate Limiting with Ignored Roles](https://github.com/github/gh-aw/blob/main/pkg/cli/workflows/test-rate-limit-ignored-roles.md) | copilot | [](https://github.com/github/gh-aw/actions/workflows/test-rate-limit-ignored-roles.lock.yml) | - | - | |
| Max int `json:"max,omitempty"` // Maximum number of runs allowed per time window (default: 5) | ||
| Window int `json:"window,omitempty"` // Time window in minutes (default: 60) | ||
| Events []string `json:"events,omitempty"` // Event types to apply rate limiting to (e.g., ["workflow_dispatch", "issue_comment"]) | ||
| IgnoredRoles []string `json:"ignored-roles,omitempty"` // Roles that are exempt from rate limiting (e.g., ["admin", "maintainer"]) |
Copilot
AI
Feb 11, 2026
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.
The example role value "maintainer" isn’t one of GitHub’s permission levels (GitHub uses maintain). Using an invalid role name here is misleading since the schema/enforcement only accepts maintain. Please update the comment example to use maintain.
| IgnoredRoles []string `json:"ignored-roles,omitempty"` // Roles that are exempt from rate limiting (e.g., ["admin", "maintainer"]) | |
| IgnoredRoles []string `json:"ignored-roles,omitempty"` // Roles that are exempt from rate limiting (e.g., ["admin", "maintain"]) |
| - Limits non-admin/non-maintainer users to 3 runs within a 30-minute window | ||
| - Exempts users with "admin" or "maintain" roles from rate limiting | ||
| - Applies to workflow_dispatch and issue_comment events | ||
|
|
||
| ## Testing | ||
|
|
||
| ### For Admin/Maintainer Users: |
Copilot
AI
Feb 11, 2026
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.
This doc uses “maintainer” terminology (e.g. “non-admin/non-maintainer”, “Admin/Maintainer Users”), but the actual GitHub permission level is maintain. Please update the wording to match the real role name to avoid confusion when testing/configuring ignored-roles.
| - Limits non-admin/non-maintainer users to 3 runs within a 30-minute window | |
| - Exempts users with "admin" or "maintain" roles from rate limiting | |
| - Applies to workflow_dispatch and issue_comment events | |
| ## Testing | |
| ### For Admin/Maintainer Users: | |
| - Limits non-admin/non-maintain users to 3 runs within a 30-minute window | |
| - Exempts users with "admin" or "maintain" roles from rate limiting | |
| - Applies to workflow_dispatch and issue_comment events | |
| ## Testing | |
| ### For Admin/Maintain Users: |
|
|
||
| # Optional list of roles that are exempt from rate limiting. Users with any of | ||
| # these roles will not be subject to rate limiting checks. Common roles: 'admin', | ||
| # 'maintain', 'write', 'triage', 'read'. |
Copilot
AI
Feb 11, 2026
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.
This reference snippet shows ignored-roles: [] without mentioning the default behavior when the field is omitted (defaults to admin/maintain/write) vs. when it is explicitly set to an empty array (no exemptions). As written, it’s easy to infer that the default is an empty list. Consider adding a short note here clarifying omit-vs-empty semantics.
| # 'maintain', 'write', 'triage', 'read'. | |
| # 'maintain', 'write', 'triage', 'read'. If this field is omitted, 'admin', | |
| # 'maintain', and 'write' are exempt by default. Set this to an empty array [] | |
| # to apply rate limiting to all roles with no exemptions. |
| // Set ignored roles (if specified) | ||
| if len(data.RateLimit.IgnoredRoles) > 0 { | ||
| // Sort roles alphabetically for consistent output | ||
| ignoredRoles := make([]string, len(data.RateLimit.IgnoredRoles)) | ||
| copy(ignoredRoles, data.RateLimit.IgnoredRoles) | ||
| sort.Strings(ignoredRoles) | ||
| steps = append(steps, fmt.Sprintf(" GH_AW_RATE_LIMIT_IGNORED_ROLES: %q\n", strings.Join(ignoredRoles, ","))) | ||
| } |
Copilot
AI
Feb 11, 2026
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.
ignored-roles: [] is intended to exempt no one, but this generator only sets GH_AW_RATE_LIMIT_IGNORED_ROLES when the slice is non-empty. That means an explicitly empty list won’t be passed to the runtime check, so the JS script will fall back to its defaults and still exempt admin/maintain/write. Consider always emitting GH_AW_RATE_LIMIT_IGNORED_ROLES (even as an empty string) when rate limiting is configured, so an explicit empty list can be honored.
| // Set ignored roles (if specified) | |
| if len(data.RateLimit.IgnoredRoles) > 0 { | |
| // Sort roles alphabetically for consistent output | |
| ignoredRoles := make([]string, len(data.RateLimit.IgnoredRoles)) | |
| copy(ignoredRoles, data.RateLimit.IgnoredRoles) | |
| sort.Strings(ignoredRoles) | |
| steps = append(steps, fmt.Sprintf(" GH_AW_RATE_LIMIT_IGNORED_ROLES: %q\n", strings.Join(ignoredRoles, ","))) | |
| } | |
| // Set ignored roles (always emit, even when empty, so an explicit empty list is honored) | |
| ignoredRolesValue := "" | |
| if len(data.RateLimit.IgnoredRoles) > 0 { | |
| // Sort roles alphabetically for consistent output | |
| ignoredRoles := make([]string, len(data.RateLimit.IgnoredRoles)) | |
| copy(ignoredRoles, data.RateLimit.IgnoredRoles) | |
| sort.Strings(ignoredRoles) | |
| ignoredRolesValue = strings.Join(ignoredRoles, ",") | |
| } | |
| steps = append(steps, fmt.Sprintf(" GH_AW_RATE_LIMIT_IGNORED_ROLES: %q\n", ignoredRolesValue)) |
| const windowMinutes = parseInt(process.env.GH_AW_RATE_LIMIT_WINDOW || "60", 10); | ||
| const eventsList = process.env.GH_AW_RATE_LIMIT_EVENTS || ""; | ||
| // Default: admin, maintain, and write roles are exempt from rate limiting | ||
| const ignoredRolesList = process.env.GH_AW_RATE_LIMIT_IGNORED_ROLES || "admin,maintain,write"; |
Copilot
AI
Feb 11, 2026
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.
Using process.env.GH_AW_RATE_LIMIT_IGNORED_ROLES || "admin,maintain,write" makes an explicitly empty env var (used to represent ignored-roles: []) fall back to the defaults, preventing users from overriding to “no ignored roles”. Use a nullish check (??) instead so "" is preserved.
This issue also appears on line 45 of the same file.
| const ignoredRolesList = process.env.GH_AW_RATE_LIMIT_IGNORED_ROLES || "admin,maintain,write"; | |
| const ignoredRolesList = process.env.GH_AW_RATE_LIMIT_IGNORED_ROLES ?? "admin,maintain,write"; |
| it("should use default ignored roles (admin, maintain, write) when not specified", async () => { | ||
| // Don't set GH_AW_RATE_LIMIT_IGNORED_ROLES, so it uses default | ||
|
|
Copilot
AI
Feb 11, 2026
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.
There’s coverage for default ignored roles and custom lists, but no test for the explicit empty override (e.g. GH_AW_RATE_LIMIT_IGNORED_ROLES=""), which should result in no exemptions (admin should go through the rate-limit path). Add a test to lock in the intended ignored-roles: [] semantics.
Implementation Complete: Add "ignored-roles" field to rate-limiting ✅
Summary
Successfully implemented the
ignored-rolesfield for the rate-limiting frontmatter configuration with default values of admin, maintain, write.Changes Implemented
IgnoredRoles []stringfield toRateLimitConfigstruct with default valuesextractRateLimitConfigto set defaults when not specifiedgenerateRateLimitCheckto pass ignored roles as env varDefault Behavior
When
ignored-rolesis not specified:To override defaults:
Validation Results
Example: Default Behavior
Result: Only triage and read users are rate limited to 5 runs per hour.
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.