Skip to content

Fix historical period requests rejected by the API - #145

Open
karlwaldman wants to merge 2 commits into
mainfrom
codex/9169-sdk-history-window-20261031
Open

karlwaldman wants to merge 2 commits into
mainfrom
codex/9169-sdk-history-window-20261031

Conversation

@karlwaldman

@karlwaldman karlwaldman commented Oct 31, 2026 •

Copy link
Copy Markdown
Member

getHistoricalPrices({ period: "past_week" }) sent an unsupported period query to /v1/prices/past_year, which production rejects with HTTP 400 UNKNOWN_PARAMETERS. Select the requested supported window route instead; keep custom dates on past_year, and reject unknown periods or period-plus-date combinations before a request.

The paginator also assumed a 500-row cap. Live raw window responses currently apply a 100-row cap, so a 500-row request could stop after the first 100 rows. Clamp automatic historical pagination to 100 and update the documented exported cap. This can require more calls, but prevents silently truncated history.

Reproduced the 400 with an internal QA key. The period regression and a 100-row-server-cap regression failed before the fixes. The historical and pagination suites now pass (42 tests), pai-check passed for all changed TypeScript files, and the package build passed. The built SDK returned WTI data from all three period routes and fetched two distinct 100-row raw pages in production using an internal QA key.

Fixes #144 when merged and released. Existing installed SDK versions remain affected until updated; evaluate a separately tested backend compatibility path for them in OilpriceAPI/oilpriceapi-api#5913. Repository auto-merge is disabled and main has no required status checks; do not merge by bypassing repository release controls.

@coderabbitai

coderabbitai Bot commented Oct 31, 2026 •

Copy link
Copy Markdown

Important

  • Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 6f039d3a-2c87-4591-b9b8-bef9e97befa8
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

@karlwaldman karlwaldman added the risk:elevated Elevated: auto-merges after an adversarial review APPROVE label Oct 31, 2026
@karlwaldman

Copy link
Copy Markdown
Member Author

Adversarial review of exact head f10ee09: BLOCK — silent historical truncation in the newly supported period routes.

Read-only production, internal QA key, 2026-10-31: /v1/prices/past_week?by_code=WTI_USD&interval=raw&per_page=500&page=1 returned 100 of 1,674 points, metadata.truncation.pagination.per_page_applied=100 and 17 total pages. Page 2 returned a different 100. /past_month and /past_year likewise returned 100 for the same 500 request. paginateHistoricalPrices clamps the caller's perPage to 500, then stops when results.length < perPage, so paginateHistoricalPrices({period:'past_week', commodity:'WTI_USD', interval:'raw', perPage:500}) now yields only the first 100 and silently stops. The request route is newly enabled by this PR; it previously failed with UNKNOWN_PARAMETERS, so a wrong partial result is a customer-visible regression.

Re-ran exact-head historical + pagination unit suites in an isolated worktree: 37 tests passed, but they do not model the live 100-row window cap. Production also confirmed the old period query returns 400 and the three selected window endpoints return 200, and a 2024 explicit-date window returns rows within its requested range. Those checks do not resolve the truncation.

Please have the generator honor the effective page size or next-page/truncation metadata for these window routes, add a 500-request/100-response multi-page regression and a normal short final-page test, verify against live window pagination, and rerun tests. The existing installed package remains broken until a compatible SDK is actually published or an independently tested server compatibility path ships.

@karlwaldman

Copy link
Copy Markdown
Member Author

Adversarial re-review of exact head b87632b: APPROVE, subject to CI and repository release controls.

The earlier BLOCK was silent pagination truncation at the production 100-row window cap. The updated generator clamps its request/short-page comparison to 100, and tests model a capped 100-row response across many pages and a short final page. In an independent detached exact-head worktree, npm run build succeeded and historical, pagination and server-cap suites passed 42 tests. With the built SDK and an internal QA key, paginateHistoricalPrices({period:'past_week', commodity:'WTI_USD', interval:'raw', perPage:500}) returned two distinct 100-row pages; this directly exercises the previously blocked case. Prior read-only production probes confirmed the three selected period paths return 200, the old unsupported period query 400, and a bounded custom 2024 date range returns rows in that range. Tests also reject ambiguous period-plus-dates and invalid periods before requests.

Tradeoffs and limits: the exported MAX_PER_PAGE changes 500→100 and complete iteration may take up to five times as many requests; customers should budget quota accordingly. getHistoricalPrices is still a single-page array and does not surface the API's truncation metadata. The two-page live smoke is not proof of every page or every customer's entitlement. Existing installed SDKs remain affected until an updated package is released, or a separate backward-compatible server change ships. Repository auto-merge/branch protection and required checks remain independent blockers.

@karlwaldman

Copy link
Copy Markdown
Member Author

Clarification: the review does not assert that required checks exist on this repository. The PR description says main has no required status checks and auto-merge is disabled. CI passing is useful evidence, but it does not supply the missing repository protection; the risk gate must fail closed.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

risk:elevated Elevated: auto-merges after an adversarial review APPROVE

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[P1] Historical period requests fail with UNKNOWN_PARAMETERS

1 participant