Repository navigation
fix: stop disclosing the seller's floor price in chat and on every counter-offer - #97
Conversation
Sirajmx
left a comment
There was a problem hiding this comment.
Reproduced both leaks live, independently, on current
main: the chat REJECT branch genuinely
outputs "Our floor for this inventory is $33.11 CPM... Maximum 5 rounds reached..." — floor
value and round budget both present, built from a hand-constructed history/round, not the PR's
own fixture. Verified the fix live on a clean merge onto currentmain: the same inputs through
the fixed formatter now return "...we've gone as far as we can on price here..." — no floor
value, no word "floor" anywhere. New test file: 25 passed. Full suite: 1592 passed. Ruff clean.One thing checked hard before approving, since it looked like a possible miss: the shared
NegotiationRoundprimitive (used by the canonicalPOST /api/v1/negotiations/messagesroute)
genuinely declaresconcession_pct/cumulative_concession_pctas real fields — confirmed via
NegotiationRound.model_fieldsand thatextra: ignoredoesn't drop them, so they do reach that
wire, the same base_price-reconstruction math #95 fixed elsewhere. Not a miss in #97, though —
#95's own body already names this exact gap and calls it "genuinely contract-sanctioned...
narrowing those is a contract discussion, not a fix here" (shared-library spec issue, correctly
out of scope for either PR). #97's actual job on this route, therationalestring content, is
correctly fixed —counter_proposal()now populates it frombuyer_rationale, so the leaked
field name is a known/disclosed gap, but the prose itself is safe.Cross-repo, re-verified independently rather than trusting the PR's own claim:
multi_seller.py
reads exactlycounter_terms.get("proposed_price")and.get("negotiation_id"), nothing else.
The otherfloor_pricereads in buyer-agent are unrelated — a different connector's raw deal
object, andmedia_kit/client.py's genuinely separateAuthenticatedPackageViewsurface, which
is exactly why the PR correctly leaves that one untouched. Approving.
…unter-offer
Two information-disclosure leaks on paths a buyer actually reaches.
1. interfaces/chat/main.py stated the floor outright in prose on a
negotiation REJECT -- "Our floor for this inventory is $X CPM",
formatted from history.floor_price. The seller told its counterparty
its own floor, mid-negotiation. The branch is now a walk-away that
discloses no number. It also no longer interpolates the engine's
REJECT rationale, whose max-rounds variant reads "Maximum N rounds
reached" and publishes the remaining concession budget.
2. flows/proposal_handling_flow.py put "floor_price": product.floor_cpm
into state.counter_terms, which is returned to the buyer on EVERY
counter-offer -- so the floor crossed the wire on the normal
negotiation path, not on an obscure read. The key is removed.
A counterparty who knows the floor concedes nothing above it. These are
two of the four fields the shared Negotiation primitive excludes
deliberately (floor_price, base_price, strategy, NegotiationLimits), per
RECONCILIATION.md.
Removing a key from counter_terms is a wire change, so the buyer was
checked first at origin/main: ad_buyer/orchestration/multi_seller.py
reads only counter_terms["proposed_price"] and
counter_terms["negotiation_id"], and no buyer code reads
counter_terms["floor_price"]. The buyer's only floor_price consumer is
media_kit/client.py, which parses GET /media-kit/packages/{id} -- a
different surface, untouched here.
tests/unit/test_counter_and_chat_floor_disclosure.py (11 tests) pins the
two sites and adds a by-field-name sweep: neither path may carry
floor_price, base_price, strategy or max_rounds as a field name at any
nesting depth, so the next guardrail added to either payload fails
without anyone remembering to write a test. Complements the
route-specific test in #95 rather than duplicating it.
Known remaining gap, recorded not half-built: the negotiation engine's
rationale strings state the floor and the strategy in prose (e.g.
"Countering at the floor price $X CPM ... (round 1/5)") and reach the
buyer via counter_terms["reason"]. Changing that is a change to the
engine's wording, on a different surface, and is not made here.
… strategy, or the round budget The negotiation engine's rationale strings stated the seller's floor, strategy and round budget in prose, and that sentence shipped to the buyer as counter_terms["reason"], in the chat counter/final-offer text, and on the REST negotiation responses — so the floor was still derivable in prose on the normal counter path even with floor_price removed from the structured payload (the known gap this PR recorded). The engine now emits both: the internal rationale is byte-identical to before and stays in logs and the stored negotiation history for audit, and a deliberately constructed buyer_rationale explains the action without stating the seller's price in prose (the structured seller_price carries the number unlabeled) and without naming the floor, the strategy, the concession budget or the round limit. Every outbound surface — counter_terms["reason"], the chat COUNTER/FINAL_OFFER text, the REST counter response, and the terminal-round mapper — sends only the buyer-facing one; the mapper falls back to empty for rounds stored before the field existed, never to the internal rationale. Engine pricing arithmetic (concession math, floor clamping, gap-split, acceptance conditions) is untouched; NegotiationRound is a seller-local model, so no shared-contract change. 14 new tests extend the existing disclosure file: per-action engine assertions that the buyer rationale carries no guardrail while the internal one still does, the flow's reason, the chat prose, the REST response, and the mapper fallback.
c3ae6fd to
bb9a283
Compare
Security-relevant — please do not merge casually
This is an information-disclosure fix on a public reference implementation, and it removes a field from a response body on the normal negotiation path. Please review the wire change deliberately rather than rubber-stamping.
This is the second of at least two PRs in this class. #95 fixed the same defect class on
GET /proposals/{proposal_id}/negotiation. That PR's own sweep found more surfaces, and this PR fixes the two that are worst by reachability. Others remain — see "Still open" below. #95 and this PR are independent and can merge in either order.The defect
Two surfaces disclosed the seller's floor price, and neither is an obscure route.
1. The chat interface said it out loud.
interfaces/chat/main.py:544, on a negotiation REJECT, emitted the literal sentence:formatted from
history.floor_price. The seller told its counterparty its own floor, in prose, mid-negotiation.2. Every counter-offer carried it.
flows/proposal_handling_flow.py:821put"floor_price": product.floor_cpmintostate.counter_terms.counter_termsis returned to the buyer on every counter-offer (it is where the buyer readsproposed_priceandnegotiation_id), so this crossed the wire on the normal negotiation path — not on a read someone had to go looking for. Observed payload before the fix:A counterparty who knows the floor concedes nothing above it, so either leak makes the seller's negotiating position structurally worthless.
floor_price,base_price,strategyandNegotiationLimits(max_rounds) are the four fields the sharedNegotiationprimitive excludes deliberately — fromRECONCILIATION.mdiniab-agentic-primitives: "floor_price,base_price,strategy, andNegotiationLimitsare deliberately excluded — they are the seller's internal guardrails and must never cross the wire."Because this is donated reference code, the pattern propagates to every implementer who copies it.
What changed
1. The chat REJECT is now a walk-away with no number in it. House voice kept, consistent with the neighbouring FINAL_OFFER branch, and it still offers the buyer a next step.
It also no longer interpolates the engine's REJECT rationale. That rationale has a max-rounds variant reading "Maximum 5 rounds reached", which publishes the remaining concession budget in prose. Dropping the interpolation is local to the branch being rewritten and removes a second guardrail disclosure from the same message.
2.
floor_priceis removed fromcounter_terms. Nothing else in the dict changed.3. Both sites carry a comment saying why, naming the four fields, so the next person to extend either payload sees the constraint at the point of edit.
4. The engine now emits a second, buyer-facing rationale, and every outbound surface sends only that one — see "The prose leak is closed here too" below. This closes the known gap the first commit recorded instead of shipping it.
Cross-repo check before removing the field — done first, not after
counter_termsis read by the buyer agent, so removing a key from it could have broken the buyer. Checked againstIABTechLab/buyer-agentatorigin/mainbefore touching it:ad_buyer/orchestration/multi_seller.py:1422-1434reads exactly two keys:counter_terms["proposed_price"]andcounter_terms["negotiation_id"]. Both untouched.counter_terms["floor_price"]. No hits anywhere insrc/.floor_priceconsumer isad_buyer/media_kit/client.py:110, in_parse_package_detail, which parsesGET /media-kit/packages/{id}. That is a different seller surface (AuthenticatedPackageView.floor_price) and is deliberately not touched here — removing it would break the buyer's media-kit client, and it needs the public-versus-private ruling described below.So this is safe for the buyer as it stands. A test in the new file pins the two keys the buyer does read, so a later tidy-up of
counter_termscannot quietly remove them.Tests
tests/unit/test_counter_and_chat_floor_disclosure.py— 25 tests in four sections.Sections 1 and 2 pin the two sites: the walk-away contains neither the floor value, nor the base price, nor the word "floor", nor the round budget, and is still a usable message;
counter_termshas nofloor_pricekey and still carries what the buyer reads.Section 3 is the part that matters for the next leak: a by-field-name sweep. It recurses to any depth and asserts that neither path carries
floor_price,base_price,strategyormax_roundsas a field name — overcounter_terms, over the whole buyer-facing proposal response, and over the chat negotiation envelope. A per-site test catches today's two leaks; this catches the next guardrail someone adds to either payload without anyone having to remember to write a new test. One test deliberately asserts that the internalNegotiationHistorydoes contain all four, so the sweep is proven non-vacuous rather than passing because it is looking at nothing.This complements #95 rather than duplicating it: that file pins one route's response shape and its auth gate; this one pins the chat text and the counter payload.
Section 4 covers the two-rationale design (below): per-action engine assertions that the buyer-facing rationale carries no floor value, no floor/minimum/strategy/concession wording and no
N/Mround-budget pattern while the internal rationale still does (so the clean checks are proven non-vacuous), pluscounter_terms["reason"]on a below-floor offer, the chat COUNTER/FINAL_OFFER prose, the REST counter response, and the terminal-round mapper's old-round fallback.Negative control (first commit). With both structured leaks restored from
origin/main, 6 of the original 11 fail, each reporting the actual leaked value — the floor33.11in the chat text,Maximum 5for the round budget, and['floor_price']in thecounter_termspayload. The 5 that still pass correctly should: the walk-away-usability test, the buyer-keys test, the non-vacuousness test, the chat envelope test (that envelope never leaked), and the name sweep over the walk-away text — prose says "our floor", notfloor_price, which is exactly why sections 1 and 2 assert on the value and the wording rather than on the field name. The fix was then re-applied and the restored diff verified byte-identical to the pre-revert diff (matching sha256).Full unit suite: 1546 passed.
ruff check src/andruff format --check src/ tests/both clean.The prose leak is closed here too: two rationales, internal and buyer-facing
The first commit recorded a known gap: the engine's
rationalestrings still stated the floor, the strategy and the round budget in prose —"Buyer offer $25.00 is below our floor. Countering at the floor price $33.11 CPM — the minimum viable price for this inventory (round 1/5).","Counter at $30.00 CPM (collaborative strategy, round 1/5).","Final offer at our floor price $X CPM. We cannot go lower.","...our maximum concession of 15%."— and that sentence shipped to the buyer ascounter_terms["reason"], in the chat COUNTER/FINAL_OFFER text, and on the REST negotiation responses. A leak moved from a named field into a prose string is not fixed.The second commit closes it structurally rather than by rewording.
NegotiationRound(a seller-local model, not the shared contract) now carries two rationales:rationale— internal, unchanged wording. Still names the floor, the strategy, the round budget and the concession cap, because it exists for logs, the stored negotiation history and audit. Nothing is lost on the seller's side.buyer_rationale— buyer-facing, constructed deliberately. Explains the action without stating the seller's price in prose (the structuredseller_price/proposed_pricecarries the number, unlabeled — on a below-floor round the counter price is the floor, so any price in the sentence would be the floor value), and without naming the floor, the strategy, the concession budget or the round limit. Example: "We cannot accept $25.00 CPM for this inventory. We have countered at a price that reflects its value."Every outbound surface sends only the buyer-facing one:
counter_terms["reason"], the chat COUNTER and FINAL_OFFER text, the REST counter response (both the legacy/proposals/{id}/counterdict and the canonical/api/v1/negotiations/messagesmapping), and the terminal-round mapper — which falls back to empty, never to the internal rationale, for rounds persisted before the field existed. The flow's truthful volume note (available-impressions shortfall) is appended to both rationales, since it disclosed nothing.The fix is structural: a future engine string cannot leak, because the buyer-facing sentence is authored as a separate value rather than filtered out of the internal one. Engine pricing arithmetic — concession math, floor clamping, gap-split, acceptance conditions — is byte-identical in behavior; only strings and the round model changed, and the full pre-existing suite passes untouched.
Still open on this class, deliberately out of scope
Each of these needs a deliberate public-versus-private ruling rather than a reflexive patch, and one of them (the package view) has a live buyer consumer that a naive removal would break:
GET /productsandGET /products/{id}are unauthenticated and returnProduct.ext.floor_cpm(contract_mappers.py:504)AuthenticatedPackageView.floor_price(models/media_kit.py:250) exposes a package floor to any authenticated buyer — and the buyer agent's media-kit client reads itfloor_cpmper product (mcp_server.py:388) and a default floor in setup status (:237,:314)GET /api/v1/rate-cardis unauthenticated whilePUTon the same path is operator-gated, thoughRECONCILIATION.mdmarks rate cards as never publicOperator impact
Operators running an earlier build should assume their product floors were readable by any buyer who received a counter-offer, and by any buyer who was rejected in chat. Both are ordinary negotiation outcomes, so the realistic exposure is "every buyer who negotiated", not "someone who probed an unusual endpoint". Called out in the CHANGELOG for that reason.
No version numbers changed.