fix(client): handle HTTP-date format in Retry-After header - #27
Conversation
parseInt returns NaN for HTTP-date values like "Wed, 21 Oct 2025 07:28:00 GMT", which propagates into RateLimitError.retryAfter. Extract parseRetryAfter() that handles both numeric seconds and HTTP-date, falling back to 60s for missing or unparseable values. Ref: #18 (item 3)
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (2)test/**/*.test.ts📄 CodeRabbit inference engine (AGENTS.md)
Files:
test/unit/**/*.test.ts📄 CodeRabbit inference engine (test/AGENTS.md)
Files:
🧠 Learnings (12)📚 Learning: 2026-03-10T07:36:54.862ZApplied to files:
📚 Learning: 2026-03-10T07:36:54.862ZApplied to files:
📚 Learning: 2026-03-10T07:36:03.586ZApplied to files:
📚 Learning: 2026-03-10T07:36:54.862ZApplied to files:
📚 Learning: 2026-03-10T07:36:54.862ZApplied to files:
📚 Learning: 2026-03-10T07:36:03.586ZApplied to files:
📚 Learning: 2026-03-10T07:36:54.862ZApplied to files:
📚 Learning: 2026-03-10T07:36:12.605ZApplied to files:
📚 Learning: 2026-03-10T07:36:54.862ZApplied to files:
📚 Learning: 2026-03-10T07:36:12.605ZApplied to files:
📚 Learning: 2026-03-10T07:36:54.862ZApplied to files:
📚 Learning: 2026-03-10T07:36:54.862ZApplied to files:
🧬 Code graph analysis (1)test/unit/client.test.ts (1)
🔇 Additional comments (1)
📝 WalkthroughWalkthroughAdded exported helper parseRetryAfter(header) to centralize Retry-After parsing (numeric seconds and HTTP-date). Production 429 handling now calls this helper. Unit tests added covering numeric, date, and malformed inputs; fallback is 60 seconds, past dates yield 0. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3fbf7f240e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 2 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Requires human review: This change modifies error handling logic in a core path (the HTTP client). Per the instructions, business logic changes in core paths require human review.
Architecture diagram
sequenceDiagram
participant App as Consumer Application
participant Client as Client.getJSON()
participant API as External Registry API
participant Parser as NEW: parseRetryAfter()
App->>Client: getJSON(url)
Client->>API: fetch()
alt HTTP 200 OK
API-->>Client: JSON Response
Client-->>App: Data
else HTTP 429 Too Many Requests
API-->>Client: 429 Response + "Retry-After" header
Note over Client,Parser: Extract header value (numeric or date string)
Client->>Parser: CHANGED: parseRetryAfter(headerValue)
alt Input is Numeric (e.g., "120")
Parser->>Parser: parseInt(value, 10)
else NEW: Input is HTTP-date (RFC 7231)
Parser->>Parser: Date.parse(value)
Parser->>Parser: Calculate delta: (Target - Date.now())
else Input Invalid or Empty
Parser->>Parser: Fallback to 60s
end
Parser-->>Client: seconds (number)
Note over Client: Instantiate error with parsed seconds
Client-->>App: throw RateLimitError(retryAfter)
end
Note over App,Parser: Ensures RateLimitError.retryAfter is never NaN<ctrl63>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/client.ts`:
- Around line 23-24: The current parse uses Number.parseInt(header, 10) which
accepts partial strings like "120s"; update the logic so you first validate
header with a full-string digit regexp (e.g. /^\d+$/) before calling
Number.parseInt, and only return the parsed value if the regex matches and the
resulting numeric is >= 0; locate the occurrence of Number.parseInt and the
variables header/numeric in src/core/client.ts and apply this strict-match check
there.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d3f69bdd-aeee-440c-b941-3eeb60b3ed99
📒 Files selected for processing (2)
src/core/client.tstest/unit/client.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🧰 Additional context used
📓 Path-based instructions (5)
src/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.ts: Keep TypeScript imports with.tsextension style in source files
Do not parse PURLs outsidesrc/core/purl.ts; callcreateFromPURLorparsePURLinstead
Do not duplicate retry/backoff constants outsidesrc/core/client.ts; centralize all retry logic
Do not hardcode cache TTL in random modules; useDEFAULT_TTLfromsrc/cache/lockfile.ts
Respect Node.js runtime floor of>=22.6.0and modern syntax assumptionsUse
.tsimport suffixes consistently
Files:
src/core/client.ts
src/core/client.ts
📄 CodeRabbit inference engine (src/AGENTS.md)
Implement all fetch and retry behavior only in
src/core/client.tsClient owns network behavior defaults (maxRetries, timeout, retry codes)
Files:
src/core/client.ts
src/core/**/*.ts
📄 CodeRabbit inference engine (src/core/AGENTS.md)
src/core/**/*.ts: Throw typed errors (InvalidPURLError, NotFoundError, RateLimitError) instead of plain Error in core flows
VersionStatus and Scope are closed unions; keep adapter outputs inside allowed values
Files:
src/core/client.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use Vitest globals with
test/unitandtest/e2esplit for testing
test/**/*.test.ts: Test files should use*.test.tsnaming convention
Vitest globals (describe,it,expect,vi) are enabled and should be used without imports in test files
Files:
test/unit/client.test.ts
test/unit/**/*.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
Unit tests should rely on mocks/spies rather than external dependencies
Files:
test/unit/client.test.ts
🧠 Learnings (7)
📚 Learning: 2026-03-10T07:36:03.586Z
Learnt from: CR
Repo: oritwoen/regxa PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-10T07:36:03.586Z
Learning: Applies to src/**/*.ts : Do not duplicate retry/backoff constants outside `src/core/client.ts`; centralize all retry logic
Applied to files:
src/core/client.tstest/unit/client.test.ts
📚 Learning: 2026-03-10T07:36:12.605Z
Learnt from: CR
Repo: oritwoen/regxa PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-03-10T07:36:12.605Z
Learning: Applies to src/core/client.ts : Implement all fetch and retry behavior only in `src/core/client.ts`
Applied to files:
src/core/client.tstest/unit/client.test.ts
📚 Learning: 2026-03-10T07:36:38.679Z
Learnt from: CR
Repo: oritwoen/regxa PR: 0
File: src/core/AGENTS.md:0-0
Timestamp: 2026-03-10T07:36:38.679Z
Learning: Applies to src/core/client.ts : Client owns network behavior defaults (maxRetries, timeout, retry codes)
Applied to files:
src/core/client.ts
📚 Learning: 2026-03-10T07:36:12.605Z
Learnt from: CR
Repo: oritwoen/regxa PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-03-10T07:36:12.605Z
Learning: Applies to src/**/!(client).ts : Do not implement fetch/retry behavior outside `src/core/client.ts`
Applied to files:
src/core/client.ts
📚 Learning: 2026-03-10T07:36:54.862Z
Learnt from: CR
Repo: oritwoen/regxa PR: 0
File: test/AGENTS.md:0-0
Timestamp: 2026-03-10T07:36:54.862Z
Learning: Applies to test/unit/purl.test.ts : PURL contract tests should be located in `test/unit/purl.test.ts` for parse/build validation
Applied to files:
test/unit/client.test.ts
📚 Learning: 2026-03-10T07:36:54.862Z
Learnt from: CR
Repo: oritwoen/regxa PR: 0
File: test/AGENTS.md:0-0
Timestamp: 2026-03-10T07:36:54.862Z
Learning: Applies to test/unit/{license,repository}.test.ts : Normalization tests should be located in `test/unit/license.test.ts` and `test/unit/repository.test.ts` for canonical output normalization
Applied to files:
test/unit/client.test.ts
📚 Learning: 2026-03-10T07:36:54.862Z
Learnt from: CR
Repo: oritwoen/regxa PR: 0
File: test/AGENTS.md:0-0
Timestamp: 2026-03-10T07:36:54.862Z
Learning: Applies to test/unit/{lockfile,cached-registry}.test.ts : Cache behavior tests should be located in `test/unit/lockfile.test.ts` and `test/unit/cached-registry.test.ts` for freshness, TTL, integrity, and wrapper behavior validation
Applied to files:
test/unit/client.test.ts
🧬 Code graph analysis (2)
src/core/client.ts (1)
src/core/errors.ts (1)
RateLimitError(51-59)
test/unit/client.test.ts (1)
src/core/client.ts (1)
parseRetryAfter(20-33)
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/unit/client.test.ts`:
- Around line 39-41: Add a unit test that verifies parseRetryAfter correctly
handles numeric strings with leading zeros (e.g., "0120") to document
RFC-compliant behavior; create a new it block (or extend the existing "handles
large numeric values" case) named something like "handles numeric values with
leading zeros" and assert parseRetryAfter("0120") === 120 to ensure leading-zero
inputs are parsed as decimal seconds.
- Around line 24-29: The test "parses HTTP-date in the future" is
timing-dependent and can flake; make it deterministic by either mocking time or
loosening the assertion: use parseRetryAfter with a fixed future timestamp by
stubbing Date.now() (or using vi.useFakeTimers()) so future = new
Date(Date.now() + 90_000) is stable, or change the upper bound assertion on
result produced by parseRetryAfter to a slightly larger value (e.g., allow >0
and <= 95) to tolerate small pauses; update the test referencing parseRetryAfter
and the "parses HTTP-date in the future" spec accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8c0972db-2a28-47a2-8275-0f9c7a528883
📒 Files selected for processing (2)
src/core/client.tstest/unit/client.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
src/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.ts: Keep TypeScript imports with.tsextension style in source files
Do not parse PURLs outsidesrc/core/purl.ts; callcreateFromPURLorparsePURLinstead
Do not duplicate retry/backoff constants outsidesrc/core/client.ts; centralize all retry logic
Do not hardcode cache TTL in random modules; useDEFAULT_TTLfromsrc/cache/lockfile.ts
Respect Node.js runtime floor of>=22.6.0and modern syntax assumptionsUse
.tsimport suffixes consistently
Files:
src/core/client.ts
src/core/client.ts
📄 CodeRabbit inference engine (src/AGENTS.md)
Implement all fetch and retry behavior only in
src/core/client.tsClient owns network behavior defaults (maxRetries, timeout, retry codes)
Files:
src/core/client.ts
src/core/**/*.ts
📄 CodeRabbit inference engine (src/core/AGENTS.md)
src/core/**/*.ts: Throw typed errors (InvalidPURLError, NotFoundError, RateLimitError) instead of plain Error in core flows
VersionStatus and Scope are closed unions; keep adapter outputs inside allowed values
Files:
src/core/client.ts
test/**/*.test.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use Vitest globals with
test/unitandtest/e2esplit for testing
test/**/*.test.ts: Test files should use*.test.tsnaming convention
Vitest globals (describe,it,expect,vi) are enabled and should be used without imports in test files
Files:
test/unit/client.test.ts
test/unit/**/*.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
Unit tests should rely on mocks/spies rather than external dependencies
Files:
test/unit/client.test.ts
🧠 Learnings (7)
📚 Learning: 2026-03-10T07:36:03.586Z
Learnt from: CR
Repo: oritwoen/regxa PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-10T07:36:03.586Z
Learning: Applies to src/**/*.ts : Do not duplicate retry/backoff constants outside `src/core/client.ts`; centralize all retry logic
Applied to files:
src/core/client.tstest/unit/client.test.ts
📚 Learning: 2026-03-10T07:36:12.605Z
Learnt from: CR
Repo: oritwoen/regxa PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-03-10T07:36:12.605Z
Learning: Applies to src/core/client.ts : Implement all fetch and retry behavior only in `src/core/client.ts`
Applied to files:
src/core/client.tstest/unit/client.test.ts
📚 Learning: 2026-03-10T07:36:38.679Z
Learnt from: CR
Repo: oritwoen/regxa PR: 0
File: src/core/AGENTS.md:0-0
Timestamp: 2026-03-10T07:36:38.679Z
Learning: Applies to src/core/client.ts : Client owns network behavior defaults (maxRetries, timeout, retry codes)
Applied to files:
src/core/client.ts
📚 Learning: 2026-03-10T07:36:12.605Z
Learnt from: CR
Repo: oritwoen/regxa PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-03-10T07:36:12.605Z
Learning: Applies to src/**/!(client).ts : Do not implement fetch/retry behavior outside `src/core/client.ts`
Applied to files:
src/core/client.ts
📚 Learning: 2026-03-10T07:36:54.862Z
Learnt from: CR
Repo: oritwoen/regxa PR: 0
File: test/AGENTS.md:0-0
Timestamp: 2026-03-10T07:36:54.862Z
Learning: Applies to test/unit/purl.test.ts : PURL contract tests should be located in `test/unit/purl.test.ts` for parse/build validation
Applied to files:
test/unit/client.test.ts
📚 Learning: 2026-03-10T07:36:54.862Z
Learnt from: CR
Repo: oritwoen/regxa PR: 0
File: test/AGENTS.md:0-0
Timestamp: 2026-03-10T07:36:54.862Z
Learning: Applies to test/unit/{license,repository}.test.ts : Normalization tests should be located in `test/unit/license.test.ts` and `test/unit/repository.test.ts` for canonical output normalization
Applied to files:
test/unit/client.test.ts
📚 Learning: 2026-03-10T07:36:54.862Z
Learnt from: CR
Repo: oritwoen/regxa PR: 0
File: test/AGENTS.md:0-0
Timestamp: 2026-03-10T07:36:54.862Z
Learning: Applies to test/unit/{lockfile,cached-registry}.test.ts : Cache behavior tests should be located in `test/unit/lockfile.test.ts` and `test/unit/cached-registry.test.ts` for freshness, TTL, integrity, and wrapper behavior validation
Applied to files:
test/unit/client.test.ts
🧬 Code graph analysis (2)
src/core/client.ts (1)
src/core/errors.ts (1)
RateLimitError(51-59)
test/unit/client.test.ts (1)
src/core/client.ts (1)
parseRetryAfter(20-34)
🔇 Additional comments (3)
src/core/client.ts (2)
20-34: Solid implementation addressing the RFC 7231 edge cases.The strict
/^\d+$/check properly rejects partial numerics like"120s"and decimals like"1.5"thatparseIntwould have accepted. The letter-check guard beforeDate.parseis a nice optimization to skip date parsing for purely numeric-like garbage.One small note:
Math.max(seconds, 0)returns 0 for past dates, which matches test expectations but differs from the function's documented "60 when unparseable" behavior. Past dates are technically parseable, just not useful. The current behavior (returning 0) is probably more correct since the server meant "retry now" - just flagging the doc mismatch.
84-84: Clean integration with RateLimitError.Delegates parsing to the new utility. Looking at
RateLimitErrorinsrc/core/errors.ts, it expects a number and builds the message from it - this works correctly now thatparseRetryAfteralways returns a valid number (no more NaN propagation).test/unit/client.test.ts (1)
1-54: Good coverage of the main parsing scenarios.Tests hit all the important cases from the PR description: numeric, HTTP-date, garbage, and the previously-broken partial numeric inputs.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/unit/client.test.ts`:
- Around line 24-29: The test "parses HTTP-date in the future" uses
vi.useFakeTimers() and vi.setSystemTime(...) but calls vi.useRealTimers()
unconditionally at the end; wrap the fake-timer setup and assertions in a
try/finally so vi.useRealTimers() always runs even if parseRetryAfter(...) or
expect(...) throws. Locate the test block that calls vi.useFakeTimers(),
vi.setSystemTime(new Date(...)), parseRetryAfter(...), and vi.useRealTimers()
and move the assertion into the try with vi.useRealTimers() in the finally to
guarantee cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 871c85d0-7007-42e5-9e10-fc51fbd697e3
📒 Files selected for processing (1)
test/unit/client.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
test/**/*.test.ts
📄 CodeRabbit inference engine (AGENTS.md)
Use Vitest globals with
test/unitandtest/e2esplit for testing
test/**/*.test.ts: Test files should use*.test.tsnaming convention
Vitest globals (describe,it,expect,vi) are enabled and should be used without imports in test files
Files:
test/unit/client.test.ts
test/unit/**/*.test.ts
📄 CodeRabbit inference engine (test/AGENTS.md)
Unit tests should rely on mocks/spies rather than external dependencies
Files:
test/unit/client.test.ts
🧠 Learnings (6)
📚 Learning: 2026-03-10T07:36:54.862Z
Learnt from: CR
Repo: oritwoen/regxa PR: 0
File: test/AGENTS.md:0-0
Timestamp: 2026-03-10T07:36:54.862Z
Learning: Applies to test/unit/purl.test.ts : PURL contract tests should be located in `test/unit/purl.test.ts` for parse/build validation
Applied to files:
test/unit/client.test.ts
📚 Learning: 2026-03-10T07:36:03.586Z
Learnt from: CR
Repo: oritwoen/regxa PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-10T07:36:03.586Z
Learning: Applies to src/**/*.ts : Do not duplicate retry/backoff constants outside `src/core/client.ts`; centralize all retry logic
Applied to files:
test/unit/client.test.ts
📚 Learning: 2026-03-10T07:36:54.862Z
Learnt from: CR
Repo: oritwoen/regxa PR: 0
File: test/AGENTS.md:0-0
Timestamp: 2026-03-10T07:36:54.862Z
Learning: Applies to test/unit/{license,repository}.test.ts : Normalization tests should be located in `test/unit/license.test.ts` and `test/unit/repository.test.ts` for canonical output normalization
Applied to files:
test/unit/client.test.ts
📚 Learning: 2026-03-10T07:36:12.605Z
Learnt from: CR
Repo: oritwoen/regxa PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-03-10T07:36:12.605Z
Learning: Applies to src/core/client.ts : Implement all fetch and retry behavior only in `src/core/client.ts`
Applied to files:
test/unit/client.test.ts
📚 Learning: 2026-03-10T07:36:54.862Z
Learnt from: CR
Repo: oritwoen/regxa PR: 0
File: test/AGENTS.md:0-0
Timestamp: 2026-03-10T07:36:54.862Z
Learning: Applies to test/unit/{lockfile,cached-registry}.test.ts : Cache behavior tests should be located in `test/unit/lockfile.test.ts` and `test/unit/cached-registry.test.ts` for freshness, TTL, integrity, and wrapper behavior validation
Applied to files:
test/unit/client.test.ts
📚 Learning: 2026-03-10T07:36:54.862Z
Learnt from: CR
Repo: oritwoen/regxa PR: 0
File: test/AGENTS.md:0-0
Timestamp: 2026-03-10T07:36:54.862Z
Learning: Applies to test/e2e/smoke.test.ts : Live smoke tests should be located in `test/e2e/smoke.test.ts` for network-sensitive ecosystem checks
Applied to files:
test/unit/client.test.ts
🧬 Code graph analysis (1)
test/unit/client.test.ts (1)
src/core/client.ts (1)
parseRetryAfter(20-34)
🔇 Additional comments (2)
test/unit/client.test.ts (2)
3-23: Good coverage of the numeric and default branches.These cases pin down the direct numeric path and the 60-second fallback, so this helper cannot silently drift back to
NaNfor nullish or empty inputs.
32-59: The edge-case matrix here is worth keeping.Past-date clamping, strict rejection of
"120s"and"1.5", and the leading-zero case all document behavior that callers will otherwise have to guess from the implementation.
2 participants
The
Retry-Afterhandler inClient.getJSONrunsNumber.parseInton the raw header value. Numeric strings like"120"work fine, but RFC 7231 also allows HTTP-date values ("Wed, 21 Oct 2025 07:28:00 GMT"), and parseInt returns NaN for those. That NaN ends up inRateLimitError.retryAfter, which breaks the error message and any consumer code that reads the field.Extracted a
parseRetryAfter()function that tries numeric first, thenDate.parsefor HTTP-date, and falls back to 60s when neither works. The registries regxa targets all use numeric values today, but the NaN fallthrough was still a bug waiting to happen.Addresses item 3 from #18.