feat: apply marketplace price multipliers#64
Merged
Conversation
📝 WalkthroughWalkthroughUpdates the ChangesMarketplace Effective Pricing
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Core
participant ServeHook as make_discover_hook
participant Settings
participant Source
participant Shim as _executed_cost_usd
Core->>ServeHook: discovery_id request
ServeHook->>Source: offers_sync(discovery_id)
Source-->>ServeHook: raw offers (unmutated)
ServeHook->>Settings: lookup provider/source price_multiplier
Settings-->>ServeHook: multiplier value
ServeHook->>ServeHook: compute effective_price_in/out via _effective_offer
ServeHook-->>Core: offers with ranking_price_multiplier
Core->>Shim: chosen offer for cost calc
Shim->>Settings: fallback lookup if price_multiplier invalid
Shim->>Shim: divide out multiplier from price_in/price_out
Shim-->>Core: raw billing cost_usd
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
… core to merged engine #26 Align the code with the stated design: the price multiplier is a RANKING-only lever, and a routing preference is an operator decision set + persisted from the Config tab — not hardcoded. Drop _DEFAULT_PRICE_MULTIPLIERS ({bedrock:0.8, openrouter:1.05}); every provider defaults to 1.0 (no nudge). A REAL per-call surcharge is not a multiplier — it belongs in the provider's reported/list price so it ranks AND bills; openrouter's 5% is a credit top-up fee, not per-call, so openrouter needs no multiplier. This also removes the billing under-report the hardcoded 1.05 caused (test_executed_cost_usd_rules: 3.0/1.05). Re-pin core e8263d4 -> 38a79c6 (engine #26 squash-merged to engine main). Tests aligned to the 1.0 default (test_providers, test_sources). Full suite 446 passed / 2 skipped against compose Postgres.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
serve.py (1)
219-235: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winExtend the try/except to cover per-offer effective-price mapping.
hook()'s docstring states it "must be fast and never raise" (called from Lua insiderank), andfn(discovery_id)is guarded accordingly. But the new_effective_offermapping on Line 233 runs outside that try/except. If any offer from a source isn't a well-formed dict (or a future source bug),dict(offer)in_effective_offerraises, and it now escapeshook()uncaught.🛡️ Proposed fix: widen the try/except scope
try: offers = fn(discovery_id) + offers = [_effective_offer(o, discovery_id, source_name) for o in offers] if offers else offers except Exception as exc: # noqa: BLE001 return {"ok": False, "error": f"{type(exc).__name__}: {exc}"} if not offers: # not-ok so the core does NOT cache emptiness for the discovery # TTL — a router that starts before the first market dump should # pick offers up on the next rank, not minutes later. return {"ok": False, "error": "no offers"} - offers = [_effective_offer(o, discovery_id, source_name) for o in offers] return {"ok": True, "fetched_at_ms": int(time.time() * 1000), "offers": offers}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@serve.py` around lines 219 - 235, The uncaught failure path in hook() comes from the per-offer _effective_offer mapping running outside the existing exception guard. Widen the try/except in hook() so it covers both fn(discovery_id) and the list comprehension that applies _effective_offer to each offer, ensuring any bad offer shape or dict(offer) failure is converted into the same {"ok": False, "error": ...} response. Keep the existing unknown discovery_id and no offers handling unchanged, and preserve the fast-never-raise behavior described for hook().
🧹 Nitpick comments (1)
tests/test_sources.py (1)
1185-1191: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMutable class attribute flagged by Ruff (RUF012).
provider_ids = ["bedrock_market"]is a mutable list default in the class body. Test-only fixture, so the risk is negligible, but aClassVarannotation or tuple would satisfy the linter.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_sources.py` around lines 1185 - 1191, The FakeBedrockSource fixture uses a mutable class attribute for provider_ids, which triggers Ruff RUF012. Update the class-level provider_ids on FakeBedrockSource to use an immutable ClassVar/tuple (or otherwise annotate it as a ClassVar) so the test fixture still works while satisfying the linter. Keep the change localized to the FakeBedrockSource definition and preserve the existing provider_id check in offers_sync.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@serve.py`:
- Around line 219-235: The uncaught failure path in hook() comes from the
per-offer _effective_offer mapping running outside the existing exception guard.
Widen the try/except in hook() so it covers both fn(discovery_id) and the list
comprehension that applies _effective_offer to each offer, ensuring any bad
offer shape or dict(offer) failure is converted into the same {"ok": False,
"error": ...} response. Keep the existing unknown discovery_id and no offers
handling unchanged, and preserve the fast-never-raise behavior described for
hook().
---
Nitpick comments:
In `@tests/test_sources.py`:
- Around line 1185-1191: The FakeBedrockSource fixture uses a mutable class
attribute for provider_ids, which triggers Ruff RUF012. Update the class-level
provider_ids on FakeBedrockSource to use an immutable ClassVar/tuple (or
otherwise annotate it as a ClassVar) so the test fixture still works while
satisfying the linter. Keep the change localized to the FakeBedrockSource
definition and preserve the existing provider_id check in offers_sync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 54b6e1da-2c29-40b0-8811-61dc09c2478a
📒 Files selected for processing (9)
coreproviders.pyserve.pyshim.pysources/__init__.pytests/test_live_wiring.pytests/test_providers.pytests/test_shim.pytests/test_sources.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
bedrock.price_multiplierto0.8andopenrouter.price_multiplierto1.05effective_price_*plusranking_price_multiplierat discovery time for selectionbedrock_marketusesbedrock.price_multiplierEngine dependency
effective_price_*and carriesprice_multiplierintochosen.Validation
PYTHONPYCACHEPREFIX=/private/tmp/unhardcoded-pycache .venv/bin/python -m pytest tests/test_sources.py::test_push_prices_only_pushes_cataloged_pairs tests/test_sources.py::test_push_prices_applies_effective_price_multiplier tests/test_sources.py::test_discover_hook_adds_effective_offer_prices_without_mutating_raw tests/test_providers.py::test_priced_providers_get_an_effective_multiplier_knob tests/test_shim.py::test_executed_cost_usd_ignores_the_ranking_multiplier tests/test_live_wiring.py::test_marketplace_offers_rank_with_effective_prices_when_present -q— 6 passedgit diff --checkPYTHONPYCACHEPREFIX=/private/tmp/unhardcoded-pycache .venv/bin/python -m py_compile providers.py serve.py shim.py tests/test_sources.py tests/test_live_wiring.py tests/test_shim.py tests/test_providers.pyNote on broader local tests
tests/test_sources.py tests/test_providers.py tests/test_shim.py tests/test_live_wiring.pyreached63 passed, 19 skippedwith no failures but stalled until interrupted at ~25 minutes. The slow path appears to be unrelated app/lifespan/live-wiring setup, not the focused multiplier cases.Summary by CodeRabbit
New Features
Bug Fixes