Conversation
Booking resolved an accepted negotiation with
storage.get_negotiation(request.quote_id). That only ever hits for a
quote-led negotiation, which the router's key derivation happens to
store under the quote id. Our buyer is proposal-led, so its negotiation
is stored under a prop- id, the lookup missed every time, and the deal
was booked at the seller's standard price while the log reported the
negotiation as successful.
NegotiationHistory now retains the quote_id that already arrived on
NegotiationMessage and was being dropped. An accepted negotiation
indexes itself as negotiation_by_quote:{quote_id}, holding the key its
record is stored under, and booking resolves through that pointer. The
direct read stays as a fallback so quote-led negotiations recorded
before the index existed are still honoured. A pointer rather than a
second copy of the record: rounds are appended to it, so two copies
would drift on the first append.
An accepted negotiation carrying no agreed price is now refused with
negotiation_price_unresolved instead of quietly booking at the
un-negotiated quoted price. A record in that state is corrupt, and
list-pricing it is the worst available outcome.
The negotiation storage key is also resolved against the store rather
than read off whichever ids a given message happened to carry, so a
continuation leading with a different id continues the same negotiation
instead of silently restarting it at round one. Which id a negotiation
is stored under is unchanged.
Behaviour change: bookings that silently ignored an accepted
negotiation now honour it, so a booked final_cpm can differ from the
quoted one and its rationale says why.
…ct the redacted guardrails The status route's rounds were serialized whole as raw dicts, and each internal round carries cumulative_concession_pct, computed as (base_price - counter_price) / base_price — so seller_price / (1 - cumulative_concession_pct) reconstructed the seller's base_price EXACTLY, handing back the anchor the top level had just dropped. Round rationale on a below-floor offer also states the floor in prose. - NegotiationStatusResponse.rounds is now list[NegotiationRoundView], a whitelist of round_number / buyer_price / seller_price / action / timestamp. concession_pct, cumulative_concession_pct and rationale do not cross the wire. Excluding rationale here is boundary filtering on this read endpoint only; whether the engine should put the floor into rationale text at all remains an open engine-side decision. - The service keeps projecting full rounds: terminal_round_response reads rationale from them internally, so the filter belongs at the response model, which is the boundary. - New test drives the REAL engine (build_negotiation_engine -> start_negotiation -> evaluate_buyer_offer -> record_round) with a below-floor offer so the round genuinely carries the nonzero concession and the floor-naming rationale, then asserts neither survives serialization. The original fixture's default 0.0 concession and hand-written rationale could not exhibit either leak. Negative control: reverting rounds to list[dict] fails this test naming cumulative_concession_pct and rationale. - NegotiationStatusResponse declares quote_id (Optional, default None) so the projection #96 adds is passed through instead of silently stripped by the pinned response model. - docs/api/openapi.json regenerated via scripts/generate_openapi.py so the drift guard arriving in #83 passes over the merged pair.
A negotiation message may name the quote its agreed price will apply to;
an accepted negotiation is then indexed by negotiation_by_quote:{quote_id}
and rewrites the booked final_cpm when that quote is booked. The quote_id
was taken off the wire unchecked, so buyer A could negotiate its own
proposal down, accept while naming buyer B's quote id, and B's booking
would silently carry A's price.
Binding a quote_id onto a negotiation (counter_proposal and
apply_terminal_action, covering both the message's quote_id field and
quote-led opens) now requires that the caller own the quote, using the
same ownership record the booking route enforces (the quote-history
buyer_id written from the verified pricing key at issue time):
- An unowned quote reads as 404 quote_not_found, byte-identical to a
nonexistent one, so the route is not an existence oracle over other
tenants' quote ids (same pattern as the change-request routes).
- A quote_id resolving to neither a quote nor its history record is
refused the same way: it could never be booked, so binding it is a
buyer bug or an attack, and dropping it silently would hide both.
- The route's auth is optional and anonymous identity fields are
self-asserted, so only a key-derived identity counts for ownership;
every other caller is treated as public, fail-closed.
- No recorded buyer (legacy) or a recorded 'public' buyer keeps today's
behavior: nothing buyer-specific to enforce, binding allowed.
Seven new tests, including the end-to-end control: the cross-tenant
accept is refused, no pointer is written, and the victim's booking
carries the quoted price. Verified by disabling the check and watching
the five enforcement tests fail.
…ct the redacted guardrails The status route's rounds were serialized whole as raw dicts, and each internal round carries cumulative_concession_pct, computed as (base_price - counter_price) / base_price — so seller_price / (1 - cumulative_concession_pct) reconstructed the seller's base_price EXACTLY, handing back the anchor the top level had just dropped. Round rationale on a below-floor offer also states the floor in prose. - NegotiationStatusResponse.rounds is now list[NegotiationRoundView], a whitelist of round_number / buyer_price / seller_price / action / timestamp. concession_pct, cumulative_concession_pct and rationale do not cross the wire. Excluding rationale here is boundary filtering on this read endpoint only; whether the engine should put the floor into rationale text at all remains an open engine-side decision. - The service keeps projecting full rounds: terminal_round_response reads rationale from them internally, so the filter belongs at the response model, which is the boundary. - New test drives the REAL engine (build_negotiation_engine -> start_negotiation -> evaluate_buyer_offer -> record_round) with a below-floor offer so the round genuinely carries the nonzero concession and the floor-naming rationale, then asserts neither survives serialization. The original fixture's default 0.0 concession and hand-written rationale could not exhibit either leak. Negative control: reverting rounds to list[dict] fails this test naming cumulative_concession_pct and rationale. - NegotiationStatusResponse declares quote_id (Optional, default None) so the projection #96 adds is passed through instead of silently stripped by the pinned response model. - docs/api/openapi.json regenerated via scripts/generate_openapi.py so the drift guard arriving in #83 passes over the merged pair.
Sirajmx
left a comment
Sirajmx
left a comment
There was a problem hiding this comment.
Reproduced the core defect end-to-end, independently, with my own fixture — not the PR's own
tests: built a proposal-led negotiation (prop-repro-1) namingquote_id=qt-repro-1, negotiated
it down from $15.00 to $12.82, accepted it, then booked the quote through the real
deal_service.book_deal. Confirmed the pointernegotiation_by_quote:qt-repro-1→prop-repro-1
(aprop-id, not aqt-id — exactly the case the oldget_negotiation(quote_id)lookup could
never hit), and the booked deal came back at $12.82, not the stale $15.00 quote, with the
correct "...Negotiated to $12.82 CPM (neg-...)" rationale.Ownership gate, verified independently: simulated an attacker naming a victim's quote id on a
fresh negotiation. Result: rejected with404 quote_not_found— indistinguishable from
nonexistent, as designed — and the negotiation itself was never persisted at all, not just the
binding dropped. Matches the PR's stated fail-closed design.Fix verified on a clean merge onto current
main(trivial CHANGELOG conflict from #94, resolved
by concatenation): new correlation test file 25/25 passed (18 correlation + 7 ownership, matches
the PR's count), full suite 1592 passed, ruff clean.Cross-repo, checked myself — this is the important operational finding: current buyer-agent
mainnever sendsquote_idat any of its 4 negotiation call sites (open/counter/accept/reject
innegotiation/client.py) — confirmed by reading every call site. So this fix is a no-op against
today's buyer traffic; the real-world mispricing bug stays live in production until both #96
and buyer #133 merge together — #133 is what starts sendingquote_idon those exact 4 calls.
Worth being explicit about this for merge urgency: #96 alone fixes nothing a live buyer will
notice yet.Design choices checked, not just read: Fault A2's
resolve_negotiation_key(try each supplied id
against the store, first hit wins) correctly prevents the silent-restart bug from a continuation
leading with a different id than the opener, and deliberately doesn't re-key storage — honestly
scoped as future structural work. The fail-loudnegotiation_price_unresolved(500) on an
accepted-with-no-price record is the right call over silently list-pricing a corrupt record.
Approving, and flagging as the one to prioritize in this batch.
This branch has not been deployed
2 participants
The defect
A buyer negotiates a price down, the negotiation concludes
accepted, the buyer books, and the deal is minted at the seller's standard price with the negotiation reported as successful.deal_service.book_dealhonoured an accepted negotiation by callingstorage.get_negotiation(request.quote_id). Negotiations are stored under whichever id the opening message led with, viarouters/negotiation.py'smessage.proposal_id or message.negotiation_id or message.quote_id.To be precise, because it matters: this block is not dead code in general. A quote-led negotiation, whose opening message carries only
quote_id, is stored under the quote id, and the booking lookup finds it. That path worked and still works. What is true is narrower and worse in practice: our buyer is proposal-led (it always sendsproposal_id, plusnegotiation_idon continuation, and neverquote_id), so its negotiation is stored under aprop-id.prop-andqt-ids can never collide, so for our buyer the lookup missed every single time, and a booked deal never carried a negotiated price.The correlation fix
The root cause is a dropped id.
NegotiationMessagecarries aquote_id; the storedNegotiationHistoryhad no such field, so the id arrived on the wire and was discarded, leaving nothing to correlate a negotiation with the quote a buyer actually books.DealBookingRequestcarries onlyquote_id, so the quote is the only handle the protocol offers.NegotiationHistory.quote_idnow retains it, populated wherever a negotiation is opened or a round recorded, and readable back viaget_negotiation_status. The first quote id seen wins, so a later round naming a different quote cannot silently move which quote the agreed price applies to.acceptedand carries a quote id, it writesnegotiation_by_quote:{quote_id}holding the key its record is stored under. Booking resolves through that pointer, so it works whichever id the buyer led with.Fault A2: key derivation
Found while correcting the framing above. The storage key was derived from whichever ids a given message happened to carry, so two messages in one negotiation could resolve to different keys. A negotiation opened quote-led is stored under the quote id; a continuation leading with
proposal_idresolved to a different key, found the stored proposal there, and opened a second negotiation at round one, discarding every concession already made. Our buyer escapes this only because it sendsproposal_idandnegotiation_idtogether soproposal_idkeeps winning. Luck, not design, and the same root cause as the main bug: identity derived from what the caller supplied rather than from the entity.The fix resolves the message to an existing negotiation first, trying each supplied id against the store and using whichever one actually resolves. A new negotiation is minted only when none of them does, and the mint path keeps the existing
proposal_id or negotiation_id or quote_idorder. Which id a negotiation is stored under is deliberately unchanged — re-keying storage onnegotiation_idis the structural work and is out of scope here.One honest limit, pinned by a test rather than left implicit: no negotiation is stored under its own
neg-id, so a message carrying onlynegotiation_idhas nothing to resolve against. That case is not a silent restart, since there is no proposal or quote under aneg-id either, so the round is refused with a 404 rather than quietly beginning again. Making it resolvable requires the re-keying above.Fail loudly instead of list-pricing
If a negotiation resolves as
acceptedbut has no usablerounds[-1].seller_price, booking now refuses withnegotiation_price_unresolvedrather than falling through and booking the stale quoted price. A negotiation that concluded accepted with no price is a corrupt record, and list-pricing it is the worst available outcome. The whole family of bugs here is silence, so every branch that can produce a surprising price should either say so in the record or refuse.The existing rationale suffix is kept as-is. It is already correct and it is what makes a booked record auditable without a second lookup.
Behaviour change
This is why the changelog entry is under
Changed, notFixed. Bookings that silently ignored an accepted negotiation will start honouring it, so a bookedfinal_cpmcan now differ from the quoted one. That is the point of the change, but it is a real change in booked prices and not a pure defect repair.Testing and negative controls
New file
tests/unit/test_negotiation_quote_correlation.py, 18 tests. The one that matters is end-to-end: negotiate overPOST /api/v1/negotiations/messagesproposal-led with aquote_id, accept, then book that quote throughPOST /api/v1/dealsand assert the deal'sfinal_cpmequalsrounds[-1].seller_priceread back from storage and that the rationale carries theNegotiated to $X CPMsuffix. It also asserts the agreed price actually differs from the quoted one, so the test cannot pass while doing nothing. The negative case is covered too: a quote with no negotiation books unchanged at the quoted price.Every behavioural change was verified by breaking it and watching a test fail, then restoring it and confirming the working diff was byte-identical to the pre-control snapshot.
quote_idon the historytest_open_records_the_quote_id_from_the_message,test_quote_id_survives_open_counter_accept,test_quote_id_is_not_overwritten_by_a_later_round,test_accept_writes_the_quote_pointernegotiation_by_quotepointertest_accept_writes_the_quote_pointer,test_negotiated_price_books_end_to_endtest_booking_resolves_a_proposal_led_negotiation,test_negotiated_price_books_end_to_end,test_accepted_negotiation_with_no_rounds_refuses_to_book,test_accepted_negotiation_with_a_null_seller_price_refuses_to_booktest_continuation_with_a_different_id_continues_the_same_negotiation, failing withcontinuation started a new negotiationon two differentneg-ids, which is the silent restart reproduced exactlytest_accepted_negotiation_with_no_rounds_refuses_to_book,test_accepted_negotiation_with_a_null_seller_price_refuses_to_bookFull unit suite: 1539 passed, up from 1521 on
main.ruff check src/clean,ruff format --check src/ tests/clean.One pre-existing test fixture needed a generic
get/setadded (test_audience_plan_validation.py), since booking now reads the KV path its hand-rolled fake did not implement. Eighteencoroutine was never awaitedRuntimeWarnings also disappeared: booking now type-checks the record it reads instead of comparing an AsyncMock's coroutine against a string.Quote binding is gated on ownership
The correlation index introduced above took
quote_idoff the wire unchecked, which would have let buyer A negotiate its own proposal down, accept while naming buyer B's quote id, and have B's booking silently carry A's price. Binding aquote_idonto a negotiation (incounter_proposalandapply_terminal_action, covering both the message'squote_idfield and quote-led opens) now requires that the caller own the quote, checked against the quote-historybuyer_idwritten from the verified pricing key at issue time — the same record the booking route enforces ownership with.quote_not_found, byte-identical to a nonexistent one, so the route is not an existence oracle over other tenants' quote ids (house pattern, PR fix: stop the unauthenticated change-request listing from returning every order's audit trail #98). The message is rejected outright rather than accepted with the binding dropped: a silently dropped binding hides a buyer bug or an attack.authentication_method == "api_key") counts for ownership; every other caller is treated as public, fail-closed. An anonymous caller asserting the victim's ids does not pass.publicbuyer keeps today's behavior: nothing buyer-specific was priced, so there is no ownership to enforce.Seven new tests, negative-controlled by disabling the check: the cross-tenant end-to-end (accept refused, no pointer written, victim books at the quoted price), the same-buyer flow booking the negotiated price with an ownership record present, the unresolvable id, the quote-led foreign open, the anonymous impostor, the public quote, and the context-less terminal bind failing closed.
Out of scope
No buyer-side change, and none of the structural work: the shared
Negotiationprimitive as the stored object,Moneymicros for negotiation prices, re-keying storage onnegotiation_id, or splitting the seller's private guardrails out of its stored blob.