From 77c7ec18c78ac4b3ad6fcecb3426a993349a381c Mon Sep 17 00:00:00 2001 From: ATC964 <82515918+atc964@users.noreply.github.com> Date: Thu, 17 Sep 2026 17:45:01 -0400 Subject: [PATCH 1/2] fix: stop disclosing the seller's floor price in chat and on every counter-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. --- CHANGELOG.md | 25 ++ src/ad_seller/flows/proposal_handling_flow.py | 10 +- src/ad_seller/interfaces/chat/main.py | 25 +- .../test_counter_and_chat_floor_disclosure.py | 397 ++++++++++++++++++ 4 files changed, 451 insertions(+), 6 deletions(-) create mode 100644 tests/unit/test_counter_and_chat_floor_disclosure.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 4a408c9a..9b31f10b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -98,6 +98,31 @@ All notable changes to the IAB Tech Lab Seller Agent are documented here. was never called and `test_self_asserted_advertiser_identity_is_floored` raised on a `None` `await_args`. CI never caught it because every job starts from a clean checkout. +- **Information disclosure:** the chat interface and every counter-offer + no longer hand the buyer the seller's floor price. Two separate + surfaces were disclosing it. (1) On a negotiation reject, the chat + interface stated the floor outright in prose — "Our floor for this + inventory is $X CPM", formatted from the negotiation's `floor_price` — + so the seller told its counterparty its own floor mid-negotiation. The + reject is now a walk-away that discloses no number, and it no longer + interpolates the engine's reject rationale, whose max-rounds variant + read "Maximum N rounds reached" and published the concession budget. + (2) `counter_terms` carried `floor_price` (the product's `floor_cpm`) + and is returned to the buyer on **every** counter-offer, so the floor + crossed the wire on the normal negotiation path rather than on an + obscure read. That key is removed; `proposed_price`, `negotiation_id` + and the rest of the counter terms are unchanged. 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. A regression test asserts that neither path carries + `floor_price`, `base_price`, `strategy` or `max_rounds` as a field + name at any nesting depth, so a future addition of a guardrail to + either payload fails rather than shipping. Known remaining gap on this + class: the negotiation engine's `rationale` strings still state the + floor and the strategy in prose (for example "Countering at the floor + price $X CPM ... (round 1/5)"), and reach the buyer through + `counter_terms["reason"]`, so the floor is still derivable there; that + is a change to the engine's wording and is not made here. ### Docs diff --git a/src/ad_seller/flows/proposal_handling_flow.py b/src/ad_seller/flows/proposal_handling_flow.py index 02919b02..1e68994a 100644 --- a/src/ad_seller/flows/proposal_handling_flow.py +++ b/src/ad_seller/flows/proposal_handling_flow.py @@ -858,9 +858,17 @@ async def generate_counter_terms(self) -> None: # only, the buyer's next round could never continue it. self.state.negotiation_history = history.model_dump(mode="json") + # counter_terms is returned to the buyer on EVERY counter-offer (it is + # where the buyer reads proposed_price and negotiation_id), so it is an + # outbound payload. Information disclosure fix: it used to carry + # "floor_price": product.floor_cpm, handing the counterparty the + # seller's absolute floor on the normal negotiation path rather than on + # some obscure read. A buyer who knows the floor concedes nothing above + # it. The seller's floor, base_price, strategy and max_rounds are + # internal guardrails -- the shared Negotiation primitive excludes all + # four deliberately -- and must not be added back here. self.state.counter_terms = { "proposed_price": round_result.seller_price, - "floor_price": product.floor_cpm, "max_impressions": self.state.evaluation.available_impressions, "reason": round_result.rationale, "negotiation_id": history.negotiation_id, diff --git a/src/ad_seller/interfaces/chat/main.py b/src/ad_seller/interfaces/chat/main.py index 979a00de..1324e45e 100644 --- a/src/ad_seller/interfaces/chat/main.py +++ b/src/ad_seller/interfaces/chat/main.py @@ -528,7 +528,15 @@ def _format_negotiation_response( round_result: Any, history: Any, ) -> str: - """Format a negotiation round result as human-readable text.""" + """Format a negotiation round result as human-readable text. + + This text goes straight to the counterparty, so it is an outbound + surface: ``history`` carries the seller's internal guardrails + (``floor_price``, ``base_price``, ``strategy``, ``limits.max_rounds``) + and none of them may be interpolated into what is returned here. The + shared ``Negotiation`` primitive excludes those same four fields + deliberately, for the same reason. + """ from ...models.negotiation import NegotiationAction action = round_result.action @@ -538,11 +546,18 @@ def _format_negotiation_response( f"Would you like me to generate a Deal ID for DSP activation?" ) elif action == NegotiationAction.REJECT: + # Information disclosure fix: this branch used to state the + # seller's floor outright -- "Our floor for this inventory is + # $X CPM" -- formatted from history.floor_price, mid-negotiation, + # to the counterparty. A buyer who knows the floor concedes + # nothing above it, so the walk-away now discloses no number at + # all. The engine's REJECT rationale is not interpolated either: + # its max-rounds variant reads "Maximum N rounds reached", which + # publishes the concession budget in prose. return ( - f"I'm sorry, but we can't go that low. " - f"{round_result.rationale}\n\n" - f"Our floor for this inventory is **${history.floor_price:.2f} CPM**. " - f"Would you like to explore other packages that might fit your budget?" + "I'm sorry, but we can't make that work for this inventory.\n\n" + "We've gone as far as we can on price here. " + "Would you like to explore other packages that might fit your budget?" ) elif action == NegotiationAction.FINAL_OFFER: return ( diff --git a/tests/unit/test_counter_and_chat_floor_disclosure.py b/tests/unit/test_counter_and_chat_floor_disclosure.py new file mode 100644 index 00000000..4cb1bcdf --- /dev/null +++ b/tests/unit/test_counter_and_chat_floor_disclosure.py @@ -0,0 +1,397 @@ +# Author: Green Mountain Systems AI Inc. +# Donated to IAB Tech Lab + +"""The chat walk-away and the counter-offer payload must not disclose the +seller's internal negotiation guardrails. + +Two leaks on two different surfaces, both 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. +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. Unlike an obscure GET, this crossed the wire + on the normal negotiation path. + +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``, ``strategy`` and ``NegotiationLimits`` (``max_rounds``) are the +four fields the shared ``Negotiation`` primitive excludes deliberately, per +``RECONCILIATION.md``: they are the seller's internal guardrails and must never +cross the wire. + +Complements ``test_negotiation_status_disclosure.py`` (PR #95), which covers +``GET /proposals/{proposal_id}/negotiation``. That file pins one route's +response; this one pins the chat text and the counter payload, and adds the +by-field-name sweep below. + +KNOWN REMAINING GAP, deliberately not closed here: the engine's +``NegotiationRound.rationale`` strings state the floor and the strategy in +prose -- e.g. *"Countering at the floor price $28.00 CPM ... (round 1/5)"* and +*"Counter at $30.00 CPM (collaborative strategy, round 1/5)"*. Those strings +are built in ``engines/negotiation_engine.py`` and reach the buyer through +``counter_terms["reason"]``, so the floor is still derivable in prose on the +counter path. The field-name sweep below cannot catch that, and neither can a +value assertion without changing engine output. Fixing it means changing how +the engine phrases its rationales, which is a separate change on a separate +surface -- recorded here rather than half-built. +""" + +import os +import sys +from types import ModuleType +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +# Settings require an Anthropic key even for key-less unit runs; same idiom +# as test_negotiation_lowball_counter.py (no LLM call is ever made here). +os.environ.setdefault("ANTHROPIC_API_KEY", "test-key-for-unit-tests") + +# Stub execution_activation_flow (cancel-scope leak on ad-server +# connection failure, unresolved -- issue #60 part 2). +_broken_flows = [ + "ad_seller.flows.execution_activation_flow", +] +for _mod_name in _broken_flows: + if _mod_name not in sys.modules: + _stub = ModuleType(_mod_name) + _cls_name = _mod_name.rsplit(".", 1)[-1].replace("_", " ").title().replace(" ", "") + setattr(_stub, _cls_name, type(_cls_name, (), {})) + sys.modules[_mod_name] = _stub + +from ad_seller.engines.negotiation_engine import NegotiationEngine # noqa: E402 +from ad_seller.engines.pricing_rules_engine import PricingRulesEngine # noqa: E402 +from ad_seller.engines.yield_optimizer import YieldOptimizer # noqa: E402 +from ad_seller.interfaces.chat.main import ChatInterface # noqa: E402 +from ad_seller.models.negotiation import NegotiationAction # noqa: E402 +from ad_seller.models.pricing_tiers import TieredPricingConfig # noqa: E402 + +# The four fields the shared Negotiation primitive excludes, by NAME. Asserting +# on names rather than on today's two leak sites is the point: a per-site test +# catches the two leaks we know about, whereas this catches the next guardrail +# someone adds to an outbound payload. +FORBIDDEN_FIELDS = {"floor_price", "base_price", "strategy", "max_rounds"} + +# Distinctive values, so a leaked number is unmistakable in the payload and +# cannot be confused with an incidental price. +BASE_PRICE = 41.37 +FLOOR_PRICE = 33.11 + + +# ============================================================================= +# Helpers +# ============================================================================= + + +def _all_keys(payload) -> set[str]: + """Every mapping key anywhere in a nested payload, at any depth. + + Recurses so a guardrail nested under ``evaluation`` or inside a + ``rounds[]`` entry is caught as readily as a top-level key. + """ + found: set[str] = set() + if isinstance(payload, dict): + for key, value in payload.items(): + found.add(str(key)) + found |= _all_keys(value) + elif isinstance(payload, (list, tuple)): + for item in payload: + found |= _all_keys(item) + return found + + +def _assert_no_guardrail_fields(payload, surface: str) -> None: + """Fail if any of the four guardrails appears as a field name.""" + leaked = _all_keys(payload) & FORBIDDEN_FIELDS + assert not leaked, ( + f"{surface} carries the seller's internal negotiation guardrail(s) " + f"{sorted(leaked)} by field name. floor_price, base_price, strategy " + f"and max_rounds must never cross the wire. Payload: {payload!r}" + ) + + +def _make_engine() -> NegotiationEngine: + config = TieredPricingConfig(seller_organization_id="test-seller") + return NegotiationEngine(PricingRulesEngine(config=config), MagicMock(spec=YieldOptimizer)) + + +def _make_product(product_id="ctv-premium-sports", base_cpm=BASE_PRICE, floor_cpm=FLOOR_PRICE): + from ad_seller.models.core import DealType, PricingModel + from ad_seller.models.flow_state import ProductDefinition + + return ProductDefinition( + product_id=product_id, + name="Premium CTV - Sports", + inventory_type="ctv", + supported_deal_types=[DealType.PREFERRED_DEAL], + supported_pricing_models=[PricingModel.CPM], + base_cpm=base_cpm, + floor_cpm=floor_cpm, + minimum_impressions=100000, + ) + + +def _make_buyer_context(buyer_tier="agency", agency_id="agency-1"): + from ad_seller.interfaces.api.deps import _build_buyer_context + + return _build_buyer_context(buyer_tier=buyer_tier, agency_id=agency_id) + + +def _proposal_data(price, product_id="ctv-premium-sports"): + return { + "product_id": product_id, + "deal_type": "preferred_deal", + "price": price, + "impressions": 1_000_000, + "start_date": "2026-08-01", + "end_date": "2026-08-31", + } + + +def _run_flow(price): + """Run ProposalHandlingFlow to a counter with the review crew stubbed. + + The crew is made to fail so the deterministic fallback runs; no LLM call + is made. Mirrors the harness in test_negotiation_lowball_counter.py. + """ + import asyncio + + from ad_seller.flows.proposal_handling_flow import ProposalHandlingFlow + + crew = MagicMock() + crew.kickoff_async = AsyncMock(side_effect=RuntimeError("no crew in tests")) + + async def _go(): + with ( + patch( + "ad_seller.flows.proposal_handling_flow.create_proposal_review_crew", + return_value=crew, + ), + patch( + "ad_seller.flows.proposal_handling_flow.emit_event", + new_callable=AsyncMock, + ), + ): + flow = ProposalHandlingFlow() + return await flow.handle_proposal_async( + proposal_id="prop-floor-disclosure", + proposal_data=_proposal_data(price=price), + buyer_context=_make_buyer_context(), + products={"ctv-premium-sports": _make_product()}, + ) + + return asyncio.run(_go()) + + +def _chat_text(action_wanted, buyer_price): + """Produce the chat text for a real engine round of the wanted action. + + Drives the actual engine rather than a hand-built round, so the text is + formatted from a history that genuinely carries the guardrails. + """ + engine = _make_engine() + buyer_context = _make_buyer_context() + history = engine.start_negotiation( + proposal_id="chat-1", + product_id="ctv-premium-sports", + buyer_context=buyer_context, + base_price=BASE_PRICE, + floor_price=FLOOR_PRICE, + ) + for _ in range(history.limits.max_rounds + 2): + round_result = engine.evaluate_buyer_offer(history, buyer_price, buyer_context) + history_next = engine.record_round(history, round_result) + if round_result.action == action_wanted: + return ChatInterface._format_negotiation_response(round_result, history_next) + history = history_next + pytest.fail(f"engine never produced a {action_wanted} round for buyer_price={buyer_price}") + + +# ============================================================================= +# (1) The chat walk-away must not state the floor +# ============================================================================= + + +class TestChatRejectDoesNotDiscloseFloor: + def test_reject_text_omits_the_floor_figure(self): + """The REJECT walk-away must not contain the floor value. + + This is the leak: the branch used to render "Our floor for this + inventory is $33.11 CPM" straight to the counterparty. + """ + text = _chat_text(NegotiationAction.REJECT, buyer_price=0.0) + assert f"{FLOOR_PRICE:.2f}" not in text, ( + f"the chat REJECT walk-away discloses the seller's floor ({FLOOR_PRICE}): {text!r}" + ) + assert f"{BASE_PRICE:.2f}" not in text, ( + f"the chat REJECT walk-away discloses the seller's base price ({BASE_PRICE}): {text!r}" + ) + + def test_reject_text_does_not_name_a_floor_at_all(self): + """Not merely the number: the walk-away must not tell the buyer that + a floor exists for this inventory, since "our floor is" invites the + buyer to probe for it.""" + text = _chat_text(NegotiationAction.REJECT, buyer_price=0.0) + assert "floor" not in text.lower(), ( + f"the chat REJECT walk-away still refers to the seller's floor: {text!r}" + ) + + def test_reject_text_is_still_a_usable_walk_away(self): + """The fix must not leave an empty or contentless message -- the buyer + still has to be told the negotiation is over and offered an out.""" + text = _chat_text(NegotiationAction.REJECT, buyer_price=0.0) + assert text.strip() + assert "?" in text, f"the walk-away offers the buyer no next step: {text!r}" + + def test_reject_text_omits_max_rounds(self): + """The engine's max-rounds REJECT rationale reads "Maximum N rounds + reached", which publishes the concession budget. It must not be + interpolated into the buyer-facing walk-away.""" + engine = _make_engine() + buyer_context = _make_buyer_context() + history = engine.start_negotiation( + proposal_id="chat-1", + product_id="ctv-premium-sports", + buyer_context=buyer_context, + base_price=BASE_PRICE, + floor_price=FLOOR_PRICE, + ) + max_rounds = history.limits.max_rounds + for _ in range(max_rounds + 1): + round_result = engine.evaluate_buyer_offer(history, 1.0, buyer_context) + history = engine.record_round(history, round_result) + if round_result.action == NegotiationAction.REJECT: + break + + assert round_result.action == NegotiationAction.REJECT + text = ChatInterface._format_negotiation_response(round_result, history) + assert f"Maximum {max_rounds}" not in text + assert str(max_rounds) not in text, ( + f"the chat REJECT walk-away discloses the round budget ({max_rounds}): {text!r}" + ) + + +# ============================================================================= +# (2) The counter payload must not carry the floor +# ============================================================================= + + +class TestCounterTermsDoesNotCarryFloor: + def test_counter_terms_has_no_floor_price_key(self): + """counter_terms is returned to the buyer on every counter-offer and + used to carry "floor_price": product.floor_cpm.""" + result = _run_flow(price=25.0) + counter_terms = result["counter_terms"] + assert counter_terms is not None, "expected a counter, got none" + assert "floor_price" not in counter_terms, ( + f"counter_terms still hands the buyer the seller's floor: {counter_terms!r}" + ) + + def test_counter_terms_still_carries_what_the_buyer_needs(self): + """Regression guard on the removal itself: the buyer reads + proposed_price and negotiation_id out of counter_terms (the buyer + agent's multi_seller orchestrator reads exactly those two), so + dropping floor_price must not have disturbed them.""" + result = _run_flow(price=25.0) + counter_terms = result["counter_terms"] + assert counter_terms["proposed_price"] == FLOOR_PRICE + assert counter_terms["negotiation_id"] + assert counter_terms["action"] == "counter" + + +# ============================================================================= +# (3) The by-field-name sweep -- the part that catches the NEXT leak +# ============================================================================= + + +class TestNoGuardrailFieldNamesOnEitherPath: + """Neither path may carry floor_price, base_price, strategy or max_rounds + as a field name, at any nesting depth. + + Sections (1) and (2) pin the two leaks that were found. This section is + the standing guard: it does not care where in the payload a guardrail + appears, so a guardrail added to counter_terms, to the evaluation block, + or to the chat negotiation envelope later fails here without anyone + having to remember to write a new test for it. + """ + + def test_counter_terms_carries_no_guardrail_field(self): + result = _run_flow(price=25.0) + _assert_no_guardrail_fields(result["counter_terms"], "counter_terms") + + def test_full_counter_response_carries_no_guardrail_field(self): + """The whole buyer-facing proposal response, not just counter_terms. + + The internal-only keys the service strips before answering + (``_negotiation_history``, ``_flow_state_snapshot``) are removed here + the same way, so this asserts on what actually reaches the buyer. + """ + result = _run_flow(price=25.0) + outbound = {k: v for k, v in result.items() if not k.startswith("_")} + assert outbound.get("counter_terms") is not None, "expected a counter, got none" + _assert_no_guardrail_fields(outbound, "the proposal counter response") + + def test_internal_history_is_not_part_of_the_response(self): + """Proof the previous test is meaningful rather than vacuous: the + history DOES hold all four guardrails, and is exactly what must stay + behind the service boundary.""" + result = _run_flow(price=25.0) + history = result.get("_negotiation_history") + assert history is not None, "expected the flow to surface a history" + # All four really are in the history (max_rounds nested under limits), + # so the sweep above is asserting absence of something that exists. + assert FORBIDDEN_FIELDS <= _all_keys(history) + assert history["floor_price"] == FLOOR_PRICE + assert all( + not k.startswith("_") or k in ("_negotiation_history", "_flow_state_snapshot") + for k in result + ) + + def test_chat_walk_away_names_no_guardrail_field(self): + """The rewritten REJECT walk-away, swept by name. + + Only the REJECT branch is swept as prose. The other three branches + interpolate ``round_result.rationale``, and the engine phrases its + rationales with the guardrails in them -- "Counter at $X CPM + (collaborative strategy, round 1/5)" names the strategy and states the + round budget, and the below-floor rationale states the floor value. + Those strings are built in ``engines/negotiation_engine.py``, not on + either path fixed here, so sweeping them would be asserting on another + surface's defect. See the module docstring's known-gap note. + """ + text = _chat_text(NegotiationAction.REJECT, buyer_price=0.0).lower() + for field in sorted(FORBIDDEN_FIELDS): + assert field not in text, f"the chat walk-away names the guardrail {field!r}: {text!r}" + + def test_chat_negotiation_envelope_carries_no_guardrail_field(self): + """The structured ``negotiation`` block the chat surface returns + alongside its text.""" + engine = _make_engine() + buyer_context = _make_buyer_context() + history = engine.start_negotiation( + proposal_id="chat-1", + product_id="ctv-premium-sports", + buyer_context=buyer_context, + base_price=BASE_PRICE, + floor_price=FLOOR_PRICE, + ) + round_result = engine.evaluate_buyer_offer(history, 25.0, buyer_context) + history = engine.record_round(history, round_result) + + # The envelope as interfaces/chat/main.py assembles it. + envelope = { + "text": ChatInterface._format_negotiation_response(round_result, history), + "type": "negotiation", + "negotiation": { + "negotiation_id": history.negotiation_id, + "round_number": round_result.round_number, + "action": round_result.action.value, + "buyer_price": round_result.buyer_price, + "seller_price": round_result.seller_price, + "status": history.status, + }, + } + _assert_no_guardrail_fields(envelope, "the chat negotiation envelope") From bb9a283ea32e6ccc1663ab20fc500bb95f646de1 Mon Sep 17 00:00:00 2001 From: ATC964 <82515918+atc964@users.noreply.github.com> Date: Tue, 22 Sep 2026 13:45:10 -0400 Subject: [PATCH 2/2] =?UTF-8?q?fix:=20two=20rationales=20=E2=80=94=20the?= =?UTF-8?q?=20buyer-facing=20one=20never=20names=20the=20floor,=20the=20st?= =?UTF-8?q?rategy,=20or=20the=20round=20budget?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- CHANGELOG.md | 21 +- src/ad_seller/engines/negotiation_engine.py | 42 +++ src/ad_seller/flows/proposal_handling_flow.py | 24 +- .../interfaces/api/contract_mappers.py | 7 +- src/ad_seller/interfaces/chat/main.py | 8 +- src/ad_seller/models/negotiation.py | 18 +- src/ad_seller/services/negotiation_service.py | 14 +- .../test_counter_and_chat_floor_disclosure.py | 320 ++++++++++++++++-- 8 files changed, 410 insertions(+), 44 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9b31f10b..27d724fe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -117,12 +117,21 @@ All notable changes to the IAB Tech Lab Seller Agent are documented here. in chat. A regression test asserts that neither path carries `floor_price`, `base_price`, `strategy` or `max_rounds` as a field name at any nesting depth, so a future addition of a guardrail to - either payload fails rather than shipping. Known remaining gap on this - class: the negotiation engine's `rationale` strings still state the - floor and the strategy in prose (for example "Countering at the floor - price $X CPM ... (round 1/5)"), and reach the buyer through - `counter_terms["reason"]`, so the floor is still derivable there; that - is a change to the engine's wording and is not made here. + either payload fails rather than shipping. (3) The prose leak on the + same class is closed with **two rationales**: the negotiation engine's + `rationale` used to state the floor, the strategy and the round budget + in a sentence (for example "Countering at the floor price $X CPM ... + (round 1/5)") which shipped to the buyer as `counter_terms["reason"]`, + in the chat counter/final-offer text, and on the REST negotiation + responses. The engine now also emits a deliberately constructed + `buyer_rationale` that 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 — and every outbound surface + sends only that one. The internal `rationale` is unchanged and stays + in logs and the stored negotiation history for audit. Engine pricing + arithmetic (concession math, floor clamping, gap-split, acceptance + conditions) is untouched. ### Docs diff --git a/src/ad_seller/engines/negotiation_engine.py b/src/ad_seller/engines/negotiation_engine.py index a655c5f9..abba78f5 100644 --- a/src/ad_seller/engines/negotiation_engine.py +++ b/src/ad_seller/engines/negotiation_engine.py @@ -139,6 +139,15 @@ def evaluate_buyer_offer( Returns: NegotiationRound with the action and counter price + + Every round carries two rationales (information-disclosure fix): + the internal ``rationale`` keeps naming the floor, the strategy and + the round budget for logs and stored history, while the + ``buyer_rationale`` is authored deliberately and never states the + seller's price in prose (the structured ``seller_price`` carries the + number), never names the floor by value or by label, and never + discloses the strategy, the concession budget or the round limit. + Outbound surfaces must send only ``buyer_rationale``. """ round_number = len(history.rounds) + 1 limits = history.limits @@ -157,6 +166,7 @@ def evaluate_buyer_offer( concession_pct=0.0, cumulative_concession_pct=cumulative_concession, rationale="Buyer price meets or exceeds seller target. Deal accepted.", + buyer_rationale=(f"We accept your offer of ${buyer_price:.2f} CPM."), ) # 2. Reject nonpositive offers — not a valid price to negotiate. @@ -174,6 +184,10 @@ def evaluate_buyer_offer( rationale=( f"Buyer price ${buyer_price:.2f} is not a valid offer. Cannot negotiate." ), + buyer_rationale=( + f"An offer of ${buyer_price:.2f} is not a valid price, " + f"so we cannot negotiate on it." + ), ) # 3. Reject if max rounds exceeded (bounds BOTH the gap-split path @@ -190,6 +204,12 @@ def evaluate_buyer_offer( f"Maximum {limits.max_rounds} rounds reached. " f"Negotiation concluded without agreement." ), + # No round count: "Maximum N rounds reached" publishes the + # seller's round budget to the counterparty. + buyer_rationale=( + "We were unable to reach agreement, so we are ending " + "this negotiation. Thank you for the discussion." + ), ) # 3.5 Below-floor offer: counter AT the floor — the seller's minimum @@ -225,6 +245,19 @@ def evaluate_buyer_offer( f"(round {round_number}/{limits.max_rounds})." ) ), + # The counter here IS the floor, so the buyer-facing string + # must not state the seller's price in prose at all — the + # structured seller_price carries the number without labeling + # it. No "floor", no "minimum", no round budget. + buyer_rationale=( + f"We cannot accept ${buyer_price:.2f} CPM for this inventory, " + f"and we have moved as far as we can on price." + if is_last_round + else ( + f"We cannot accept ${buyer_price:.2f} CPM for this inventory. " + f"We have countered at a price that reflects its value." + ) + ), ) # Generate counter price @@ -264,6 +297,7 @@ def evaluate_buyer_offer( concession_pct=incremental_concession, cumulative_concession_pct=new_cumulative, rationale="Buyer price is acceptable within concession limits.", + buyer_rationale=(f"We accept your offer of ${buyer_price:.2f} CPM."), ) return NegotiationRound( @@ -278,6 +312,9 @@ def evaluate_buyer_offer( f"This represents our maximum concession of " f"{limits.total_concession_cap * 100:.0f}%." ), + # No concession percentage, no price in prose: final_price can + # equal the floor, and the concession cap is a guardrail. + buyer_rationale=("We have moved as far as we can on price for this inventory."), ) # 5. Counter with gap-split @@ -293,6 +330,11 @@ def evaluate_buyer_offer( f"({history.strategy.value} strategy, " f"round {round_number}/{limits.max_rounds})." ), + # No strategy name, no round budget, no price in prose (the + # gap-split counter can land exactly on the floor). + buyer_rationale=( + "We have countered at a price that reflects the value of this inventory." + ), ) def record_round( diff --git a/src/ad_seller/flows/proposal_handling_flow.py b/src/ad_seller/flows/proposal_handling_flow.py index 1e68994a..d133e212 100644 --- a/src/ad_seller/flows/proposal_handling_flow.py +++ b/src/ad_seller/flows/proposal_handling_flow.py @@ -835,13 +835,17 @@ async def generate_counter_terms(self) -> None: f"the available volume." ) if round_result.action == NegotiationAction.ACCEPT: + # The volume note discloses nothing (availability is the + # truthful subject of the counter), so it goes on BOTH + # rationales; the agreed price is the buyer's own offer. + agreeable = ( + f"Price ${round_result.seller_price:.2f} CPM is agreeable. {volume_note}" + ) round_result = round_result.model_copy( update={ "action": NegotiationAction.COUNTER, - "rationale": ( - f"Price ${round_result.seller_price:.2f} CPM is " - f"agreeable. {volume_note}" - ), + "rationale": agreeable, + "buyer_rationale": agreeable, } ) elif round_result.action in ( @@ -849,7 +853,10 @@ async def generate_counter_terms(self) -> None: NegotiationAction.FINAL_OFFER, ): round_result = round_result.model_copy( - update={"rationale": f"{round_result.rationale} {volume_note}"} + update={ + "rationale": f"{round_result.rationale} {volume_note}", + "buyer_rationale": f"{round_result.buyer_rationale} {volume_note}", + } ) history = neg_engine.record_round(history, round_result) @@ -870,7 +877,12 @@ async def generate_counter_terms(self) -> None: self.state.counter_terms = { "proposed_price": round_result.seller_price, "max_impressions": self.state.evaluation.available_impressions, - "reason": round_result.rationale, + # buyer_rationale, never rationale: the internal rationale names + # the floor, the strategy and the round budget in prose, which is + # the same disclosure as the removed floor_price key, moved into + # a sentence. The buyer-facing rationale is authored in the + # engine to explain the action without any of the four. + "reason": round_result.buyer_rationale, "negotiation_id": history.negotiation_id, "round_number": round_result.round_number, "action": round_result.action.value, diff --git a/src/ad_seller/interfaces/api/contract_mappers.py b/src/ad_seller/interfaces/api/contract_mappers.py index 14c997be..8893a3f9 100644 --- a/src/ad_seller/interfaces/api/contract_mappers.py +++ b/src/ad_seller/interfaces/api/contract_mappers.py @@ -443,7 +443,12 @@ def terminal_round_response( buyer_price=float_to_money(buyer_p) or Money(amount_micros=0), seller_price=float_to_money(seller_p) or Money(amount_micros=0), action=action, - rationale=last.get("rationale", ""), + # buyer_rationale, never rationale: this response is built from + # STORED rounds, whose internal rationale names the seller's + # guardrails (e.g. "Maximum N rounds reached" when the last round + # was an engine walk-away). Older stored rounds without a + # buyer_rationale fall back to empty, never to the internal one. + rationale=last.get("buyer_rationale", ""), ), rounds_remaining=0, ) diff --git a/src/ad_seller/interfaces/chat/main.py b/src/ad_seller/interfaces/chat/main.py index 1324e45e..3180057b 100644 --- a/src/ad_seller/interfaces/chat/main.py +++ b/src/ad_seller/interfaces/chat/main.py @@ -560,18 +560,22 @@ def _format_negotiation_response( "Would you like to explore other packages that might fit your budget?" ) elif action == NegotiationAction.FINAL_OFFER: + # buyer_rationale, never rationale: the internal rationale labels + # the price as the floor and states the concession cap. return ( f"Here's our **best and final offer**: **${round_result.seller_price:.2f} CPM**.\n\n" - f"{round_result.rationale}\n\n" + f"{round_result.buyer_rationale}\n\n" f"This is the lowest we can go. Would you like to accept?" ) else: # COUNTER + # buyer_rationale, never rationale: the internal rationale names + # the strategy, the round budget and (below floor) the floor. rounds_left = history.limits.max_rounds - round_result.round_number return ( f"We appreciate your offer of ${round_result.buyer_price:.2f} CPM. " f"How about **${round_result.seller_price:.2f} CPM**?\n\n" - f"{round_result.rationale}\n\n" + f"{round_result.buyer_rationale}\n\n" f"{'We have room for further discussion.' if rounds_left > 1 else 'This is close to our limit.'}" ) diff --git a/src/ad_seller/models/negotiation.py b/src/ad_seller/models/negotiation.py index d0579176..4d5f4e9d 100644 --- a/src/ad_seller/models/negotiation.py +++ b/src/ad_seller/models/negotiation.py @@ -84,7 +84,20 @@ class NegotiationLimits(BaseModel): class NegotiationRound(BaseModel): - """A single round in a negotiation.""" + """A single round in a negotiation. + + Carries TWO rationales, and the distinction is load-bearing: + + - ``rationale`` is the INTERNAL explanation. It may (and does) name the + seller's guardrails — the floor, the strategy, the round budget, the + concession caps — because it exists for logs, stored history and audit. + It must never be interpolated into anything the buyer receives. + - ``buyer_rationale`` is the BUYER-FACING explanation. It is authored + deliberately in the engine and never names the floor (by value or by + label), the strategy, the concession budget or the round limit. Every + outbound surface (counter_terms["reason"], the chat text, the REST + negotiation responses) must use this one. + """ round_number: int buyer_price: float # What buyer offered @@ -92,7 +105,8 @@ class NegotiationRound(BaseModel): action: NegotiationAction concession_pct: float = 0.0 # How much seller conceded this round (0-1) cumulative_concession_pct: float = 0.0 # Total concession so far (0-1) - rationale: str = "" + rationale: str = "" # Internal-only: logs, stored history, audit + buyer_rationale: str = "" # Outbound: the only rationale the buyer may see timestamp: datetime = Field(default_factory=datetime.utcnow) diff --git a/src/ad_seller/services/negotiation_service.py b/src/ad_seller/services/negotiation_service.py index 32f27b4f..27959b84 100644 --- a/src/ad_seller/services/negotiation_service.py +++ b/src/ad_seller/services/negotiation_service.py @@ -303,7 +303,12 @@ async def counter_proposal( "seller_price": round_result.seller_price, "concession_pct": round_result.concession_pct, "cumulative_concession_pct": round_result.cumulative_concession_pct, - "rationale": round_result.rationale, + # This dict is returned to the buyer verbatim on the legacy counter + # route and mapped into the shared NegotiationRoundResponse on the + # canonical route, so it carries the buyer-facing rationale. The + # internal rationale (floor, strategy, round budget) stays in the + # stored history persisted above. + "rationale": round_result.buyer_rationale, "status": history.status, "rounds_remaining": history.limits.max_rounds - round_result.round_number, } @@ -345,6 +350,13 @@ async def apply_terminal_action( if action == "accept" else "Buyer declined (walk-away)." ), + # Same text on both: the buyer's own terminal move + # discloses no seller guardrail. + buyer_rationale=( + f"Buyer accepted at ${seller_price:.2f} CPM." + if action == "accept" + else "Buyer declined (walk-away)." + ), ) ) history.status = "accepted" if action == "accept" else "rejected" diff --git a/tests/unit/test_counter_and_chat_floor_disclosure.py b/tests/unit/test_counter_and_chat_floor_disclosure.py index 4cb1bcdf..3602519e 100644 --- a/tests/unit/test_counter_and_chat_floor_disclosure.py +++ b/tests/unit/test_counter_and_chat_floor_disclosure.py @@ -27,19 +27,23 @@ response; this one pins the chat text and the counter payload, and adds the by-field-name sweep below. -KNOWN REMAINING GAP, deliberately not closed here: the engine's -``NegotiationRound.rationale`` strings state the floor and the strategy in -prose -- e.g. *"Countering at the floor price $28.00 CPM ... (round 1/5)"* and -*"Counter at $30.00 CPM (collaborative strategy, round 1/5)"*. Those strings -are built in ``engines/negotiation_engine.py`` and reach the buyer through -``counter_terms["reason"]``, so the floor is still derivable in prose on the -counter path. The field-name sweep below cannot catch that, and neither can a -value assertion without changing engine output. Fixing it means changing how -the engine phrases its rationales, which is a separate change on a separate -surface -- recorded here rather than half-built. +THE PROSE LEAK IS NOW CLOSED with two rationales (section 4 below). The +engine's ``NegotiationRound.rationale`` used to state the floor, the strategy +and the round budget in prose -- e.g. *"Countering at the floor price $28.00 +CPM ... (round 1/5)"* -- and that sentence shipped to the buyer as +``counter_terms["reason"]``, in the chat COUNTER/FINAL_OFFER text, and on the +REST negotiation responses. The engine now emits BOTH an internal +``rationale`` (unchanged wording, kept for logs, stored history and audit) +and a deliberately constructed ``buyer_rationale`` that never states the +seller's price in prose (the structured ``seller_price`` carries the number +unlabeled), never says floor/minimum/strategy, and never discloses the round +budget or the concession caps. Every outbound surface sends only +``buyer_rationale``, so a future engine string cannot leak: the buyer-facing +sentence is constructed, not filtered. """ import os +import re import sys from types import ModuleType from unittest.mock import AsyncMock, MagicMock, patch @@ -186,7 +190,7 @@ async def _go(): return asyncio.run(_go()) -def _chat_text(action_wanted, buyer_price): +def _chat_text(action_wanted, buyer_price, base_price=BASE_PRICE, floor_price=FLOOR_PRICE): """Produce the chat text for a real engine round of the wanted action. Drives the actual engine rather than a hand-built round, so the text is @@ -198,8 +202,8 @@ def _chat_text(action_wanted, buyer_price): proposal_id="chat-1", product_id="ctv-premium-sports", buyer_context=buyer_context, - base_price=BASE_PRICE, - floor_price=FLOOR_PRICE, + base_price=base_price, + floor_price=floor_price, ) for _ in range(history.limits.max_rounds + 2): round_result = engine.evaluate_buyer_offer(history, buyer_price, buyer_context) @@ -351,20 +355,25 @@ def test_internal_history_is_not_part_of_the_response(self): ) def test_chat_walk_away_names_no_guardrail_field(self): - """The rewritten REJECT walk-away, swept by name. - - Only the REJECT branch is swept as prose. The other three branches - interpolate ``round_result.rationale``, and the engine phrases its - rationales with the guardrails in them -- "Counter at $X CPM - (collaborative strategy, round 1/5)" names the strategy and states the - round budget, and the below-floor rationale states the floor value. - Those strings are built in ``engines/negotiation_engine.py``, not on - either path fixed here, so sweeping them would be asserting on another - surface's defect. See the module docstring's known-gap note. + """Every chat branch's prose, swept by name. + + The COUNTER and FINAL_OFFER branches used to interpolate the engine's + internal ``rationale``, which names the strategy, the round budget + and (below floor) the floor -- so only REJECT could be swept when + this file was first written. They now interpolate the deliberately + constructed ``buyer_rationale``, so all branches are swept. """ - text = _chat_text(NegotiationAction.REJECT, buyer_price=0.0).lower() - for field in sorted(FORBIDDEN_FIELDS): - assert field not in text, f"the chat walk-away names the guardrail {field!r}: {text!r}" + for action, price in ( + (NegotiationAction.REJECT, 0.0), + (NegotiationAction.COUNTER, 25.0), + (NegotiationAction.FINAL_OFFER, 25.0), + (NegotiationAction.ACCEPT, BASE_PRICE + 1.0), + ): + text = _chat_text(action, buyer_price=price).lower() + for field in sorted(FORBIDDEN_FIELDS): + assert field not in text, ( + f"the chat {action.value} text names the guardrail {field!r}: {text!r}" + ) def test_chat_negotiation_envelope_carries_no_guardrail_field(self): """The structured ``negotiation`` block the chat surface returns @@ -395,3 +404,262 @@ def test_chat_negotiation_envelope_carries_no_guardrail_field(self): }, } _assert_no_guardrail_fields(envelope, "the chat negotiation envelope") + + +# ============================================================================= +# (4) Two rationales: the buyer-facing one never carries a guardrail +# ============================================================================= + +# "(round 1/5)"-style round-budget disclosure, anywhere in a string. +ROUND_BUDGET_RE = re.compile(r"\b\d+\s*/\s*\d+\b") + +# Words that label a guardrail in prose. "minimum" covers "the minimum viable +# price" (a floor synonym); "concession" covers the cap percentages. +FORBIDDEN_WORDS = ("floor", "minimum", "strategy", "concession") + + +# A second distinctive floor, deep enough that the concession-cap FINAL_OFFER +# and the gap-split COUNTER land ABOVE it (the agency tier discount pulls the +# effective base to ~$35.16, and the 15% collaborative cap bottoms out at +# ~$29.89): with this floor those prices are not the floor, so "the floor +# value is absent" is a real assertion rather than a coincidence. +DEEP_FLOOR_PRICE = 21.47 + + +def _drive_engine_to(action_wanted, buyer_price, base_price=BASE_PRICE, floor_price=FLOOR_PRICE): + """Drive the real engine until it emits the wanted action. + + Returns (round, history) with the round already recorded, so assertions + run against genuine engine output, not hand-built rounds. + """ + engine = _make_engine() + buyer_context = _make_buyer_context() + history = engine.start_negotiation( + proposal_id="prop-two-rationales", + product_id="ctv-premium-sports", + buyer_context=buyer_context, + base_price=base_price, + floor_price=floor_price, + ) + for _ in range(history.limits.max_rounds + 2): + round_result = engine.evaluate_buyer_offer(history, buyer_price, buyer_context) + history = engine.record_round(history, round_result) + if round_result.action == action_wanted: + return round_result, history + pytest.fail(f"engine never produced {action_wanted} for buyer_price={buyer_price}") + + +def _assert_buyer_facing(text: str, surface: str) -> None: + """The buyer-facing rationale contract, in one place. + + No floor value, no base-price value, no guardrail word, no round-budget + pattern -- and the string is still a usable explanation, not empty. + """ + low = text.lower() + assert f"{FLOOR_PRICE:.2f}" not in text, f"{surface} states the floor value: {text!r}" + assert f"{BASE_PRICE:.2f}" not in text, f"{surface} states the base price: {text!r}" + for word in FORBIDDEN_WORDS: + assert word not in low, f"{surface} says {word!r}: {text!r}" + assert not ROUND_BUDGET_RE.search(text), f"{surface} discloses the round budget: {text!r}" + assert text.strip(), f"{surface} is empty -- the buyer gets no explanation" + + +class TestEngineEmitsTwoRationales: + """Engine level: for every action the engine can take, the buyer-facing + rationale is clean while the internal one still says everything it said + before (the internal assertions double as proof the clean checks are + non-vacuous).""" + + def test_below_floor_counter(self): + round_result, _ = _drive_engine_to(NegotiationAction.COUNTER, buyer_price=25.0) + assert round_result.seller_price == FLOOR_PRICE # countering AT the floor + _assert_buyer_facing(round_result.buyer_rationale, "the below-floor COUNTER") + # Internal keeps the full story: floor value, the word, the budget. + assert f"{FLOOR_PRICE:.2f}" in round_result.rationale + assert "floor" in round_result.rationale.lower() + assert ROUND_BUDGET_RE.search(round_result.rationale) + + def test_below_floor_final_offer(self): + round_result, _ = _drive_engine_to(NegotiationAction.FINAL_OFFER, buyer_price=25.0) + assert round_result.seller_price == FLOOR_PRICE + _assert_buyer_facing(round_result.buyer_rationale, "the below-floor FINAL_OFFER") + assert f"{FLOOR_PRICE:.2f}" in round_result.rationale + assert "floor" in round_result.rationale.lower() + + def test_concession_cap_final_offer(self): + """FINAL_OFFER via the concession-cap path, where the final price sits + ABOVE the floor -- so the floor value is a distinct secret and its + absence from the buyer-facing string is a real assertion.""" + round_result, _ = _drive_engine_to( + NegotiationAction.FINAL_OFFER, buyer_price=25.0, floor_price=DEEP_FLOOR_PRICE + ) + assert round_result.seller_price > DEEP_FLOOR_PRICE + _assert_buyer_facing(round_result.buyer_rationale, "the cap-path FINAL_OFFER") + assert f"{DEEP_FLOOR_PRICE:.2f}" not in round_result.buyer_rationale, ( + f"the cap-path FINAL_OFFER states the floor value: {round_result.buyer_rationale!r}" + ) + assert "%" not in round_result.buyer_rationale, ( + f"the cap-path FINAL_OFFER discloses a concession percentage: " + f"{round_result.buyer_rationale!r}" + ) + # Internal still records the cap. + assert "concession" in round_result.rationale.lower() + assert "%" in round_result.rationale + + def test_gap_split_counter(self): + round_result, history = _drive_engine_to( + NegotiationAction.COUNTER, buyer_price=25.0, floor_price=DEEP_FLOOR_PRICE + ) + assert round_result.seller_price > DEEP_FLOOR_PRICE # gap-split, not floor + _assert_buyer_facing(round_result.buyer_rationale, "the gap-split COUNTER") + assert f"{DEEP_FLOOR_PRICE:.2f}" not in round_result.buyer_rationale + # Internal still names the strategy and the budget. + assert history.strategy.value in round_result.rationale + assert ROUND_BUDGET_RE.search(round_result.rationale) + + def test_accept(self): + round_result, _ = _drive_engine_to(NegotiationAction.ACCEPT, buyer_price=BASE_PRICE + 1.0) + _assert_buyer_facing(round_result.buyer_rationale, "the ACCEPT rationale") + + def test_reject_max_rounds(self): + round_result, history = _drive_engine_to(NegotiationAction.REJECT, buyer_price=1.0) + _assert_buyer_facing(round_result.buyer_rationale, "the max-rounds REJECT") + max_rounds = history.limits.max_rounds + assert str(max_rounds) not in round_result.buyer_rationale, ( + f"the max-rounds REJECT discloses the round budget ({max_rounds}): " + f"{round_result.buyer_rationale!r}" + ) + assert f"Maximum {max_rounds}" in round_result.rationale + + def test_reject_invalid_offer(self): + round_result, _ = _drive_engine_to(NegotiationAction.REJECT, buyer_price=0.0) + _assert_buyer_facing(round_result.buyer_rationale, "the invalid-offer REJECT") + + +class TestCounterReasonIsBuyerFacing: + """Flow level: counter_terms["reason"] -- the widest surface, shipped on + every counter-offer -- carries the buyer-facing rationale, while the + stored history keeps the internal one untouched.""" + + def test_below_floor_reason_carries_no_guardrail(self): + result = _run_flow(price=25.0) + reason = result["counter_terms"]["reason"] + _assert_buyer_facing(reason, 'counter_terms["reason"]') + + def test_stored_history_keeps_the_internal_rationale(self): + """The audit trail is unchanged: the persisted round still names the + floor, the word and the budget, and also carries the buyer-facing + string that actually went to the wire.""" + result = _run_flow(price=25.0) + stored_round = result["_negotiation_history"]["rounds"][0] + assert f"{FLOOR_PRICE:.2f}" in stored_round["rationale"] + assert "floor" in stored_round["rationale"].lower() + assert ROUND_BUDGET_RE.search(stored_round["rationale"]) + assert stored_round["buyer_rationale"] == result["counter_terms"]["reason"] + + +class TestChatProseIsBuyerFacing: + """Chat level: the COUNTER and FINAL_OFFER texts no longer interpolate + the internal rationale. + + The floor-value assertions use scenarios where the offered price sits + ABOVE the floor: on a below-floor round the counter price IS the floor, + so the number legitimately appears as the offer itself (unlabeled) and + only the word-level checks apply there. + """ + + def test_counter_text_above_floor(self): + text = _chat_text(NegotiationAction.COUNTER, buyer_price=34.0) + assert f"{FLOOR_PRICE:.2f}" not in text, ( + f"the chat COUNTER discloses the floor ({FLOOR_PRICE}): {text!r}" + ) + low = text.lower() + for word in FORBIDDEN_WORDS: + assert word not in low, f"the chat COUNTER says {word!r}: {text!r}" + assert not ROUND_BUDGET_RE.search(text) + + def test_final_offer_text_above_floor(self): + text = _chat_text( + NegotiationAction.FINAL_OFFER, buyer_price=25.0, floor_price=DEEP_FLOOR_PRICE + ) + assert f"{DEEP_FLOOR_PRICE:.2f}" not in text, ( + f"the chat FINAL_OFFER discloses the floor ({DEEP_FLOOR_PRICE}): {text!r}" + ) + low = text.lower() + for word in FORBIDDEN_WORDS: + assert word not in low, f"the chat FINAL_OFFER says {word!r}: {text!r}" + assert not ROUND_BUDGET_RE.search(text) + + def test_counter_text_below_floor_labels_nothing(self): + """Below floor the counter price equals the floor, so the value is + inherently visible as the offer -- what must not appear is any label + telling the buyer that it IS the bottom.""" + text = _chat_text(NegotiationAction.COUNTER, buyer_price=25.0) + low = text.lower() + for word in FORBIDDEN_WORDS: + assert word not in low, f"the below-floor chat COUNTER says {word!r}: {text!r}" + assert not ROUND_BUDGET_RE.search(text) + + +class TestRestNegotiationResponseIsBuyerFacing: + """REST level: the counter-offer response dict (returned verbatim on the + legacy route and mapped into the shared NegotiationRoundResponse on the + canonical route) carries the buyer-facing rationale, and the terminal + mapper never falls back to a stored internal rationale.""" + + async def test_counter_proposal_response_rationale(self): + from ad_seller.services import negotiation_service + + engine = _make_engine() + buyer_context = _make_buyer_context() + history = engine.start_negotiation( + proposal_id="prop-rest-rationale", + product_id="ctv-premium-sports", + buyer_context=buyer_context, + base_price=BASE_PRICE, + floor_price=FLOOR_PRICE, + ) + + mock_storage = AsyncMock() + mock_storage.get_negotiation.return_value = history.model_dump(mode="json") + with ( + patch("ad_seller.storage.factory.get_storage", return_value=mock_storage), + patch("ad_seller.events.helpers.emit_event", new_callable=AsyncMock), + ): + response = await negotiation_service.counter_proposal( + proposal_id="prop-rest-rationale", + buyer_price=25.0, + buyer_context=buyer_context, + ) + + _assert_buyer_facing(response["rationale"], "the REST counter response rationale") + # The PERSISTED history still holds the full internal rationale. + persisted = mock_storage.set_negotiation.call_args.args[1] + assert f"{FLOOR_PRICE:.2f}" in persisted["rounds"][0]["rationale"] + assert "floor" in persisted["rounds"][0]["rationale"].lower() + + def test_terminal_round_response_never_leaks_a_stored_internal_rationale(self): + """A buyer 'reject' on an already-terminal negotiation answers off the + LAST STORED round, which can be an engine walk-away whose internal + rationale reads "Maximum N rounds reached". The mapper must send the + stored buyer_rationale -- and fall back to empty, never to the + internal one, for rounds persisted before this field existed.""" + from ad_seller.interfaces.api import contract_mappers as cm + + status_data = { + "negotiation_id": "neg-terminal-1", + "rounds": [ + { + "round_number": 5, + "buyer_price": 25.0, + "seller_price": FLOOR_PRICE, + "action": "reject", + "rationale": "Maximum 5 rounds reached. Negotiation concluded without agreement.", + # No buyer_rationale key: a round stored by an older build. + } + ], + } + response = cm.terminal_round_response(status_data, cm.NegotiationAction.REJECT) + assert response.round.rationale == "", ( + f"the terminal mapper fell back to the internal rationale: {response.round.rationale!r}" + )