[P1][Review] Reject malformed price rows instead of manufacturing zero-dollar data

[karlwaldman]

Review-only / approval gate

No implementation, PR merge, package publication or production mutation until the owner approves the full sprint plan.

Evidence

Reviewed main 68795010c4c83b02ae5972959c8bd2321691aebf.

src/Client.php historical/demoPrices use Price::fromArray without latestPriceOrFail validation. src/Price.php defaults absent code to empty string, price to 0.0, currency to USD. Mock successful historical response data.prices=[{}] returns a Price with code='',price=0,currency='USD'; a missing/non-array prices field becomes an empty successful list.

Preferred smallest change

Use one minimal validated price-row boundary across latest/history/demo. Distinguish a legitimately empty list from a malformed envelope. Preserve legitimately zero numeric prices; do not use truthiness or invent defaults for missing required market data.

Acceptance / negative-path tests

Fixtures: absent/non-array prices, empty list, null/missing/non-numeric/zero/negative values according to contract, missing code/currency, malformed timestamps. Latest/history/demo agree and surface ApiException rather than misleading data.

Risk and PR gate

P1 data correctness; response-only isolated fix, Sunday eligible if no API schema change.
One focused issue PR; independent adversarial review must challenge deletion safety, public compatibility, failure recovery and whether a smaller deletion suffices. Include red/green test evidence and packed/installable artifact proof for any published SDK/MCP change. Merge only in the approved priority order after reviewer objections are resolved. Do not publish a breaking change as a patch. If already fixed at execution time, record evidence and skip without a cosmetic PR.

[karlwaldman]

Adversarial sprint admission — rank 23/70

Scheduled Sunday, 2026-09-06 (America/New_York) in the approval-gated seven-night plan. Planning only; no implementation or merge yet.

Narrow outcome: Reject malformed price rows instead of inventing zero.

Required issue → PR → independent review → ordered merge evidence

• Reproduce on the execution-day main and confirm exact files/callers; the review baseline was the current-main SHA recorded in this issue. If already fixed or unnecessary, record proof and close without a cosmetic PR.
• Record why deletion alone does or does not solve this; delete redundant code before adding a helper/guard/automation.
• A focused failing regression/fixture first where practical; test the relevant malformed, absent, stale, rejected or timeout path as well as success. No production customer mutations or raw-secret evidence.
• One independently revertible PR, containing the narrow correction and targeted tests; no unrelated cleanup. Explain public/API compatibility and rollback.
• Independent adversarial reviewer tests the failure case, challenges the assumptions and simpler alternatives, and resolves every blocking finding.
• Refresh against the actual merge base and rerun checks before merging in this plan's numbered order. Preserve all existing required checks; no bypass to hit ten.
• Link merge SHA plus approved deployed/published artifact and post-release proof where relevant; otherwise mark release pending rather than claiming shipped.

Risk window: Only isolated low/medium-risk changes. If admission discovers authentication/entitlement, destructive migration, live ingestion semantics or a breaking interface change, hold and move to the next approved Friday/Saturday window.

[karlwaldman]

Coverage-audit revision — current sprint admission

Plan v2: oilpriceapi-api#8015 supersedes earlier scheduling comments. Rank 23/70; Sunday, 2026-09-06, America/New_York. Low/medium-risk only; reclassify to a Friday/Saturday slot if boundary/compatibility risk is higher.

Intended outcome: Reject malformed price rows instead of inventing zero.

Coverage ledger: #8056. Inventory/pattern screening is complete; exhaustive semantic/runtime sign-off is not. Before implementation, review the complete affected file set and callers, revalidate against current main, supply the missing negative-path proof, and retain prior scope corrections. Prefer deletion, then simplification; correct behavior needs no PR.

No implementation is approved yet. After Karl approves: focused reversible PR → independent adversarial review → passing affected checks → merge in plan order → separately approved release/publish and artifact/customer-path verification. Maximum ten planned slots per night, not ten forced merges. If the dated window elapses before approval, re-anchor the calendar rather than backdate execution.

[karlwaldman]

Two more members of this defect family, both confirmed by execution on main (3feb505) today, PHP 8.5.8. Adding them here rather than filing separately since they are in scope for this issue and PR #18 already fixes one of them.

1. TypeError escapes the ApiException hierarchy — fixed by #18, live on main today.

$t = new MockTransport();
$t->queue(200, ['status' => 'success', 'data' => ['prices' => ['garbage', 1]]]);
(new Client('k', 'https://api.oilpriceapi.com', 10.0, 0, $t))->pastDay('B');

TypeError: OilPriceAPI\Price::fromArray(): Argument #1 ($data) must be of type array, string given
instanceof ApiException? false

src/Client.php:190 and :167 pass every element of data.prices straight into Price::fromArray(...). With declare(strict_types=1) a non-array element is a TypeError, which does not extend ApiException, so the catch (ApiException $e) pattern the README teaches gets a fatal instead. #18's priceOrFail() guards with is_array($row) and fixes this — worth keeping the regression test.

2. is_numeric() accepts INF, which then poisons the caller's serialization — NOT fixed by #18.

$p = Price::fromArray(['code' => 'B', 'price' => '1e400']);
var_dump($p->price);                               // float(INF)
json_encode($p->toArray(), JSON_THROW_ON_ERROR);   // JsonException: Inf and NaN cannot be JSON encoded

is_numeric('1e400') is true, so #18's !is_numeric($data['price']) guard passes it through and (float) produces INF. A caller who round-trips a Price through toArray() + json_encode() — the obvious thing to do with a DTO — gets an exception from their own serializer, at a call site with no connection to the SDK. Suggest is_finite((float) $data['price']) alongside the is_numeric() check.

Two further gaps found in the same review are filed separately because they are distinct defects that #18 leaves open: #20 (fabricated timestamps) and #21 (the remaining (string) casts on currency/unit/etc.).