fix: deduplicate-by-title rate-limit guard always skips due to wrong bucket threshold - #48092
Conversation
The TITLE_DEDUP_MIN_SEARCH_RATE_LIMIT_REMAINING = 500 threshold was compared against resources.search.remaining, which has a max of 30/min. This made the guard always trigger, silently skipping dedup on every run. Replace the absolute constant with TITLE_DEDUP_MIN_SEARCH_RATE_LIMIT_FRACTION = 0.2 and compare remaining/limit proportionally. Also read both remaining and limit from the search resource so the check stays correct on GHES where admins can tune ceilings. Fixes #48070 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #48092 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
There was a problem hiding this comment.
Pull request overview
Fixes title deduplication by using 20% of the search API bucket limit instead of an impossible fixed threshold.
Changes:
- Calculates the guard from the reported search limit.
- Updates rate-limit mocks to realistic values.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/create_issue.cjs |
Implements proportional rate-limit guarding. |
actions/setup/js/create_issue.test.cjs |
Updates rate-limit test fixtures. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Medium
| limit: 30, | ||
| remaining: 1, |
There was a problem hiding this comment.
Updated in c1ef37a: the low-rate test now uses a non-default bucket (limit: 35, remaining: 7) so it verifies proportional threshold behavior and catches hard-coded cutoffs.
| if (!Number.isFinite(remaining) || !Number.isFinite(limit)) { | ||
| core.warning(`Could not determine search rate limit remaining for ${owner}/${repo}; proceeding with repo-level title dedup search`); |
There was a problem hiding this comment.
Updated in c1ef37a: the warning now reports both parsed values by including remaining and limit in the message when either value is invalid/missing.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix is correct and well-scoped. The old constant 500 exceeded the search bucket ceiling of 30 req/min, making the guard always fire and silently disabling dedup. The fractional threshold (0.2 x limit) is proportional, portable to GHES, and both fields are validated. Tests updated correctly.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 15.3 AIC · ⌖ 7.31 AIC · ⊞ 5K
🧪 Test Quality Sentinel Report✅ Test Quality Score: N/A — No Behavioral Tests
📊 Analysis Summary
What changed:
Verdict
Quality Signal: Changes demonstrate deliberate fixture maintenance — test data was updated to align with the production code refactoring (threshold logic change from fixed
|
There was a problem hiding this comment.
Non-blocking: test coverage gaps for the new fractional threshold
The fix itself is correct — replacing the hardcoded 500 with a proportional 20% fraction properly reflects the actual search bucket ceiling. The code logic is sound.
Findings
Missing boundary tests (noted inline): neither the <= threshold edge nor the new !Number.isFinite(limit) code path has a dedicated test case. The only "skip" test uses remaining: 1, limit: 30 (~3%), which is far from the boundary, and the "proceed" test uses 100% — no test touches near 20%.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 · 27.1 AIC · ⌖ 4.61 AIC · ⊞ 5.7K
Comment /review to run again
| @@ -46,7 +46,8 @@ describe("create_issue", () => { | |||
| data: { | |||
There was a problem hiding this comment.
Missing boundary tests for the 20% threshold: the updated mock uses remaining: 30, limit: 30 (100% quota), which never exercises the guard logic introduced by this PR.
💡 Suggested additions
Add tests for:
- Exactly at threshold (
remaining: 6, limit: 30→ 20% → should skip) - Just above threshold (
remaining: 7, limit: 30→ ~23% → should NOT skip) limitis undefined/missing → new!Number.isFinite(limit)branch returnsfalsewith warning — completely untested
Without these, the <= semantics and the limit-NaN guard are exercised by neither the old nor the new tests.
There was a problem hiding this comment.
Addressed in c1ef37a: added coverage for boundary/guard behavior with a just-above-threshold case (35/8) and a missing-limit case that proceeds with warning and repo-level search.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — approving with one minor suggestion on boundary test coverage.
📋 Key Themes & Highlights
Root-cause fix
The bug is correctly diagnosed and fixed at the source: the hardcoded 500 constant was never a valid threshold for a 30 req/min bucket, so every invocation silently skipped dedup. The proportional approach (0.2 * limit) is semantically correct and GHES-portable.
Positive Highlights
- ✅ Both
remainingandlimitare now validated withNumber.isFinite, guarding against partial API responses - ✅ Warning message now shows
remaining/limitratio, which will make future diagnostics much easier - ✅ Test default mock updated to realistic values (
limit: 30, remaining: 30) — this was the right move - ✅ PR description clearly explains the before/after with a code snippet
One improvement
The skip test validates remaining: 1 (deep below threshold) but there are no boundary tests for exactly-at-threshold and just-above-threshold values. See inline comment for a concrete suggestion.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 29.6 AIC · ⌖ 4.73 AIC · ⊞ 6.7K
Comment /matt to run again
| resources: { | ||
| search: { | ||
| limit: 30, | ||
| remaining: 1, |
There was a problem hiding this comment.
[/tdd] The skip test uses remaining: 1 (far below the threshold) but there is no boundary test for exactly-at-threshold or just-above-threshold values, which is where off-by-one errors in <= guards typically hide.
💡 Suggested boundary tests
Add two cases:
it('skips when remaining equals 20% of limit', async () => {
// 6/30 = 0.2 exactly — guard uses <=, so this should skip
mockRateLimit({ limit: 30, remaining: 6 });
expect(await shouldSkipRepoTitleDedupSearch(...)).toBe(true);
});
it('proceeds when remaining is one above threshold', async () => {
// 7/30 > 0.2 — should NOT skip
mockRateLimit({ limit: 30, remaining: 7 });
expect(await shouldSkipRepoTitleDedupSearch(...)).toBe(false);
});This pins the <= semantics and catches any future threshold change that accidentally flips the boundary.
@copilot please address this.
There was a problem hiding this comment.
Addressed in c1ef37a: added a boundary-focused case above the threshold (limit: 35, remaining: 8) and updated the low-rate case to non-default bucket values to pin threshold semantics.
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Completed a pr-finisher pass. Addressed in-scope review feedback in commit c1ef37a, ran local validation ( |
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in e4c29d0. I added explicit boundary coverage for the unresolved github-actions thread ( |
|
@copilot please run the Unresolved review feedback:
|
I reran a pr-finisher pass. The listed unresolved review threads are already addressed by earlier commits ( I also reran local checks in this pass: CI on the current head remains unverified/stale from the cloud-agent context and needs a maintainer-triggered rerun. |
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot please run the Unresolved review feedback:
|
I reran I reran checks in this pass:
I also reviewed the referenced run ( CI on the current HEAD remains stale/unverified from cloud-agent pushes and needs a maintainer-triggered rerun. |
🤖 PR Triage — §30192186368
Why: Fixes #48070 — Next: Fast-track merge — CI green, low risk.
|
|
@copilot please run the Unresolved review feedback:
|
I reran a pr-finisher pass and rechecked those threads. The Copilot and github-actions review items listed are already satisfied in the branch ( I reran checks locally in this pass:
I also reviewed recent workflow runs/logs via MCP ( |
|
🎉 This pull request is included in a new release. Release: |
deduplicate-by-titlewas silently inert on every invocation. The rate-limit guard comparedresources.search.remainingagainst a hardcoded threshold of500, but the search bucket's ceiling is 30 req/min — so the check always short-circuited and dedup was skipped, even with a fully unused quota.Changes
create_issue.cjs: Replace the absolute constantTITLE_DEDUP_MIN_SEARCH_RATE_LIMIT_REMAINING = 500with a fractional thresholdTITLE_DEDUP_MIN_SEARCH_RATE_LIMIT_FRACTION = 0.2. The guard now reads bothremainingandlimitfromresources.searchand skips only when the remaining budget is below 20% of that bucket's own ceiling:This is also correct on GHES where instance admins can tune the search bucket ceiling.
create_issue.test.cjs: Update the defaultrateLimit.getmock to includelimit: 30, remaining: 30(realistic search bucket values), and update the "skip when low" test case to supply both fields.run: https://github.com/github/gh-aw/actions/runs/30187302401