Fix user-rate-limit alias schema and event fallback#56328
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot remove legacy alias in favor of codemod |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Ponytail Reviewer completed successfully!
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
❌ Design Decision Gate 🏗️ failed to deliver outputs during design decision gate check.
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict
No blocking changed-line defects stood up on this pass.
Highlights
- The schema/runtime/docs now consistently describe the deprecated
user-rate-limitmax aliases. - The omitted-
eventsfallback is intentionally broader now, which closes the previous under-application gap whenon:had no supported programmatic triggers. - I did not find a correctness, crash, or security regression in the touched lines that justifies blocking merge.
🔎 Code quality review by PR Code Quality Reviewer · pi · gpt54 · 14.5 AIC · ⌖ 6.88 AIC · ⊞ 4.6K
Comment /review to run again
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
I’d trim the extra helper indirection in the rate-limit config and trigger-safety paths; it doesn’t buy much and can be collapsed back into the surrounding logic. net: -18 lines possible.
Generated by ✂️ Ponytail Reviewer for #56328 · codex · mai10 · 7.59 AIC · ⌖ 1.56 AIC · ⊞ 16.7K
Comment /ponytail to run again
| break | ||
| } | ||
| } | ||
| func hasOnlySafeOnMapEvents(onMap map[string]any, roles []string) bool { |
There was a problem hiding this comment.
pkg/workflow/role_checks.go:397: yagni: helper split for a simple event scan. Keep the logic inline in hasOnlySafeOnMapEvents and delete isRoleCheckConfigKey/countWorkflowEvents.
| if len(config.Events) > 0 { | ||
| roleLog.Printf("Inferred events from workflow triggers: %v", config.Events) | ||
| } | ||
| func extractRateLimitInt(config map[string]any, keys ...string) int { |
There was a problem hiding this comment.
pkg/workflow/role_checks.go:259: yagni: tiny type-switch helpers for one config struct. Inline the few branches back into extractRateLimitConfig and drop the extra helpers.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on one correctness bug.
📋 Key Themes & Highlights
Key Themes
-
Silent zero for expression aliases (correctness bug):
extractRateLimitInthas nocase stringarm. A workflow usingmax: "${{ inputs.n }}"compiles without error but enforcesmax=0, silently disabling rate limiting. The schema test confirms the schema accepts the expression, but the runtime discards it. See inline comment onrole_checks.go:269. -
Missing regression test: No
role_checks_test.gocase exercises the expression-string path for deprecated aliases; the schema test alone is insufficient. The gap means the silent-zero bug has no automated signal. See inline comment onrole_checks_test.go:340. -
Fragile cross-language sync: The
// Keep in synccomment is the only guard between the Go and JS event lists. A small automated test or a generated source of truth would make future drift impossible to miss.
Positive Highlights
- ✅ Clean extraction of
extractRateLimitInt,extractRateLimitEvents,extractRateLimitIgnoredRoles— the refactor is easier to read and test than the original nested switch. - ✅
hasOnlySafeOnMapEvents+countWorkflowEvents+isRoleCheckConfigKeyare good deep-module improvements: simple interfaces, clearly named, easy to unit-test. - ✅
rateLimitProgrammaticEventssingle constant shared across bothinferEventsFromTriggerscall sites eliminates the old duplicate map. - ✅ Schema
oneOfenforcement of mutually-exclusive alias fields is the right choice; the new schema tests cover the ambiguous-alias edge cases well.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet46 · 39.1 AIC · ⌖ 11.5 AIC · ⊞ 7.6K
Comment /matt to run again
| return int(typedValue) | ||
| case uint64: | ||
| return int(typedValue) | ||
| case float64: |
There was a problem hiding this comment.
[/diagnosing-bugs] extractRateLimitInt silently returns 0 for GitHub Actions expression strings — this can disable rate limiting without any error.
The schema and the new "legacy max-runs expression alias" test case both permit "${{ inputs.max_runs }}" as a valid value, but no case string arm exists in the type switch, so config.Max becomes 0 at runtime.
💡 Root cause and fix
Option A – Propagate the expression string into a new RateLimitConfig.MaxExpr string field and emit it in the generated step YAML alongside Max int.
Option B – Reject non-numeric values with a compile-time validation error, and remove the string pattern from alias properties (max-runs, max), keeping it only on max-runs-per-window where the existing pipeline already handles expressions.
Either way, the current gap means max: "${{ inputs.n }}" compiles successfully but enforces max=0, silently bypassing rate limiting with no warning to the user.
Minimal guard until a full fix lands:
case string:
roleLog.Printf("WARNING: expression string %q for %q is not supported at compile time; rate limit will be skipped", typedValue, keys[0])@copilot please address this.
| assert.Equal(t, 2, cfg.Max) | ||
| } | ||
| }) | ||
|
|
There was a problem hiding this comment.
[/tdd] No Go-level test verifies that an expression string (e.g. "${{ inputs.max }}") for a deprecated alias produces the correct compiled output — the schema test only validates schema acceptance.
The schema_test.go case "legacy max-runs expression alias" confirms the schema does not reject the value, but there is no corresponding role_checks_test.go case that asserts what extractRateLimitConfig does with it. As a result the silent max=0 regression (see the other comment) has no test to catch it.
💡 Suggested test
t.Run("expression string for max-runs alias is handled", func(t *testing.T) {
cfg := c.extractRateLimitConfig(map[string]any{
"user-rate-limit": map[string]any{
"max-runs": "${{ inputs.max_runs }}",
},
})
// Assert that the expression is preserved and Max is not silently 0
if assert.NotNil(t, cfg) {
assert.NotEqual(t, 0, cfg.Max, "expression string must not silently resolve to 0")
}
})This test would currently fail — which is the /tdd signal that the implementation is incomplete.
@copilot please address this.
| @@ -15,6 +15,19 @@ import ( | |||
|
|
|||
| var roleLog = logger.New("workflow:role_checks") | |||
There was a problem hiding this comment.
[/diagnosing-bugs] The // Keep in sync comment is the only safeguard between the JS and Go event lists — there is no automated enforcement.
PROGRAMMATIC_EVENTS in check_rate_limit.cjs and rateLimitProgrammaticEvents in role_checks.go must remain identical. A developer adding or renaming an event in Go will not be reminded to update the JS constant (and vice versa), and the divergence will go undetected until a rate-limit regression occurs in production.
💡 Suggested safeguard
Add a Go test that reads the JS file and asserts the two lists are identical, e.g.:
func TestRateLimitEventsMatchJS(t *testing.T) {
raw, err := os.ReadFile("../../actions/setup/js/check_rate_limit.cjs")
require.NoError(t, err)
// parse out the PROGRAMMATIC_EVENTS array and compare to rateLimitProgrammaticEvents
...
}Alternatively, generate both from a single source of truth (a shared JSON or YAML file that both Go and JS import at build time).
@copilot please address this.
There was a problem hiding this comment.
Review: Fix user-rate-limit alias schema and event fallback
The changes are well-structured and correct. The refactoring of extractRateLimitConfig into focused helper functions (extractRateLimitInt, extractRateLimitEvents, extractRateLimitIgnoredRoles) significantly improves readability and testability. The oneOf schema constraint correctly enforces that exactly one max alias is present, and the test coverage for the new aliases is thorough.
One non-blocking issue found: extractRateLimitInt silently returns 0 for string values (GitHub Actions expressions), which means max-runs: "${{ inputs.max }}" falls back silently to the default. A warning log would make misconfiguration more visible.
The event fallback behavior change (returning all programmatic events instead of nil when no recognized triggers are found) is safe and correctly aligns the Go compiler with the JS check_rate_limit.cjs behavior.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet46 · 45.7 AIC · ⌖ 9.21 AIC · ⊞ 6.2K
| case float64: | ||
| return int(typedValue) | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing string case silently discards GitHub Actions expressions
extractRateLimitInt handles int, int64, uint64, and float64 but silently returns 0 for string values. The JSON schema explicitly allows "${{ inputs.max }}" expressions for all three max aliases (max-runs-per-window, max-runs, and max). When a user writes:
user-rate-limit:
max-runs: "${{ inputs.max_runs }}"the expression is silently dropped and Max falls back to the compile-time default (5), with no warning. This was a pre-existing gap, but now that max-runs and max are officially documented aliases, more users will hit this path.
The expression string should either be passed through to the env var template, or at minimum a warning should be logged when a string value is silently ignored:
case string:
roleLog.Printf("extractRateLimitInt: ignoring string value %q for key %s — expression pass-through not yet supported", typedValue, key)@copilot please address this.
There was a problem hiding this comment.
Pull request overview
Aligns user rate-limit aliases and fallback event behavior across compiler, runtime, schema, tests, and documentation.
Changes:
- Adds broad fallback for omitted events.
- Synchronizes supported programmatic event lists.
- Attempts to address legacy max aliases and documentation.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/role_checks.go |
Refactors rate-limit parsing and event inference. |
pkg/workflow/role_checks_test.go |
Tests parsing and fallback behavior. |
pkg/parser/schemas/main_workflow_schema.json |
Clarifies event fallback semantics. |
pkg/parser/schema_test.go |
Adds max-field validation cases. |
actions/setup/js/check_rate_limit.cjs |
Expands default programmatic events. |
actions/setup/js/check_rate_limit.test.cjs |
Tests pull-request rate limiting. |
docs/src/content/docs/reference/rate-limiting-controls.md |
Documents event fallback. |
docs/src/content/docs/reference/frontmatter.md |
Updates rate-limit guidance. |
docs/src/content/docs/reference/frontmatter-full.md |
Updates generated reference text. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Suppressed comments (1)
pkg/workflow/role_checks_test.go:333
- The legacy
maxalias is also part of the compatibility contract, so this test should assert extraction of2rather than codifying the silent fallback to the default limit.
t.Run("ignores legacy max alias", func(t *testing.T) {
cfg := c.extractRateLimitConfig(map[string]any{
"user-rate-limit": map[string]any{
"max": 2,
},
- Files reviewed: 9/9 changed files
- Comments generated: 4
- Review effort level: Balanced
| name: "legacy max-runs alias", | ||
| rateLimit: map[string]any{"max-runs": 5}, | ||
| wantErr: true, | ||
| errContains: "Unknown property: max-runs", |
| Users with any of the `ignored-roles` are not rate limited. The default exemptions are `admin`, `maintain`, and `write`; set `ignored-roles: []` to rate limit every user, including administrators. | ||
|
|
||
| Legacy frontmatter that used a top-level `rate-limit:` section, or `max:`/`max-runs:` instead of `max-runs-per-window:`, is migrated automatically by `gh aw fix`. | ||
| Legacy frontmatter that used a top-level `rate-limit:` section, or `max:`/`max-runs:` instead of `max-runs-per-window:`, must be migrated with `gh aw fix`. |
| } | ||
| } | ||
| config := &RateLimitConfig{ | ||
| Max: extractRateLimitInt(rateLimitMap, "max-runs-per-window"), |
| t.Run("ignores legacy max-runs alias", func(t *testing.T) { | ||
| cfg := c.extractRateLimitConfig(map[string]any{ | ||
| "user-rate-limit": map[string]any{ | ||
| "max-runs": 4, | ||
| }, | ||
| }) | ||
|
|
||
| if assert.NotNil(t, cfg) { | ||
| assert.Zero(t, cfg.Max) | ||
| } | ||
| }) |
user-rate-limitaccepted legacy max aliases in the parser while rejecting them in the JSON schema. Its omitted-eventsfallback could also under-apply rate limiting whenon:lacked recognized programmatic triggers.Schema alignment
max-runs-per-window,max-runs, ormax.max-runsandmaxas deprecated aliases.Event fallback semantics
eventsis omitted and no supported trigger can be inferred.Docs/tests
Example: