Skip to content

fix(search): decode wrb.fr chunks by JSON grammar, and surface Google's error envelopes - #224

Open
olivierbarbosa wants to merge 3 commits into
punitarani:mainfrom
olivierbarbosa:fix/wire-framing-and-error-envelope
Open

fix(search): decode wrb.fr chunks by JSON grammar, and surface Google's error envelopes#224
olivierbarbosa wants to merge 3 commits into
punitarani:mainfrom
olivierbarbosa:fix/wire-framing-and-error-envelope

Conversation

@olivierbarbosa

@olivierbarbosa olivierbarbosa commented Aug 8, 2026

Copy link
Copy Markdown

Two independent defects in fli/search/_wire.py. Both surface to the user as No flights found, which is why they are easy to misread as "this route has no service". The second one is issue #223.

1. Chunk framing desynchronises on non-ASCII responses

iter_wrb_chunks treats the decimal header preceding each chunk as a UTF-8 byte count and slices the body by length - 1 bytes.

Measured on a GetShoppingResults response captured 2026-08-07 (CDG→JFK; 91,043 characters for 91,120 bytes, i.e. 77 multi-byte characters in accented airport and carrier names), the announced length matches neither reading: it counts the chunk plus both surrounding newlines, in characters.

On an ASCII-only body the two conventions coincide exactly — which is why the checked-in fixture never exercised the difference. tests/search/fixtures/booking_results_aa_jfk_lax.bin contains 0 multi-byte characters across 48,444, and header − chunk == 2 for all four of its chunks. A single accent is enough to desynchronise the stream.

With any non-ASCII content the slice comes up short. In order:

  1. the chunk fails to parse and is discarded — Unterminated string starting at: line 1 column 17
  2. the cursor is left inside the chunk, so the next header read is garbage — Malformed length header at offset N; truncating chunk stream
  3. parse_first_wrb_payload returns None and the search reports no flights

Fix: stop using the header as a frame delimiter. json.JSONDecoder().raw_decode() derives the end of each chunk from the JSON grammar, which is correct under either convention and needs no assumption about Google's framing. The headers are still used to resynchronise after an unparseable chunk, so the previous "skip the bad chunk, keep the rest" behaviour is preserved.

2. A rejected request is indistinguishable from an empty result set

When Google declines a request it answers HTTP 200 with a payload-less row:

)]}'

[["wrb.fr",null,null,null,null,[13]],["di",39],["af.httprm",38,"-196…",5]]

_chunks_from_outer skips that row (no inner payload string), parse_first_wrb_payload returns None, _fetch_flights returns None, and the CLI prints No flights found.

Observed codes line up with gRPC canonical statuses: 3 INVALID_ARGUMENT for a payload Google cannot decode, 13 INTERNAL for a request it declines to serve. A genuinely empty payload gets a plain HTTP 400 instead, so the three cases are distinguishable.

Fix: detect the envelope and raise the new SearchBackendError, carrying error_code. It subclasses SearchClientError, which fli/cli/errors.py already routes — the user now sees Search failed. Google Flights returned error 13 (INTERNAL) instead of results. No change to the CLI, and no change to the JSON error contract.

SearchParseError lives in flights.py rather than exceptions.py, so importing it from _wire.py would introduce a cycle; hence the new exception in exceptions.py.

Tests

All offline, over synthetic responses — no requests to Google.

  • TestNonAsciiFraming (4): an accented single chunk is not dropped; an accented chunk does not desynchronise a multi-chunk stream; ASCII still parses; a header matching neither convention is ignored.
  • TestErrorEnvelope (4): codes 13 and 3 raise with the status name; an unmapped code still raises with the number; a short payload-less row remains a silent skip.
  • TestBackendErrorEnvelope in tests/search/test_search_flights.py (2): search() raises through the full stack; a genuinely empty response still returns None.

The existing _multi_chunk helper (byte-counted headers) is kept beside the new _google_framed helper (character-counted), pinning the reader as correct under both conventions. 7 of the new tests fail before the fix. The captured multi-chunk fixture test passes before and after.

Full suite: no regression against baseline. The remaining failures are the live tests that require a working Google endpoint, identical before and after this change.

The two fixes are independent — happy to split them into separate PRs if you prefer.

Note on the underlying outage

Separately from these two defects, GetShoppingResults currently rejects requests from any plain HTTP client. The endpoint now requires an X-Goog-BatchExecute-Bgr header that the page's JavaScript produces; without it the response is the error envelope above, which is how we ran into defect 2. That part is outside the scope of this PR — the point here is only that the library should say so rather than report an empty result.

Note on the TypeScript port

fli-js/src/search/wire.ts is a direct port and carries both defects (chunkBytes = Math.max(length - 1, 0), and row.length < 3 skipping the error envelope). Not touched here — happy to follow up if useful.


Investigated, developed and debugged with Claude Opus 5, in collaboration.

Greptile Summary

The PR replaces byte-count-based wrb.fr slicing with JSON-grammar decoding and introduces a typed error for HTTP 200 backend rejection envelopes.

  • Parses ASCII and non-ASCII chunks independently of Google's announced frame length.
  • Attempts to resynchronize at the next chunk header after malformed JSON.
  • Exports SearchBackendError and preserves backend status codes and canonical names.
  • Adds synthetic parser and full-search regression coverage.

Confidence Score: 4/5

The PR appears safe to merge after the non-blocking test-helper typing violations are addressed.

The parser and backend-error changes preserve the established search paths with focused regression coverage; the only accepted issue is incomplete typing in newly added test helpers.

Files Needing Attention: tests/search/test_search_flights.py, tests/search/test_wire.py

Important Files Changed

Filename Overview
fli/search/_wire.py Replaces length-based framing with grammar-based decoding and detects backend error envelopes; no blocking defect identified.
fli/search/exceptions.py Adds a compatible SearchClientError subclass carrying the backend status code.
fli/search/init.py Exposes SearchBackendError through the package's public search API.
tests/search/test_wire.py Adds broad framing and envelope coverage, but two new helpers omit required type annotations.
tests/search/test_search_flights.py Verifies full-stack envelope propagation and genuine empty results, but the new fake response helper is untyped.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Google Flights HTTP response] --> B[Strip XSSI prefix and framing noise]
    B --> C[JSONDecoder.raw_decode outer chunk]
    C -->|Malformed| D[Search for next chunk boundary]
    D --> C
    C -->|Decoded| E{wrb.fr row has inner payload?}
    E -->|Yes| F[Decode and yield inner JSON]
    E -->|No, status envelope| G[Raise SearchBackendError]
    E -->|No status| H[Skip row]
Loading

Fix All in Cursor Fix All in Claude Code Fix All in Codex

Prompt To Fix All With AI
### Issue 1
tests/search/test_search_flights.py:365
**Test helpers lack type annotations**

The new `_fake_post` helper omits parameter and return annotations, as do `_google_framed` and `_error_envelope` in `test_wire.py`, weakening static type checking and violating the repository's full-annotation standard.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Reviews (1): Last reviewed commit: "fix(search): decode wrb.fr chunks by JSO..." | Re-trigger Greptile

Greptile also left 1 inline comment on this PR.

Context used:

…nvelopes

Two independent defects in the batchexecute reader, both of which end as
"No flights found" for the user.

1. Chunk framing broke on non-ASCII responses.

   iter_wrb_chunks trusted the decimal length header preceding each chunk
   and sliced the body by that many UTF-8 bytes. Google counts the chunk
   plus its two surrounding newlines in *characters*, so the two agree
   only while the response is pure ASCII — which every checked-in fixture
   happens to be. Add one accented airport name and the reader slices
   short: the chunk fails to parse and is discarded, the cursor lands
   mid-JSON, and the stream is truncated with "Malformed length header at
   offset N". A Paris-New York search returns nothing at all.

   Rather than swap one guess about Google's convention for another, stop
   using the header as a frame delimiter: json.JSONDecoder.raw_decode
   locates the end of each chunk exactly, under either convention. The
   headers are still used to re-synchronise after an unparseable chunk, so
   the previous "skip the bad chunk, keep the rest" behaviour is retained.

2. A rejected request read as an empty result set.

   When Google declines a request it answers HTTP 200 with a payload-less
   row -- ["wrb.fr",null,null,null,null,[13]] -- carrying a gRPC status
   code (3 INVALID_ARGUMENT, 13 INTERNAL). The reader skipped that row as
   "no inner payload", parse_first_wrb_payload returned None, and the CLI
   printed "No flights found", indistinguishable from a route with no
   service.

   Detect the envelope and raise the new SearchBackendError, which carries
   the status code and subclasses SearchClientError, so the CLI's existing
   error path reports "Google Flights returned error 13 (INTERNAL)".

Both paths are covered by offline tests over synthetic responses; no
network access is involved.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Btxbg4GF1AdfozL1amqzbJ
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

sf = SearchFlights()

def _fake_post(url, data, **kwargs): # noqa: ANN001
return type(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Test helpers lack type annotations

The new _fake_post helper omits parameter and return annotations, as do _google_framed and _error_envelope in test_wire.py, weakening static type checking and violating the repository's full-annotation standard.

Context Used: CLAUDE.md (source)

Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/search/test_search_flights.py
Line: 365

Comment:
**Test helpers lack type annotations**

The new `_fake_post` helper omits parameter and return annotations, as do `_google_framed` and `_error_envelope` in `test_wire.py`, weakening static type checking and violating the repository's full-annotation standard.

**Context Used:** CLAUDE.md ([source](https://github.com/punitarani/fli/blob/main/CLAUDE.md))

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Fix in Cursor Fix in Claude Code Fix in Codex

CLAUDE.md requires full type annotations. The three helpers added by this
PR omitted them; the neighbouring pre-existing helpers are left untouched
to keep the diff focused.
@olivierbarbosa

Copy link
Copy Markdown
Author

Fixed in ee01284 — thanks.

Annotated the three helpers this PR adds: _google_framed, _error_envelope in test_wire.py, and _fake_post in TestBackendErrorEnvelope.

I left the neighbouring pre-existing helpers (_single_chunk, _multi_chunk, and the _fake_post in TestSearchErrors) untouched, to keep the diff scoped to this change. Happy to annotate those too in a follow-up if you'd like the file made consistent.

Worth noting for anyone reading the rule: ANN isn't in the ruff select list, so this isn't lint-enforced — it comes from CLAUDE.md, which does ask for full annotations. The # noqa: ANN001 comments already in the file are therefore inert.

ruff check and ruff format --check are clean. The 10 new tests pass; the 3 remaining failures in test_search_flights.py are the live network tests and fail identically on main.

@alexechoi

Copy link
Copy Markdown

Really nice fix — grammar-based decoding is the right call here. While reverse-engineering GetExploreDestinations for #226 I hit exactly the failure modes this PR addresses, so I tested it against my captures. A few observations:

1. Raising from inside the generator is position-dependent, and full drains lose partial results.
parse_first_wrb_payload returns at the first chunk and abandons the generator, so an error row after chunk 0 is never surfaced. Conversely, consumers that drain fully (list(iter_wrb_chunks(...)) in booking options, and the explore loop in #226) lose everything already accumulated when a trailing error row raises — I confirmed a body of [good_wrb_row, error_row] yields the good chunk and then raises, so the caller's accumulator goes out with the exception. Recording the code and raising only when no chunk was yielded would make both consumption modes agree and preserve partial data. (There's currently no test covering an error row that follows a valid chunk.)

2. Worth carrying the error detail, not just the code.
A live rejection I captured yesterday from GetExploreDestinations has the richer shape:

["wrb.fr",null,null,null,null,[13,null,[["type.googleapis.com/travel.frontend.flights.ErrorResponse",[[null,null,0,"<request-id>"],0]]]]]

_error_code handles this correctly (not just the bare [13] form in the tests 👍), but the type URL and request id are discarded. In my case error 13 didn't mean "Google declined to serve" — it meant "you omitted the x-same-domain/origin header", and the detail block was the debugging hint. Even a truncated repr in the exception message would help a lot.

3. Fixture evidence for the framing claim.
I audited the committed fixtures: the only header-framed one (booking_results_aa_jfk_lax.bin) is 100% ASCII, so header − chunk == 2 under both the byte and char conventions — nothing currently in the repo can confirm or refute the char-count observation. Committing a non-ASCII, header-framed capture alongside this PR would let future readers reproduce the reasoning (and is also the strongest argument for this PR: grammar decoding is correct either way).

Tiny nit: isinstance(status[0], int) accepts bool, and a code of 0 (OK) would raise a spurious "error 0" — a code > 0 and not isinstance(code, bool) guard closes both.

FYI there's a one-assertion interaction with #226 (test_error_13_body_returns_none expects None where this raises) — trivial for whichever of us merges second; happy to adapt on my side.

Review feedback from @alexechoi on punitarani#224.

Raising from inside the generator made the outcome depend on how far the
caller drained it: `parse_first_wrb_payload` returns at chunk 0 and never
saw a trailing error row, while the full drains (`get_booking_options`,
and the explore loop in punitarani#226) lost every chunk already accumulated to the
exception. The status is now recorded and raised only when the body is
exhausted without a single usable chunk, so both consumption modes agree
and whatever Google did send is always delivered. A swallowed error is
logged with the chunk count.

Also carry the status detail. A live GetExploreDestinations rejection has
the shape `[13, null, [["type.googleapis.com/….ErrorResponse", [[…,
"<request-id>"], 0]]]]`, where error 13 meant "you omitted the
x-same-domain/origin header" rather than "Google declined to serve" — the
type URL and request id were the debugging hint, and both were discarded.
They are now surfaced in the message and on a new `error_detail`
attribute, capped at 200 characters.

Finally, only a strictly positive int counts as a rejection: `0` is
gRPC's OK and raised a spurious "error 0", and `bool` slipped through as
a subclass of `int`.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants