Repository navigation
fix: send quote_id on negotiation messages and assert the booked price equals the agreed price - #133
Conversation
…e equals the agreed price Two independent halves of one defect: a buyer negotiates a price down, books, and is billed the seller's standard price with "Negotiation succeeded" in the log. Correlation on the wire. DealBookingRequest carries a quote_id and no negotiation_id, so the quote is the only key the booking contract offers for tying an agreed price to the deal being struck. The buyer's outbound NegotiationMessage sent only proposal_id/negotiation_id, so the seller had nothing to index the negotiation against for the buyer's own flow. The quote being negotiated is now threaded through NegotiationSession and sent as quote_id on the open, every counter, the accept and the reject. The shared model's validator rejects only the all-None case, so the three ids are not mutually exclusive: sending quote_id alongside proposal_id is valid and leaves the seller's proposal-first key derivation untouched. The stale "exactly one of" comment on the wire builder is corrected. Assertion, not a bound, on the booked price. The only post-negotiation guard was `final_cpm is None or final_cpm > max_cpm`, a ceiling test that passes for ANY price under the buyer's limit including the seller's undiscounted list price, so a negotiation whose agreed price was silently dropped booked at list price and reported success. The buyer now also asserts the bookable quote's final_cpm EQUALS the agreed price, before the quote enters the ranked list and therefore before anything can book it, so a mismatch refuses to book rather than surfacing once money is committed. The two guards stay separate and are reported separately (requote_above_ceiling vs requote_price_mismatch): one answers "never pay more than our limit", the other "never pay a price we did not agree to". The ceiling check runs first because it is the more specific diagnosis and because it also disposes of the unpriced case. Equality is compared with a half-a-cent tolerance. CPMs cross the wire as Money micros and come back as floats, so a bit-exact comparison would trip on representation noise; half a cent is orders of magnitude below any real pricing discrepancy.
…lerance The post-negotiation price assertion compared CPMs as floats with a $0.005 tolerance. The mechanism that window was sized against has since been measured: money crosses the wire as integer micros, both sides convert with round(v * 1e6) out and m / 1e6 back, and the round trip is lossless across every two-decimal CPM from $0.01 to $200 -- delta exactly 0.0. The largest delta the path can produce at all is half a micro, so the window was four orders of magnitude wider than the noise it defended against. That gap was not free. Cost is computed from the booked price, so a difference inside the window moved real money ($5 per million impressions) with no warning, inside a guard whose entire purpose is that silence is the bug. Compare integer micros instead, via the existing _money_from_float mapper. Two prices are equal if and only if they are the same number of micros -- the same resolution the contract itself uses. This absorbs the only delta the path can actually create (the sub-micro quantization of a ragged float, which a bit-exact != would turn into a spurious walk-away) while rejecting a real difference of one micro and upward. test_sub_cent_float_noise_is_not_a_mismatch tested the opposite of what it claimed: AGREED + 0.001 is a tenth of a cent, two thousand times the largest achievable representation delta, so it asserted that a genuine price difference must be booked. Replaced with three tests that discriminate: a ragged agreed price returning micros-quantized still books, one micro of difference is refused, and a tenth of a cent is refused.
Sirajmx
left a comment
Sirajmx
left a comment
There was a problem hiding this comment.
Independently verified the micros-equality math rather than trusting the write-up: swept every
2-decimal CPM from $0.01 to $200.00 through_money_from_float's round trip — max delta exactly
0.0, confirming "lossless" precisely, not approximately. The$34/3ragged-price example lands
on11333333micros on both sides and books; one-micro-off and tenth-of-a-cent-off are both
correctly refused. Negative control, run myself: restoring the old$0.005tolerance fails
exactly the two tests the PR predicts (test_one_micro_price_difference_is_a_mismatch,
test_a_tenth_of_a_cent_difference_is_a_mismatch), 14/16 pass otherwise — matches exactly.Confirmed the validator claim directly too:
NegotiationMessage(proposal_id=..., quote_id=...)
together validates fine, an all-Nonemessage is genuinely rejected — the old "exactly one of"
docstring was wrong, the correction is accurate.New test file: 16/16 passed. Full suite: 3638 passed, 83 skipped (you report 3641/80 — the small
gap is environmental on my end, no Redis service / Python 3.12 vs your 3.11+Redis CI, zero
failures either way). Ruff clean. Fast-forwards cleanly onto currentmain, zero conflicts.The check that matters most: real cross-repo interop against seller #96, not assumed
compatibility. Built the exact wire JSONto_wire_negotiation_messageproduces and ran it through
a real quote → real proposal → counter → accept flow against #96's actual handler. Result:200
throughout, and the stored negotiation correctly retains thequote_id, with the
negotiation_by_quotepointer correctly resolving to the proposal id at accept time. Also
confirmed additive safety against current sellermain(pre-#96): the same message with
quote_idpresent doesn't break anything there either, it's just inert until #96 lands.The held-back re-quote removal is correctly scoped out — shipping it ahead of #96 being deployed,
not merely merged, would book at the pre-negotiation price, a worse failure than today's.
Approving — recommend merging this together with seller #96, since neither alone closes the real
mispricing bug.
The defect
A buyer negotiates a price down, books, and is billed the seller's standard price, with "Negotiation succeeded" in the log. Two independent faults combine. This PR fixes both buyer-side halves.
1. Send the quote id on negotiation messages
DealBookingRequestcarries aquote_idand nonegotiation_id, so the quote is the only key the booking contract offers for tying an agreed price to the deal being struck. The buyer's outboundNegotiationMessagesent onlyproposal_id/negotiation_id, so the seller had nothing to index the negotiation against for the buyer's own flow, which is the flow that matters.The quote being negotiated is now threaded through
NegotiationSessionand sent asquote_idon the open, every counter, the accept and the reject.Checked before assuming: the shared
NegotiationMessagevalidator rejects only the all-None case (iab_agentic_primitives/protocol/negotiation.py), sonegotiation_id/proposal_id/quote_idare not mutually exclusive. Sendingquote_idalongsideproposal_idvalidates cleanly and leaves the seller's proposal-first key derivation untouched, so this is additive with no behaviour change for the seller today. The wire builder's docstring, which claimed "exactly one of", was wrong and is corrected.Tests assert on the real serialized request body (captured through
httpx.MockTransport, with a genuinehttpx.AsyncClientdoing the serialization) and then re-validate that body with the sharedNegotiationMessage, so a field rename on either side fails the test instead of passing it.2. Assert the booked price equals the agreed price
The only post-negotiation guard was
final_cpm is None or final_cpm > max_cpm: a ceiling test that passes for any price under the buyer's limit, including the seller's undiscounted list price. A negotiation whose agreed price was silently dropped therefore booked at list price and reported success. This is the change that converts that whole class of defect from silent to loud.The buyer now also asserts the bookable quote's
final_cpmequals the agreed price. The assertion sits before the quote enters the ranked list, and therefore before anything can book it, so a mismatch refuses to book rather than surfacing once the money is committed.The two guards stay separate and are reported separately,
requote_above_ceilingvsrequote_price_mismatch: one answers "never pay more than our limit", the other "never pay a price we did not agree to". They are not collapsed into one check. The ceiling check runs first because it is the more specific diagnosis and because it also disposes of the unpriced (final_cpm is None) case; the code says so, so a future reorder is not silent.Equality is compared in integer micros, with no tolerance window. Both sides are quantized with the buyer's existing
_money_from_floatmapper and the resultingamount_microsare compared as integers, so two prices are equal if and only if they are the same number of micros, which is the same resolution the contract itself uses.An earlier revision of this PR used a
$0.005float tolerance, on the theory that money crossing the wire as micros and returning as floats would trip a bit-exact comparison. That was measured rather than assumed, and it does not hold. Both sides convert withround(v * 1e6)out andm / 1e6back, the seller assigns an acceptedtarget_cpmverbatim with no arithmetic on it, and seller counters are already rounded to two decimals at the source. The round trip is lossless across every two-decimal CPM from$0.01to$200: delta exactly0.0. The largest delta the path can produce at all is half a micro,$0.0000005, from the micros quantization itself. The window was four orders of magnitude wider than the noise it was defending against.That gap was not free. Cost is computed from the booked price, not the agreed price, so a difference inside the window moved real money,
$5per million impressions, while the negotiation record stored the agreed price and nothing reported the disagreement. A silent band is exactly what this guard exists to eliminate.Micros rather than a bit-exact
!=on floats, because!=is correct today only because both sides happen to use the same two operations, and it would refuse a legitimate booking if the agreed price ever carried more than six decimal places: the quantization then produces a real~1e-7delta, and the buyer would walk away from a good deal in a way that looks like a seller outage. Quantizing first absorbs that one genuinely representational case and nothing else.Deliberately NOT in this PR: stop the
target_cpmre-quoteThe buyer currently books a negotiated price by re-quoting with
target_cpm=<agreed>and relying on the seller honouring an acceptable target as the quote'sfinal_cpm. The right end state is to delete that re-quote and book the original quote, letting the seller strike the agreed price at booking time, which is what the seller's own code always intended.That change is held back on purpose. It depends on the seller being able to resolve a negotiation from a quote (recording the
quote_idon the stored negotiation and indexing by it) being deployed, not merely merged. Shipping it ahead of the seller would book at the pre-negotiation price, which is a worse failure than today's. So it is a separate PR, sequenced behind the seller's.This PR is the safe half, and it is the half that is worth landing on its own merits: the two changes here are additive, and the equality assertion is exactly the tripwire that would catch the sequencing going wrong.
Testing
tests/unit/test_negotiation_quote_correlation.py: 16 tests. Wire-body assertions for open/counter/accept/reject plus the full auto-negotiate loop, the additive no-quote case, the agreed-price equality assertion, and the ceiling guard's own independence.target_cpm=agreedand the seller honours it, so the price comes back at the agreed price and equality holds.test_ragged_agreed_price_round_tripped_through_micros_booksagrees at34/3(11.333333333333334,11_333_333micros) and has the seller return the micros round trip11.333333: not equal as floats, the same money, so it books.test_one_micro_price_difference_is_a_mismatchreturns11.333334, one micro up and well under the ceiling, and it is refused.test_a_tenth_of_a_cent_difference_is_a_mismatchcovers the case the old tolerance waved through.test_sub_cent_float_noise_is_not_a_mismatchis gone, deliberately. It asserted thatAGREED + 0.001must book, calling it representation noise. A tenth of a cent is a thousand micros, two thousand times the largest delta this path can produce, so the test asserted that a genuine price difference must be accepted, which is the opposite of the guard's purpose. Its replacement asserts that input is refused.$0.005tolerance fails both mismatch tests; switching to a bit-exact float!=fails the ragged-price test. Each of the three tests therefore discriminates against a specific wrong implementation, and none remains in the tree.uv run pytest tests/ -q→ 3641 passed, 80 skipped.ruff check src/ tests/andruff format --check src/ tests/both clean.