Fix unbookable award prices rendering on cash rows - #34
Open
ak2k wants to merge 19 commits into
Open
Conversation
…lback A live MSY→MIA search rendered `9.1k Delta + $6` as the economy award on AA3539 — a redemption that cannot exist, since Delta can't ticket American metal. The route+time codeshare fallback in match.py keyed solely on (origin, destination, departure-to-the-minute), documented as "a near-tight identity." It isn't. On a dense domestic route two carriers routinely push back from the same airport pair at the same minute: MSY→MIA 2026-09-09T10:45 carries both AA3539 and DL1424. Both hash to the same key, the join unions them, and the renderer's lowest-miles-per-cabin pick surfaced Delta's price on the American row. The number was real — just for a different aircraft. Gate the fallback on `same_metal`: the two flight numbers must share an IATA prefix, or their carriers must be co-members of an alliance / bilateral partnership. Identical-carrier covers the ordinary case; the partner table preserves the genuine AA-marketed/BA-operated codeshare the fallback exists to bridge. Unparseable carrier fails closed — a missed match shows no award, a wrong one invents an unbookable price. Scoped to the route+time fallback only. The (flight#, date) primary key is self-corroborating and untouched, so partner redemptions on an exact flight match (Alaska miles on AA867) still render. Verified live: AA3539 economy now reads `11.0k American Airlines + $6`; all other rows, including the Alaska-on-AA oneworld awards, are unchanged. The 5 new regression tests fail against the pre-fix matcher and pass after.
Round 1 gated the route+time fallback on `same_metal` and claimed the collision class was closed. Code review (correctness + adversarial, both reproducing against the worktree) showed it wasn't — the guard relocated the bug rather than removing it, and made it less visible: a Delta price on an AA row reads as obviously wrong, an Alaska price does not. Four confirmed defects, all now pinned by tests that fail against round 1: 1. Intra-alliance collisions survived. Same-route/same-minute competition is DENSEST inside an alliance, which is exactly what `same_metal` admits. Cash AA118 JFK->LAX 10:45 with awards AA118/American/25k and AS17/Alaska/ 7.5k attached both, and the renderer's lowest-miles pick showed 7.5k Alaska on the American row. Fix: `_pick_metal` resolves a bucket rather than filtering it — an exact-carrier award wins outright and suppresses the partners beside it; a bucket of two or more DIFFERENT partner carriers is unresolvable and attaches nothing. 2. The alliance table rejected the codeshares the fallback exists for. Regional operators are not alliance members, so mainline-marketed / regional-operated — the dominant real shape, and the repo's own documented LH9498/EN8858 example — stopped joining. Fix: `_REGIONAL_OPERATORS`, directional on purpose (a symmetric table would pair MQ with OH, two unrelated AA regionals). 3. Connection count was never compared. A 1-stop award is cheaper than the nonstop it renders beside, so it won the lowest-miles pick and printed under a "nonstop" label taken from the cash slice. 4. `(flight#, date)` omitted route, so an AA100 JFK->LHR row could attach an AA100 MIA->DFW award. Flight numbers are unique per route per day, not per day. Pre-existing, but round 1 newly asserted this key was "self-corroborating" — the claim was wrong, so the key is now (flight#, date, origin, dest). Deliberate fail-closed change: cash AA6939 with both a BA174 and an AA6939 award in one bucket now keeps only AA6939. That bucket is structurally identical to the AA118/AS17 collision, so the two cannot be told apart from (route, time, flight number) alone. Cost is a hidden second booking option for one seat; the alternative cost is another aircraft's price on the row. Only `Slice.legs[i].operating_carrier` ground truth can distinguish them — see the characterization test, which should flip back when the join reads it. Live: AA3553 economy 4.5k Alaska -> 9.5k American, AA867 business 9.0k Alaska -> 25.5k American. Round 1 was still rendering wrong prices on those rows. make check green: 528 tests, ruff + basedpyright clean.
Answering "is failing closed on the AA6939/BA174 bucket optimal?" — no. The premise was wrong: I claimed (route, time, flight number) was all we had, but `arrival` is present on BOTH sides (`AwardFlight.arrival`, `Slice.arrival`) and was simply unused. It is also a near-perfect discriminator. Two aircraft sharing a departure minute on the same route essentially never share an arrival minute — both sides read the same published schedule. Measured on a live MSY→MIA/FLL payload: keying on route+departure left 3 multi-carrier buckets out of 41; adding arrival left 0 out of 91. So `_pick_metal` now filters the bucket by arrival first, and the deliberate regression from the previous commit is gone: cash AA6939 + a BA174 award at the same arrival resolves to the codeshare (attached), while an AS99 award sharing only the departure minute is correctly excluded. The characterization test that recorded the fail-closed behavior is replaced by tests asserting the resolution. Carrier logic (same-carrier-wins, then single-partner) stays as the backstop: `Slice.arrival` is nullable and providers may omit it. The arrival filter also only applies when it matched something, so an award side without arrivals degrades to the old path instead of wiping the bucket. This caught one more live error the previous round missed: AA3539 economy was still reading 11.0k, picked up from the AA2566 award — same route, same 10:45 departure, arrival 13:47 vs 13:46. Correct value is 15.5k. Only arrival separated them. make check green: 530 tests, ruff + basedpyright clean.
Every award-enabled run emitted ~20 warning lines and burned ~10 pointless
round-trips:
pp_airline_search_failed airline=ThaiAirways body='{"error":"unsupported
airline"}' status=400 (x9, twice per leg)
PP's /api/pricing-info advertises a superset of what /api/airline-search
serves. Nine of its entries — ANA, Aeromexico, BritishAirways, CathayPacific,
EvaAir, Finnair, JapanAirlines, Southwest, ThaiAirways — carry no
`enable<Airline>` feature flag, and `enabled_airlines` treats a missing flag
as always-on (correct for American/Delta/United/JetBlue/Alaska). So we called
them every run and collected a 400 every time.
A hardcoded exclusion list would be wrong: from the flags alone the two cases
are indistinguishable. AirFrance also has no exact-name flag (only
`enableAirFranceV2`) and DOES serve results. So instead of guessing, record
the server's own verdict — a 400 whose body names the airline as unsupported
— in a 7-day negative cache and skip those airlines up front.
Deliberately narrow and fail-open:
- Only a 400 *and* an "unsupported airline" body qualifies. A 429 or 503
with the same body must not blacklist a working airline, so status is
checked too (tested).
- Any cache read problem (missing, corrupt, wrong shape, mixed types)
yields the empty set: a stale-empty cache costs one wasted request,
whereas a wrongly-populated one would silently hide real awards.
- The 7-day TTL means PP adding support, or a tier change, heals on its own
with nothing for the user to clear.
- The remaining genuine failures still log at warning; the expected
steady-state ones drop to debug.
Live: warnings on a warm run go 18 -> 0, the fan-out drops 9 requests per
leg, and the rendered award table is unchanged.
make check green: 535 tests, ruff + basedpyright clean.
…fixes
Second ce-code-review pass. Findings from reliability, maintainability, and
testing; the two highest-stakes reviewers are still running.
P2 (reliability) — the negative cache could never expire. The TTL was keyed on
the FILE's mtime, so learning any new airline refreshed every existing entry.
Since a run that learns one airline rewrites the file, in steady state nothing
aged out and the self-healing the TTL exists for did not happen. Reproduced:
with a 2s TTL, an entry 2.4s old survived because a sibling was written at
1.2s. Each entry now carries its own learned-at timestamp. The legacy flat-list
format is still read (entries treated as learned now) so upgrading doesn't
re-query every unsupported airline.
Also on that path: the write is now write-then-rename, so a crash mid-write
leaves the previous cache rather than a truncated file that reads as empty.
Documented the no-await invariant that makes the read-modify-write safe under
the anyio fan-out — a future async file API there would open a real race.
P2 (maintainability) — `_cash_slice` was extracted last round but never wired
in; all three cash_* key functions still re-derived the bounds check inline.
Now routed through it, and moved above its first use.
P2 x2 (testing) — the connection-count check was only ever exercised at zero
stops, and the negative cache had no end-to-end coverage. Added multi-stop
match/reject tests, a `_slice_stop_count` fallback test, and a new
tests/pp/test_client_unsupported_airline.py driving the real MockTransport
path: 400 records, cached airline is never re-requested, a non-verdict 400 and
a 429 both refuse to blacklist, and five concurrent rejections all survive the
fan-out.
Also pinned the arrival comparison against Matrix's real offset-suffixed wire
format ('2026-09-09T09:04-04:00' vs PP's naive '2026-09-09T09:04:00'), which
was previously only tested with naive strings — a format drift there would
silently turn the arrival discriminator into a no-op.
Testing reviewer independently reproduced all three of the prior rounds'
before/after claims exactly (12 fail, 1 fail, 535 pass).
make check green: 550 tests, ruff + basedpyright clean.
…efore The correctness reviewer found that commit 2569c04 (arrival as the award-match identity) reintroduced the exact bug this branch exists to eliminate. `_pick_metal` filtered the ENTIRE bucket by arrival before computing the exact-carrier set. So an award with the wrong metal whose arrival happened to match could survive, while the correct same-carrier award — one that merely omitted its arrival — was deleted before the carrier rule ever ran. Reproduced through the real renderer: cash AA6939 (arr 06:30) with awards BA174/American/60k (no arrival) and AS99/Alaska/7.5k (arr 06:30) rendered `7.5k Alaska + $6` on the American row. Round 2 returned a safe no-match for the same input, so this was a regression I introduced, not a pre-existing gap. It also re-opened the AA118/AS17 collision that fb59713's own test asserts is closed, whenever arrival coverage is partial. Fix: arrival now narrows WITHIN each carrier stage via `_narrow`, never ahead of them. `_narrow` also abstains unless every candidate carries an arrival — partial coverage is precisely the dangerous case, since filtering a mixed bucket silently drops the awards that only omit the field. And when nothing matches the cash arrival it keeps the bucket rather than zeroing it: the reviewer's cross-check of the seats.aero fixture against our live Matrix cache found AA106 disagreeing by 5 minutes (19:20 vs 19:15), so "both sides read the same schedule" is not exceptionless in our own data. Also documented, from the same review: seats.aero's `Z` suffix is mislabelled — those timestamps are local, not UTC. Harmless today because `_iso_minute` truncates the suffix away, but a future caller that genuinely parses them as instants gets a ~offset-sized error (~12h JFK→LHR block times instead of ~7h). `_narrow`'s docstring records this. Four new tests; the two covering the P0 fail against the pre-fix ordering. make check green: 554 tests, ruff + basedpyright clean.
… fallback Adversarial review: every commit on this branch hardened the route+time fallback and left the two HIGHER-priority keys unguarded. Both render unbookable prices, so the arrival discriminator added in 2569c04 was dead code whenever either of them hit. P0 — primary key. `(flight#, date, origin, dest)` uses the FIRST segment only, so every connecting journey starting on that flight collapses onto one key. Confirmed in this repo's own seats.aero fixture: three distinct AA1444 JFK→LHR journeys share a key, arriving 06:55 / 09:05 / 12:50, business fares 104k–115.5k. All three attached and the renderer's lowest-miles pick quoted the 12:50 journey's 104k on the 06:55 row. Ten more collided keys in the same 56-award payload. Now narrowed by arrival. P0 — matched-id path. Zero corroboration: PP mints matchedGoogleFlightId from a hint we supply and its matcher is documented as deliberately loose, so the echoed ID is a claim, not proof. It re-attached DL1424/Delta/9.1k to an AA3539 cash row — the branch's original bug, arriving by a different door. Now gated on `same_metal` (not exact, so codeshare bridging survives) plus arrival. Pre-existing, but this path becomes the default as gflight rolls out. P1 — `_narrow` had two escape hatches that re-admitted disproved awards: one member missing an arrival disabled the filter for the whole bucket, and a bucket where every arrival disagreed — the strongest possible evidence — was restored wholesale. Now per-candidate: an arrival that agrees or is absent is kept, one that disagrees is dropped. That last change reverses a call from the previous commit. Schedule drift is real (a fixture-vs-live cross-check found AA106 at 19:20 vs 19:15), so dropping on disagreement will cost some legitimate matches. Taken knowingly: an empty cell invites investigation, a wrong price invites a booking attempt. A tolerance window is the fix if drift proves common — not a wholesale fallback. The superseded test is replaced by one asserting the new behavior and recording why. Five new tests; three fail against the pre-fix matcher. make check green: 558 tests, ruff + basedpyright clean. Live output unchanged (AA3539 15.5k American, AA867 4.5k Alaska).
…rors Clearing the two items the last review left as follow-ups. Both were called "pre-existing," which was true and not a reason — the first is load-bearing for the arrival discriminator this branch just added, and the second is a written project rule. seats.aero timestamps. `DepartsAt`/`ArrivesAt` carry a 'Z' suffix but hold LOCAL time at each airport. Confirmed by fixture arithmetic: JFK->LHR nonstops read ~12.1h block time when the Z is honoured, against a real ~7h — and 09:35 local JFK plus 7h is 21:35 London, which is the 21:40 they report. Correctness today depends on `_iso_minute` happening to truncate the suffix away, i.e. on the mislabel staying benign. Now normalized at the provider boundary (`_local_naive`) so `AwardFlight.departure`/`.arrival` mean naive-local for every provider, and the models.py annotation says what the field actually is instead of repeating upstream's error. The wire-shape test still pins the 'Z' as what arrives; a new provider test pins that it doesn't survive. PP catalog errors. `pricing_info`/`extension_config` called `r.raise_for_status()` directly, leaking `httpx.HTTPStatusError` past the boundary — contra AGENTS.md Principle 1. Added `PPApiError` carrying endpoint and status, raised via a `_raise_for_status` helper with the httpx error as `__cause__`. Both callers already catch broad `Exception`, so behavior is unchanged; the error now names its domain and the traceback keeps the cause. Per-airline search failures are deliberately untouched — those are non-fatal and handled inline. make check green: 561 tests, ruff + basedpyright clean. Live output unchanged.
…l review Independent adversarial pass from a different model family, prompted to refute rather than confirm. It broke the claim a fourth time — each finding reproduced here through the real renderer before being accepted. P0 — matched-ID buckets were filtered, not resolved. Several awards can share one matchedGoogleFlightId, and `same_metal` alone still admits a partner beside the true same-carrier award. On a RETURN leg (slice_index=1, which had almost no coverage): cash AA117 with AA117/25k and AS19/7.5k both attached and `_fmt_award_cell` printed 7.5k Alaska. Both this and the primary key now go through `_pick_metal`, the same resolver the route+time bucket uses. P0 — journeys sharing a first segment collapsed. Every key here identifies a journey by segment 0, and seats.aero discarded segments 2+ at the provider boundary. It returns both "AA1444, BA216" and "AA1444, AA100" on one JFK→LHR date; both attached to the [AA1444, BA216] cash row and the renderer printed the cheaper journey's 12k. `AwardFlight.segment_flight_numbers` now carries the ordered list, and `_by_segments` drops contradicting candidates. Providers that don't supply segments (PointsPath sends only the first) are unaffected — an empty list is absence of evidence, not disagreement. P0 — a cheaper no-arrival sibling beat the exact-arrival award. My previous "missing arrival is admitted" rule let a 10k candidate with no arrival sit beside the 60k award that positively confirmed the cash flight, and win the cell. `_narrow` now admits missing arrivals only when nothing matched exactly. Fixing that surfaced a subtlety worth naming: arrival is used two different ways and conflating them is itself a bug. Eliminating a candidate whose arrival CONTRADICTS the cash flight is sound evidence of different metal. Letting arrival CHOOSE between carriers is not — a partner that merely omits its arrival would lose to an unrelated carrier that happens to publish a matching one. Split into `_drop_contradicted` (safe before the ambiguity check) and `_narrow` (selects, so only within one carrier). P1 — legacy cache entries were immortal. The flat-list migration stamped them `now` on every READ, so they refreshed continuously and could never age out. They are now dated from the file's mtime, the only real timestamp they have. Nine new tests; all fail against the pre-fix code. make check green: 567 tests, ruff + basedpyright clean. Live output unchanged.
Cross-model correctness pass (codex/gpt-5.6). Two more P0s, both reproduced through the renderer before being accepted. P0 — the architectural one. Each key's bucket was resolved independently and the winners unioned, so a bucket seeing only part of the field decided on partial information. Cash AA6939 with a BA174 award (found by route+time) and an AS99 award (found by matched-ID): route+time sees two partner carriers and correctly abstains, but the matched-ID bucket sees AS99 alone, calls it an unambiguous single partner, and admits it — rendering 7.5k Alaska on an American row, undoing the rejection the other bucket had just made. Keys are DISCOVERY; resolution is one judgment over everything they found. Now: gather the union, dedup, resolve once. I had probed this exact shape myself and got a clean result — but only with a *contradicted* award, which `_drop_contradicted` catches in every bucket. The leak needs a candidate rejected for AMBIGUITY, which is bucket-local state. A weak probe read as proof of absence. P0 — `same_carrier` compares only the IATA prefix, so AA999 claimed exact-match priority over cash AA867 and skipped to `_narrow`, which admits a missing arrival when nothing matched exactly. Stage 1 now requires the full normalized flight number. An airline does not sell one departure under two of its own numbers, so a same-carrier number mismatch is a different flight and must prove itself with a matching arrival; only a genuine codeshare (different carrier) explains a mismatch without one. P2 — connection-count mismatches are now dropped BEFORE resolution. Filtering them afterwards let an ineligible candidate make the field look ambiguous and suppress a valid codeshare that would have won on its own. P2 — an expired cache entry was never re-stamped: `remember_unsupported_airline` returned early on mere presence, so a lapsed airline was re-queried, rejected, and still looked stale next run — re-queried forever, the exact waste the cache exists to prevent. Now re-stamps only when actually expired, so repeated rejections can't extend a fresh entry past its TTL either. Seven new tests; four fail against the pre-fix code. make check green: 573 tests, ruff + basedpyright clean. Live output unchanged.
…ntity Closes the PointsPath segment-identity gap flagged at the end of the last review, using data PP was already sending and we were discarding. PP's /api/airline-search returns `stops` (connection airports in order) and `travelDurationMinutes`; neither was in our pydantic model, so `extra="ignore"` dropped them silently. Matrix populates the directly comparable `Slice.stops`, and seats.aero has `Connections` — so connection airport works across ALL providers, where segment flight numbers only ever worked for seats.aero. The gap was real. Live MSY->LHR returns four distinct AA1650 journeys sharing a 12:22 departure and one connection, separable only by hub and arrival. With PP supplying no segment list, a cheaper wrong-hub award attached and the renderer's lowest-miles pick printed it — reproduced at 9k/ORD displacing 60k/DFW, now rejected. `_by_segments` becomes `_by_journey_shape`, applying two independent checks — connection airports and segment flight numbers — each skipped when its evidence is absent, so a provider supplying neither is judged exactly as before and no previously-working match is lost. Also added, for the other flagged item: every match admitted by the hand-curated partner/regional tables now logs `award_match_via_partner_table` at debug. Those tables are the weakest evidence here and by construction only decide when nothing stronger applied, which makes a stale entry fail SILENTLY. Measured on live traffic: 0 of 471 awards lacked an arrival, so this path effectively never fires today — the log turns that assumption into evidence before anyone considers deleting the tables. Four new tests; three fail against the pre-fix code. make check green: 577 tests, ruff + basedpyright clean. Live output unchanged.
…sponse
Found by a codex adversarial pass over `_gflight_ids.py` / the gflight adapter.
Pre-existing, unrelated to the award matcher.
fli types `FlightResult.price` as `NonNegativeFloat | None` — its own comment
says "None when not surfaced", which Google does on some premium round-trip
rows carrying an empty price head. `_price_string` formatted it
unconditionally and raised
TypeError: unsupported format string passed to NoneType.__format__
while building the SearchResult — so a single price-less row took down the
entire search, discarding every other itinerary in the response rather than
just its own.
Now returns "" for an unsurfaced price and the cheapest-price notice skips
those rows. The itinerary, its flights and its award overlay all remain
useful with the cash price shown as unavailable.
Two tests, both failing against the pre-fix code.
Note on the same review: the index parsing in `_gflight_ids.py` was the actual
target and came back CLEAN — the reviewer independently confirmed itinerary
index 17 and leg indices 12-17/22 against the installed Legrooms+ extension.
The remaining findings there (fli 0.9 makes NID cookie seed/persist a silent
no-op; an unknown airline/airport code turns valid rows into a false "no
results") are pre-existing robustness gaps, filed rather than fixed here.
make check green: 579 tests, ruff + basedpyright clean.
… rename Second finding from the codex pass over `_gflight_ids.py`. Pre-existing. fli <=0.8 exposed `Client._client`; 0.9 replaced it with a per-thread `Client._session()`. Both `_seed_cookies` and `_persist_cookies` still reached for the old attribute, so both raised AttributeError into their best-effort `except ... log.debug` and became NO-OPS. Verified against the installed client: it has no `_client`, and seeding set its once-per-process latch while installing nothing. That silently un-did a documented mitigation. This module's own comments put the cold-session empty rate at ~40% without a seeded NID versus ~0% with one — those empties are the "cold-session retries" the search then burns requests on. `_cookie_jar` now resolves either shape and RAISES when neither is present, so the next upstream rename fails loudly at the callers' debug log instead of degrading to a no-op indefinitely. Three tests, all failing against the pre-fix code. They run against the REAL installed fli client on purpose — a stub would have kept passing straight through the rename, which is precisely how this went unnoticed. make check green: 582 tests, ruff + basedpyright clean.
Codex adversarial pass over `pp/cli.py`, the renderer every matcher bug surfaced through. Eight findings, all pre-existing; the five with a concrete user-visible failure are fixed here. P1 — `--json` return leg described the OUTBOUND flight. `_serialize_matches` hardcoded `slices[0]` while the per-leg wrapper labelled the entry "return", so the return leg's awards were emitted beside the outbound flight number, route and departure. Now takes the leg's own `slice_index`. P1 — basic-economy awards rendered as unrestricted Economy. They carry different seat, bag and change rights, so showing one under the plain heading overstates what the miles buy — and being cheaper, it displaced real fares. Now excluded from the main pick; surfaced with an explicit "(basic)" label only when nothing unrestricted exists. P2 — equal-mile offers were ordered by provider, so a 30k + $500 award could beat an identical 30k + $6 one. Ranking is now (miles, tax): miles first, as the scarce currency, ties broken on cash out-of-pocket. P2 — the funding column unioned banks across every attached award, implying programs that fund hidden costlier offers also fund the displayed one (a 30k Amex winner beside a hidden 40k Chase offer rendered "Amex, Chase"). Now restricted to the awards actually shown. Five tests, all failing against the pre-fix code. Not fixed, filed: seats.aero "Premium" doesn't match the CLI's "Premium economy" column label (a canonical cabin enum is the real fix, wider than this change); non-USD taxes are printed as dollars and subtracted from USD cash; `_dedupe_per_leg` still keys on first-flight+date, so two connections sharing a first flight can collapse — same journey-identity weakness already fixed in the matcher, now visible one layer up. make check green: 587 tests, ruff + basedpyright clean.
Codex adversarial pass over `links.py` — the deep links the user clicks to go
book. Seven findings; this is the one with a clean, contained fix.
`extract_pin_segments_from_slice` dated the last segment of a slice by ARRIVAL
whenever the slice spanned midnight. A nonstop trivially satisfies "last
segment", so an overnight nonstop was pinned to its arrival date: BA178
JFK->LHR departing 2026-12-31 and landing 2027-01-01 decoded with segment date
2027-01-01 — the wrong day, and across a year boundary. The user follows the
link and searches a date they never chose.
A segment is dated by when it DEPARTS. That equals the arrival date only for
the last leg of a genuine multi-segment slice that crosses midnight, which is
now the precise condition. Verified by decoding the emitted URL: overnight
nonstop 2026-12-31; overnight connection 2026-12-31 then 2027-01-01; same-day
nonstop unchanged. Also removed three duplicated lines above the loop.
Three tests; the overnight-nonstop one fails against the pre-fix code.
Filed, NOT fixed — these need design decisions wider than this branch:
- `--pick N` indexes `matrix_res[0]` while the table renders merged
Matrix+Google rows, so a cheaper Google-only row shifts what N means and
the link pins a different itinerary. Highest-severity of the set.
- Google pinned links encode children and infants as adults (protobuf field
8 is [1,1] for two adults AND for one adult plus a child).
- Two repeated --slice legs fold into a round trip, dropping the second
route entirely; a three-leg itinerary silently loses leg 3 and is still
labelled pinned.
- Matrix links drop routing language, extension codes and arrival-date
intent; a nonstop-only Google link is byte-identical to an unconstrained
one.
make check green: 590 tests, ruff + basedpyright clean.
The highest-severity finding from the codex pass over the link builders, and the one I had wrongly filed as "needs a design decision". It does not — the merged rows already carry their own itineraries, so this was a wiring gap. `_render_merged` numbers rows 1..N off the merged GF+Matrix list. `_emit_urls` indexed `matrix_res.solutions` — a different sequence. A Google-only row is cheaper, so it sorts first and shifts every Matrix row down one: `--pick 2` then emitted a link to the itinerary displayed at row 1. That link is the handoff to an actual purchase, so the failure is a user booking a flight they did not select, with nothing anywhere reporting an error. The pin source is now built from the rendered rows. `session` and `solution_set` are carried across, since Matrix pinned URLs need those server-generated identifiers and would otherwise degrade silently to a plain deep link. Awards-only runs render no merged table, so they keep indexing Matrix's own ordering — which is what the user saw in that mode. Extracted `_overlay_awards` to keep `_run_enriched_path` under the statement limit rather than suppressing the lint: the function sat exactly at 50 before this change, so the overflow is mine, not pre-existing. One test, pinning both the row order and the identifier passthrough. make check green: 591 tests, ruff + basedpyright clean.
Pushed the previous commit before reading the gate output; basedpyright flagged an Optional access and the private-symbol import. Both fixed — the suppression names the rule and its reason, per AGENTS.md Principle 2. make check green: 591 tests.
The response cache under every network call had no expiry, no eviction, and cached application-level errors. A live cache inspected during review: 111 entries spanning 77 days, newest 16 hours old — so it had effectively never served a fresh hit — and 19 of those were memoized failures, including `Internal server error` and `QPX Warning. Bad route specification`, each replayed forever because nothing expired. Storage moves to diskcache; policy stays ours. Choosing the library was measured, not assumed. My first instinct was to keep hand-rolling, on the grounds that diskcache is sync-only and a thread hop would defeat a dedupe cache. That was wrong: through `anyio.to_thread` (which reuses its worker pool) a set costs ~0.15 ms against ~0.12 ms for the raw JSON write it replaces — noise beside a 30-45 s Matrix call. The upstream's own async caveat (grantjenks/python-diskcache#116) is about `asyncio.run()` spawning a fresh executor per call, which we don't do. In exchange: per-entry TTL, LRU eviction and a size cap from a tested implementation, in a project that had already shipped two expiry bugs in hand-rolled caches this month (mtime-refresh, restamp-on-read). What no cache library could have done for us is the error check. Matrix signals failure with HTTP 200 plus `{"error": ...}`, so neither a status code nor an HTTP-aware policy can see it — `is_cacheable_body` needs Matrix's own shape. That predicate was always going to be ours, which is why "the library doesn't fix the real bug" was not an argument against the library. TTL is 15 minutes: Matrix is research-only (booking hands off to the Google Flights link), so a slightly stale fare costs a re-search, never a bad purchase. Long enough to dedupe one command's repeat queries, short enough that nobody acts on old pricing. First test file for `_http.py`, which had none despite sitting under every request. Verified directly against the pre-change module: two identical requests returning an error body made 1 network call before (memoized, `cache_hit` logged) and 2 after. make check green: 597 tests, ruff + basedpyright clean.
Answering the one open product question: both upstreams support multi-city (fli exposes TripType.MULTI_CITY=3, Matrix's SPA takes a slice per leg), so there was never a reason to reject the input — we were just encoding it wrong. Three bugs, all reproduced by decoding the emitted link: Matrix — `n == 2` meant round trip unconditionally, without checking the legs actually invert. Round-trip's SPA encoding folds both legs into ONE slice carrying two dates, which structurally cannot express a second route, so the second leg was DELETED: SFO->JFK plus LAX->HNL encoded as SFO->JFK with a return date, LAX and HNL gone. Byte-identical output to a real SFO<->JFK round trip. Now `_is_inverse_pair` requires the return to depart where the outbound landed and land where it started; anything else is multi-city, which keeps a slice per leg. Multi-airport legs count as inverse only on an exact set match. Google, trip type — `>= 2 slices` meant round trip, so a three-leg itinerary was labelled one and Google read only the first two slices. Leg 3 vanished from a link we still described as "pinned". Now one-way / round-trip / multi-city by slice count. Google, passengers — field 8 is a repeated varint carrying each occupant's TYPE, but we emitted a bare `1` per occupant. `1` is ADULT, so a pinned link for 1 adult + 1 child searched and priced as 2 adults: a different, costlier itinerary than the row the user selected. Enum values taken from fast_flights' generated protobuf (flights_pb2.Passenger) rather than guessed: ADULT=1, CHILD=2, INFANT_IN_SEAT=3, INFANT_ON_LAP=4. Six tests; four fail against the pre-fix code. make check green: 603 tests, ruff + basedpyright clean.
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.
The bug
An unbookable award price could render on a cash row — e.g. a Delta SkyMiles price shown on American metal, which cannot be ticketed (no DL/AA interline award agreement). A user could try to book it.
Found in live output:
MSY→MIA 2026-09-09T10:45carries bothAA3539(American) andDL1424(Delta). The route+time codeshare fallback inpp/match.pykeyed on(origin, destination, departure-minute)— documented as "a near-tight identity," which it is not — so both awards attached to the same cash itinerary and the renderer's lowest-miles pick surfaced Delta's9.1kon the American row.What this took
The initial fix was wrong, and so were the next three. Five review rounds (four Claude personas + two cross-model codex passes) each broke the central claim after it had been declared fixed and verified live. Every finding was reproduced through the real renderer before being accepted; every fix has a test that fails against the prior commit.
_pick_metal: exact-carrier-wins, then single-partner-or-nothing_drop_contradicted/_narrow; segment identityThe shape of the fix
Three match keys — matched-ID,
(flight#, date, route), and(route, departure-minute)— are discovery mechanisms. None is an identity: each can return several genuinely different aircraft. Resolution is now a single judgment over their union:The last distinction is subtle and was itself a bug: eliminating a contradicted candidate is sound evidence, but letting arrival select across carriers lets a partner that merely omits its arrival lose to an unrelated carrier that happens to publish a matching one.
Also fixed along the way
Zsuffix is mislabelled — those timestamps are local, not UTC. Honouring it gives ~12.1h JFK→LHR against a real ~7h. Normalized at the provider boundary.httpx.HTTPStatusErrorleaked past the PP catalog boundary, contra AGENTS.md Principle 1. NowPPApiError.Verification
basedpyrightstrict —make checkgreen.AA3539economy9.1k Delta→15.5k American. Rows moved three times across rounds, meaning earlier "fixes" were still rendering wrong prices.Known residual risk
_PARTNER_GROUPS/_REGIONAL_OPERATORSare hand-curated. They now only arbitrate when arrival is missing on both sides — measured at 0 of 471 live awards, so effectively never — but a stale entry would fail silently. Every table-arbitrated match now logsaward_match_via_partner_tableat debug so that assumption becomes evidence. The principled replacement isSlice.legs[i].operating_carrierground truth, which only the gflight backend populates today.