Skip to content

feat: add rate limiter for API calls - #4

Closed
benaiad wants to merge 2 commits into
mainfrom
test/strict-reviewer
Closed

feat: add rate limiter for API calls#4
benaiad wants to merge 2 commits into
mainfrom
test/strict-reviewer

Conversation

@benaiad

@benaiad benaiad commented Apr 27, 2026

Copy link
Copy Markdown
Owner

Adds a sliding-window rate limiter class and a header parser for rate limit responses.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The PR adds a sliding-window rate limiter and a header parser in a single new file. There is one critical off-by-one correctness bug in tryAcquire that allows maxRequests + 1 requests per window, and a warning-level defect in parseRateLimit where parseInt on a missing value produces NaN that silently fails the truthiness guard. A few additional info-level concerns include bare any types, no NaN bounds-checking on parsed integers, negative windowMs accepted by reset, and no unit tests for the new code. Overall risk is medium — the core limiter logic is subtly incorrect, and the parser has fragile validation.

6 finding(s): 1 error, 1 warning, 4 info

[ERROR] correctness

File: src/utils/rate-limit.ts:20

Off-by-one: the condition this.timestamps.length <= this.config.maxRequests allows maxRequests+1 requests through. After filtering expired entries, if there are exactly maxRequests active timestamps, the check still passes and one more is pushed, exceeding the configured limit.

Suggestion: Change <= to <: if (this.timestamps.length < this.config.maxRequests) { ... }


[WARNING] error-handling

File: src/utils/rate-limit.ts:61

If a header part lacks = (e.g. malformed input), part.split("=") returns a single-element array, making val undefined. parseInt(undefined) yields NaN. The final if (maxRequests && windowMs) truthiness check rejects NaN (and also incorrectly rejects legitimate zero values), returning null without any indication of malformed input.

Suggestion: Validate the parse result explicitly: const parsed = parseInt(val, 10); if (isNaN(parsed)) continue; and use maxRequests !== null && windowMs !== null for the final guard (type maxRequests and windowMs as number | null instead of any).


[INFO] type-discipline

File: src/utils/rate-limit.ts:57

maxRequests and windowMs are typed as any, losing type safety. If the NaN path were later refactored, any would hide type errors.

Suggestion: Declare as let maxRequests: number | null = null; let windowMs: number | null = null;


[INFO] correctness

File: src/utils/rate-limit.ts:63

parseInt(val) without a radix parameter relies on implicit base-10 detection, which can produce surprising results for inputs with leading zeros or 0x prefixes.

Suggestion: Pass radix 10: parseInt(val, 10) and parseInt(val, 10) * 1000.


[INFO] correctness

File: src/utils/rate-limit.ts:50

reset(windowMs?) accepts any number including negative or zero values for windowMs, which would break the limiter's windowing logic (all timestamps would be considered expired or never expired).

Suggestion: Add a guard: if (windowMs !== undefined && windowMs > 0) { this.config.windowMs = windowMs; } or throw on invalid values.


[INFO] testing

File: src/utils/rate-limit.ts

No unit tests accompany the new rate limiter module. Given the off-by-one bug found, test coverage would have caught it.

Suggestion: Add tests covering: exact limit enforcement, window expiration, remainingRequests accuracy, waitForSlot behavior, and parseRateLimit for valid, invalid, and edge-case inputs.


Reviewed by pi-relay · View full run

Comment thread src/utils/rate-limit.ts
tryAcquire(): boolean {
const now = Date.now();
this.timestamps = this.timestamps.filter((t) => now - t < this.config.windowMs);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[ERROR] correctness

Off-by-one: the condition this.timestamps.length <= this.config.maxRequests allows maxRequests+1 requests through. After filtering expired entries, if there are exactly maxRequests active timestamps, the check still passes and one more is pushed, exceeding the configured limit.

Suggestion: Change <= to <: if (this.timestamps.length < this.config.maxRequests) { ... }

Comment thread src/utils/rate-limit.ts Outdated
case "limit":
maxRequests = parseInt(val);
break;
case "window":

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[WARNING] error-handling

If a header part lacks = (e.g. malformed input), part.split("=") returns a single-element array, making val undefined. parseInt(undefined) yields NaN. The final if (maxRequests && windowMs) truthiness check rejects NaN (and also incorrectly rejects legitimate zero values), returning null without any indication of malformed input.

Suggestion: Validate the parse result explicitly: const parsed = parseInt(val, 10); if (isNaN(parsed)) continue; and use maxRequests !== null && windowMs !== null for the final guard (type maxRequests and windowMs as number | null instead of any).

Comment thread src/utils/rate-limit.ts Outdated

for (const part of parts) {
const [key, val] = part.split("=");
switch (key.trim()) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[INFO] type-discipline

maxRequests and windowMs are typed as any, losing type safety. If the NaN path were later refactored, any would hide type errors.

Suggestion: Declare as let maxRequests: number | null = null; let windowMs: number | null = null;

Comment thread src/utils/rate-limit.ts
break;
case "window":
windowMs = parseInt(val) * 1000;
break;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[INFO] correctness

parseInt(val) without a radix parameter relies on implicit base-10 detection, which can produce surprising results for inputs with leading zeros or 0x prefixes.

Suggestion: Pass radix 10: parseInt(val, 10) and parseInt(val, 10) * 1000.

Comment thread src/utils/rate-limit.ts
}
}

export function parseRateLimit(header: string): RateLimitConfig | null {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[INFO] correctness

reset(windowMs?) accepts any number including negative or zero values for windowMs, which would break the limiter's windowing logic (all timestamps would be considered expired or never expired).

Suggestion: Add a guard: if (windowMs !== undefined && windowMs > 0) { this.config.windowMs = windowMs; } or throw on invalid values.

@github-actions

Copy link
Copy Markdown

AI Review: Fixes Applied

Reviewed and found 6 issue(s). All addressed.

Changes Made

Fixed 2 findings in src/utils/rate-limit.ts:

  1. [error] Off-by-one in tryAcquire() (line 20): Changed <= to < in the length check so that exactly maxRequests entries blocks further acquisitions instead of allowing maxRequests+1.

  2. [warning] Malformed header parsing in parseRateLimit() (line 61):

    • Changed from part.split("=") destructuring to part.indexOf("=") with explicit eqIdx === -1 guard, avoiding undefined val when input lacks =.
    • Typed maxRequests and windowMs as number | null instead of any.
    • Added explicit Number.isNaN() validation after parseInt(val, 10).
    • Changed final guard from truthiness check (maxRequests && windowMs) to explicit null check (maxRequests !== null && windowMs !== null), which correctly handles legitimate zero values.
    • Used radix 10 in parseInt calls (also addressed the related info finding).

Info findings (no tests, reset() validation) were not addressed as instructed.

Verification passed.


View full run

@github-actions
github-actions Bot dismissed their stale review April 27, 2026 06:25

Superseded by new review.

@benaiad

benaiad commented Apr 27, 2026

Copy link
Copy Markdown
Owner Author

Test PR — verified updated reviewer prompt catches real defects and dismisses stale reviews after fixes. Closing.

@benaiad benaiad closed this Apr 27, 2026
@benaiad
benaiad deleted the test/strict-reviewer branch April 27, 2026 06:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant