fix(futures): stop truncating a commodity code into a different contract - #111
Merged
Merged
Conversation
`normalize_futures_slug` split on the first separator and kept the leading
token, discarding the geography and the currency -- the two fields that
distinguish these instruments. A caller asking for one thing received another
contract's curve, with no error.
Measured against all 604 live catalog codes on 2026-09-13: 586 raised
(correct), 18 silently rewrote.
LNG_NW_EUROPE_EUR -> lng-jkm NW Europe LNG in EUR -> Japan/Korea Marker in USD
LNG_SOUTH_EUROPE_EUR -> lng-jkm
LNG_EU_AVERAGE_EUR -> lng-jkm
WTI_MIDLAND_USD -> wti Permian basis grade -> Cushing WTI
WTI_SPOT_CUSHING_USD -> wti spot -> futures curve
BRENT_SPOT_EUROPE_USD -> brent
Reachable from every method in resources/futures.py (lines 340-411). Same class
as the mcp-server substitution shipped as v3.3.0 today.
The suffix stripping exists for a real reason -- month and order markers on
contract codes ("CL.1", "CL1!", "BZ-2025-12") -- so it is kept, but the
discarded tail must now actually look like a month/order marker. Alphabetic
tails carrying geography and currency are no longer thrown away, and an
unrecognised input raises ValueError as it already did for the other 586.
After the fix all 604 live codes raise and none are rewritten; the four
contract-code forms still resolve.
Red before: 6 failed, 34 passed. Green after: 40 passed.
Wider unit suite unchanged at 109 pre-existing failures, verified identical on
clean origin/main.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015ao5paex73xXvuM424Libo
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 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. Comment |
karlwaldman
added a commit
that referenced
this pull request
Sep 13, 2026
#122) (#125) * fix(url,retry,futures): un-break today's three regressions (#119, #118, #122) All three shipped today and all three are reachable from the SDK's own code. #119 -- `_url.py` rejected U+0020 SPACE. `range(0x21)` is 0x00-0x20 INCLUSIVE; the comment above it names CR/LF request splitting and NUL, all below 0x20, so the range over-reached by exactly one character. `/v1/prices?name=Brent Crude` raised where it previously worked, and `resources/demo.py` builds exactly that shape (`path = f"{path}?codes={','.join(codes)}"`). A space cannot introduce an authority or split a request line -- httpx percent-encodes it -- so nothing the guard exists to stop is enabled by allowing it. Now `range(0x20)`; 0x00-0x1F, 0x7F and backslash stay refused, pinned by tests. #118 -- #115 keyed replay safety on the HTTP method, which is right, but no audit of this SDK's own call sites was done, so its POST-shaped READS lost all retries. A station lookup is idempotent by construction: same lat/lng/radius, same answer, nothing created. Removing its retries was a pure availability regression -- a call that used to ride out one bad gateway response now fails on the first blip. The rule used to classify them: a POST is read-shaped when the endpoint creates or mutates no server-side resource AND the same request sent twice returns the same answer. Six call sites qualify and now pass `idempotent=True` -- diesel `get_stations`, `webhooks.test`, `alerts.test`, each in both clients. `data_sources.test` deliberately does NOT: it triggers a fetch against the customer's configured source and can write to its ingest log. `create()` and `rotate_credentials()` are unchanged and still sent exactly once. `test_every_post_call_site_declares_intent` walks the package with `ast` and fails until every `method="POST"` call either passes `idempotent=` or is listed as a known write with a reason, so the next POST-shaped read cannot quietly lose its retries the same way. #122 -- `normalize_futures_slug` raised a builtin `ValueError`, outside the SDK hierarchy. That predates #111, but #111 turned 18 live catalog codes from "resolved to a DIFFERENT instrument" into "raises" -- the right call -- so user code written as `except OilPriceAPIError: fall_back()` went from a silently wrong answer to an uncaught crash. #111's own comment argues a refusal is recoverable; it is only recoverable if it is catchable. New `FuturesContractError(ValidationError, ValueError)`: catchable through the documented base class, and still a `ValueError` so existing callers are unaffected. It renders its own message, which lists every valid slug and contract code, so the guidance survives regardless of #117's merge order. Tests: tests/unit/test_url_allows_space.py (10), tests/unit/test_read_shaped_post_retries.py (12), tests/unit/test_futures_contract_error.py (26). Proven red against `origin/main` source: 28 failed / 23 passed. Suite: 112 failed / 681 passed / 63 skipped, against a measured baseline of 112 failed / 630 passed / 63 skipped on clean main (the 112 are pre-existing, all missing pytest-asyncio and respx in the environment). Note for reviewers: `resources/alerts.py` and `resources/diesel.py` are CRLF files in this repo. Their line endings are preserved -- the diff is 5 and 6 lines, not the whole file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015ao5paex73xXvuM424Libo * style: sort FuturesContractError into the __init__ import block ruff's isort rule failed CI: the new export was inserted before ValidationError rather than in alphabetical position after DataNotFoundError. No behaviour change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015ao5paex73xXvuM424Libo --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Sep 13, 2026
karlwaldman
added a commit
that referenced
this pull request
Sep 13, 2026
…month slug (#128) (#136) * fix(futures): stop answering a dated contract with the generic front-month slug (#128) `normalize_futures_slug` stripped a trailing digit run unconditionally, so a DATED contract was silently answered with the GENERIC slug -- a different instrument: WTI2026 -> 'wti' BRENT2027 -> 'brent' NG2026 -> 'natural-gas' TTF2026 -> 'ttf-gas' CONTRACT_CODE_TO_SLUG holds no key containing a digit, so every one of those came from the strip rather than a real mapping. The request went out as /v1/futures/wti/curve, returned data, and nothing told the caller their year had been discarded. This is the other half of what #111 fixed. #111 removed the guess from the separator-splitting branch (WTI_MIDLAND_USD now raises rather than resolving to wti); the digit strip on the very next line still guessed. An order marker is now bounded to one or two digits -- the same bound _is_month_or_order_suffix already implies for the separator branch, and wide enough for CL1, CL2, CL12 and the TradingView CL1!/CL.1 forms. Three or more digits is a year or an unknown code and falls through to the existing FuturesContractError refusal. Verified against the live catalog on 2026-09-13: of 604 codes, none has a trailing digit run at all, so the narrowing refuses nothing that exists today. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015ao5paex73xXvuM424Libo * test(futures): mock the transport the way this repo already does respx is not in the [dev] extra, so the two wire tests failed CI with ModuleNotFoundError on every Python version. Patch httpx.Client.request / httpx.AsyncClient.request instead, matching tests/unit/test_diesel_envelope.py, rather than adding a test dependency for two assertions. Same assertions, same red. Adds a control that CL1! still reaches /v1/futures/wti, so the test proves the refusal is specific to a dated code and not a blanket break. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015ao5paex73xXvuM424Libo --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Same bug class as the mcp-server substitution released today as v3.3.0: a local resolver rewriting a real code onto a different instrument, silently.
Defect
normalize_futures_slug(oilpriceapi/resources/_futures_slug.py) split on the first separator and kept only the leading token. That discards the geography and the currency — exactly the fields that distinguish these instruments.Executed against all 604 live catalog codes on 2026-09-13: 586 raised
ValueError(correct), 18 silently rewrote.LNG_NW_EUROPE_EURlng-jkm— NW Europe LNG in EUR → Japan/Korea Marker in USDLNG_SOUTH_EUROPE_EURlng-jkmLNG_EU_AVERAGE_EURlng-jkmWTI_MIDLAND_USDwti— Permian basis grade → Cushing WTIWTI_SPOT_CUSHING_USDwti— spot → futures curveBRENT_SPOT_EUROPE_USDbrentReachable from every method in
resources/futures.py(lines 340–411). The caller receives another contract's curve and has no way to tell.Fix
The suffix stripping exists for a legitimate reason — month and order markers on contract codes (
CL.1,CL1!,BZ-2025-12) — so it is kept. What changed is that the discarded tail must actually look like a month or order marker. An alphabetic tail carrying geography and currency is no longer thrown away, and the input raisesValueErroras it already did for the other 586 codes.A refusal is recoverable; a wrong instrument's curve is not.
Evidence
Pre-existing failures, NOT caused by this change
The wider
tests/unitsuite has 109 failures on cleanorigin/main(mostlytest_streaming.py). Verified identical with and without this change by reverting the file and re-running the same scope. Untouched here.Related, not in scope
resources/prices.py:41stamps the requested code onto the response when the response omits one. That manufactures the appearance of a match and would defeat a consumer-sidedata.code == requestedcheck — worth its own fix, since that assertion is the durable guard for this whole bug class.🤖 Generated with Claude Code
https://claude.ai/code/session_015ao5paex73xXvuM424Libo