Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
rate-limit to user-rate-limit and migrate max-runs to max-runs-per-window
|
@copilot make sure the codemod only updates the rate-limit, not other max-runs |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in |
There was a problem hiding this comment.
Pull request overview
Renames the rate-limiting frontmatter configuration from rate-limit to user-rate-limit and migrates the nested max key to max-runs-per-window, while keeping rate-limit as a documented legacy alias. The PR also adds a CLI codemod and updates schema/docs/autocomplete/test fixtures to match the new naming.
Changes:
- Update compiler frontmatter extraction and experimental warning messaging to prefer
user-rate-limitwhile still accepting legacyrate-limit. - Add a
rate-limit→user-rate-limitcodemod (including targeted tests) that migratesmax-runs/max→max-runs-per-windowonly within the rate-limit block. - Update JSON schema, docs, autocomplete assets, and workflow/examples to use
user-rate-limit+max-runs-per-window(and documentrate-limitas legacy).
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/role_checks.go | Prefer user-rate-limit, accept legacy key, and migrate internal parsing to max-runs-per-window. |
| pkg/workflow/rate_limit_experimental_warning_test.go | Update integration test workflows and expected warning text to user-rate-limit. |
| pkg/workflow/frontmatter_types.go | Rename JSON tags to user-rate-limit and max-runs-per-window. |
| pkg/workflow/compiler_validators.go | Update experimental warning text to reference user-rate-limit. |
| pkg/parser/schemas/main_workflow_schema.json | Add user-rate-limit schema and document rate-limit as legacy alias; migrate required max key. |
| pkg/cli/workflows/test-rate-limit-ignored-roles.md | Update CLI test workflow to new frontmatter keys. |
| pkg/cli/workflows/test-rate-limit-defaults.md | Update CLI test workflow to new frontmatter keys. |
| pkg/cli/fix_codemods.go | Register new rate-limit codemod in the fix command codemod registry. |
| pkg/cli/fix_codemods_test.go | Assert new codemod is present and ordered correctly. |
| pkg/cli/codemod_user_rate_limit.go | New codemod to rename rate-limit → user-rate-limit and migrate max key within the block. |
| pkg/cli/codemod_user_rate_limit_test.go | Unit tests for the new codemod, including non-overreach cases. |
| docs/src/content/docs/reference/rate-limiting-controls.md | Update reference docs to new key names. |
| docs/src/content/docs/reference/frontmatter-full.md | Update full frontmatter reference snippet to new key names. |
| docs/src/content/docs/reference/cost-management.md | Update examples and references to the new rate-limit naming. |
| docs/src/content/docs/guides/maintaining-repos.md | Update guide examples to the new rate-limit naming. |
| docs/scripts/generate-autocomplete-data.js | Update autocomplete root ordering to prefer user-rate-limit. |
| docs/public/editor/autocomplete-data.json | Refresh editor autocomplete data to include user-rate-limit and legacy rate-limit (plus other generated updates). |
| .github/workflows/workflow-generator.md | Update repo workflow frontmatter to new keys. |
| .github/workflows/auto-triage-issues.md | Update repo workflow frontmatter to new keys. |
| .github/workflows/ai-moderator.md | Update repo workflow frontmatter to new keys. |
| .github/aw/syntax.md | Update syntax docs to reference user-rate-limit and max-runs-per-window. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
pkg/workflow/role_checks.go:249
- The schema allows
user-rate-limit.max-runs-per-windowto be a GitHub Actions expression string, butextractRateLimitConfigonly handles numeric types and silently ignores string values. That means configs likemax-runs-per-window: ${{ inputs.max }}will fall back to the default (5) at runtime instead of using the expression. Consider either (1) supporting string values by carrying a raw string through to the generatedGH_AW_RATE_LIMIT_MAXenv var, or (2) tightening the schema/docs to disallow expressions if they aren’t actually supported.
// Extract max-runs-per-window (default: 5)
maxValue, ok := v["max-runs-per-window"]
if !ok {
maxValue, ok = v["max-runs"]
}
if !ok {
maxValue, ok = v["max"] // legacy compatibility
}
if ok {
switch max := maxValue.(type) {
case int:
config.Max = max
case int64:
config.Max = int(max)
case uint64:
config.Max = int(max)
case float64:
config.Max = int(max)
}
- Files reviewed: 21/21 changed files
- Comments generated: 2
| // Emit experimental warning for user-rate-limit feature | ||
| if workflowData.RateLimit != nil { | ||
| fmt.Fprintln(os.Stderr, console.FormatWarningMessage("Using experimental feature: rate-limit")) | ||
| fmt.Fprintln(os.Stderr, console.FormatWarningMessage("Using experimental feature: user-rate-limit")) | ||
| c.IncrementWarningCount() |
| max-runs: 5 | ||
| user-rate-limit: | ||
| max-runs-per-window: 5 | ||
| window: 1h |
Generated by the Design Decision Gate workflow. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Commit pushed:
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (>100 new lines in AI has analyzed the PR diff and generated a draft ADR to help you get started: 📄 Draft ADR: The draft has been committed directly to this PR branch. What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. Why this PR triggered the gate
Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs in this repo are stored in
References: §25642880879
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — this PR is a rename/migration refactor with a backward-compat layer and associated codemod, so test coverage quality and behavior correctness are the primary lens.
Key Themes
- Silent legacy path:
rate-limitis accepted at compile time but users are only notified via a debug log, not a visible deprecation warning. The codemod (gh aw fix) handles it, but compile-time silence means authors who don't runfixwon't know. - Undocumented intermediate key:
max-runsis accepted as an alias inextractRateLimitConfig, but it doesn't appear in the schema, docs, or codemod. Its provenance is unclear. - Backward-compat gap in tests: The existing test file was updated to
user-rate-limitthroughout; no test exercises therate-limit→extractRateLimitConfigcompat path.
Positive Highlights
- ✅ Excellent codemod implementation — the
findAndReplaceInLineprefix-matching approach is safe and well-scoped - ✅
DoesNotRenameOtherMaxRunsandDoesNotRenameNestedMaxRunstests show disciplined edge-case thinking - ✅ Schema, docs, autocomplete data, and syntax guide are all updated consistently
- ✅ Codemod is properly registered in
fix_codemods.goand covered infix_codemods_test.go - ✅ Skips ambiguous documents (both old and new keys present) — good defensive default
Verdict
This is a well-executed rename with good tooling support. The three inline comments flag the main gaps — particularly the lack of a user-visible deprecation warning for the silent legacy path, which is the most important one to address before merge.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · ● 9.3M
| roleLog.Print("No rate-limit configuration specified") | ||
| roleLog.Print("No user-rate-limit configuration specified") | ||
| return nil | ||
| } |
There was a problem hiding this comment.
[/tdd] The legacy rate-limit key path is detected but only logged at debug level — no user-facing deprecation warning is emitted. Users with old configs won't know they need to migrate, and CI won't surface it.
Consider emitting a console warning when the legacy key is used:
if legacyKey {
fmt.Fprintln(os.Stderr, console.FormatWarningMessage(
"Deprecated: 'rate-limit' is renamed to 'user-rate-limit'. Run 'gh aw fix' to migrate automatically."))
c.IncrementWarningCount()
}This matches how other deprecations (e.g. needs.activation.outputs.*) are surfaced to authors.
| if !ok { | ||
| maxValue, ok = v["max"] // legacy compatibility | ||
| } | ||
| if ok { |
There was a problem hiding this comment.
[/tdd] The intermediate max-runs key is silently accepted as a third alias (alongside max and max-runs-per-window), but it isn't documented anywhere — not in the schema, syntax guide, or docs. This creates a hidden third valid value that can confuse future maintainers and makes the migration story harder to reason about.
If max-runs was never a public key, remove this branch. If it was, add it to the schema as a deprecated alias and add a test case for it.
| @@ -22,12 +22,12 @@ func TestRateLimitExperimentalWarning(t *testing.T) { | |||
| expectWarning bool | |||
There was a problem hiding this comment.
[/tdd] The test suite was updated to use user-rate-limit throughout, but the backward-compat path (accepting the old rate-limit key at compile time) has no test coverage. A regression here would be silent.
Add a test case:
{
name: "legacy rate-limit key still compiles without error",
content: `---
on: workflow_dispatch
engine: copilot
rate-limit:
max: 5
window: 60
permissions:
contents: read
---
`,
expectWarning: true,
},This verifies the backward-compat branch in extractRateLimitConfig and provides a regression harness for future cleanup.
🧪 Test Quality Sentinel ReportTest Quality Score: 90/100✅ Excellent test quality
Test Classification DetailsView all 7 test classifications
Modifications to existing tests:
Flagged Tests — Requires Review
|
| Component | Weight | Score |
|---|---|---|
| Behavioral coverage (6/7 = 86%) | 40 pts | 34 |
| Error/edge case coverage (6/7 = 86%) | 30 pts | 26 |
| Duplication penalty (0 clusters) | 20 pts | 20 |
| Test inflation (1.67:1 ratio, under 2:1) | 10 pts | 10 |
| Total | 100 pts | 90 |
Verdict
✅ Check passed. 14% of new tests are implementation tests (threshold: 30%). No mock libraries, no missing build tags. Minor guideline note: add assertion messages to the bare
assert.*calls.
📖 Understanding Test Classifications
Design Tests (High Value) verify what the system does:
- Assert on observable outputs, return values, or state changes
- Cover error paths and boundary conditions
- Would catch a behavioral regression if deleted
- Remain valid even after internal refactoring
Implementation Tests (Low Value) verify how the system does it:
- Assert on internal function calls or struct metadata
- Only test the happy path with typical inputs
- Break during legitimate refactoring even when behavior is correct
- Give false assurance: they pass even when the system is wrong
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.
References: §25642880880
🧪 Test quality analysis by Test Quality Sentinel · ● 5.8M · ◷
There was a problem hiding this comment.
✅ Test Quality Sentinel: 90/100. Test quality is excellent — 14% of new tests are implementation tests (threshold: 30%). No mock libraries, no missing build tags. Minor note: add descriptive assertion messages to bare assert.* calls for better failure diagnostics.
|
@copilot review all comments |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in I addressed the remaining review threads: updated the experimental warning to key-agnostic wording ( |
Renames rate-limit frontmatter to
user-rate-limitand migrates nested max keys tomax-runs-per-window, with codemod support and compatibility for legacy configs.Changes Made
rate-limit→user-rate-limitmax-runs/max→max-runs-per-window(scoped to direct children of the rate-limit block only)max-runskeys are not rewritten.user-rate-limitnaming while keeping legacy parsing compatibility.rate limiting) to avoid confusion when legacyrate-limitis used.window: 60).Testing
make golint,go build ./cmd/gh-aw)