fix(discovery): bootstrap an honest provider-diverse failover pool - #770
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
|
@opencode-agent review the exact current head after the GREEN implementation lands. Verify that unknown prices are ranked after known prices rather than as zero-cost, provider-diverse bootstrap takes one independent provider family before duplicates, NVIDIA NIM primary/sub remain one outage domain, all-unpriced ordering is deterministic, and existing discovery/provider credentials are unchanged. Submit a formal current-head review. |
|
@opencode-agent Work on the existing branch at exact head |
|
@opencode-agent Review exact current head |
|
Current-head repair: 6b603ef. Added docs/doctoring/provider-diverse-discovery-routing.md with APA 7 references and an exact code-to-research mapping for the provider-diverse, cost-honest selector. It links the prerequisite-stack OA PDFs and states the implementation is deterministic eligibility/cost accounting, not a learned quality judge. Focused discovery/CLI tests: 35 passed; paper-contract tests: 3 passed; Ruff, compileall, and diff check passed. Please review this exact head. |
|
@opencode-agent Review exact current HEAD only. Verify provider-diverse bootstrap selection, shared chat capability filtering, unknown-price honesty, and stale-discovery handling against the current #768 stack. Publish a formal verdict from same-head Checks. |
|
@opencode-agent Review exact current HEAD 6b603ef only. Verify provider-diverse bootstrap selection, shared chat capability filtering, unknown-price honesty, and stale-discovery handling against the current #768 stack. Publish a formal verdict from same-head Checks. |
|
Current-head verification and independent review request (2026-08-21) Exact production head checked: Local exact-head evidence:
Please perform an independent review against exactly |
|
Review exact current HEAD 6b603ef only. The predecessor research-grounding finding was addressed in docs/doctoring/provider-diverse-discovery-routing.md with APA 7 references and code-to-research mapping, while the selector remains deterministic and provider/cost-honest. Revalidate current source and documentation against the live #768 base; report current findings only. |
Exact-head validation — PR #770
@opencode-agent please review only exact current HEAD |
|
Correction to the preceding validation: GitHub currently reports PR #770 |
|
Exact-head correction and review update: current HEAD is now |
|
Correction: the exact current HEAD is |
|
Current-head audit for |
|
Rechecked the research-grounding finding at exact head The branch already contains the required cite+link+summary path: This satisfies the no-redistribution fallback in |
Exact-head review audit
|
|
Verified against the current PR head 7494f22: the research-grounding requirement is already satisfied and no duplicate artifact is needed.
The finding appears to be based on an earlier snapshot, so no source change is required. |
…ovider-bootstrap feat: persist provider credentials and durable model catalog
84b010a
into
fix/chat-capability-isolation-embedding-models
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| prices = (model.prompt_price_per_1k, model.completion_price_per_1k) | ||
| known = [price for price in prices if price is not None] | ||
| if not known: | ||
| return (1, float("inf"), model.provider_name, model.model_id) | ||
| return (0, sum(known), model.provider_name, model.model_id) |
There was a problem hiding this comment.
🟡 Partial prices ranked as fully-known cost in the bootstrap selector
_known_cost_sort_key marks a model with only a prompt or only a completion price as known-priced and ranks it by that single component. Such a partial-price model can outrank a fully-priced, truly comparable candidate in select_provider_diverse_models and win a failover-pool slot, breaking the price-honesty rule the rest of discovery enforces (both components required).
| prices = (model.prompt_price_per_1k, model.completion_price_per_1k) | |
| known = [price for price in prices if price is not None] | |
| if not known: | |
| return (1, float("inf"), model.provider_name, model.model_id) | |
| return (0, sum(known), model.provider_name, model.model_id) | |
| prices = (model.prompt_price_per_1k, model.completion_price_per_1k) | |
| if any(price is None for price in prices): | |
| return (1, float("inf"), model.provider_name, model.model_id) | |
| return (0, sum(prices), model.provider_name, model.model_id) |
Was this helpful? React with 👍 or 👎 to provide feedback.
| for model in ordered: | ||
| if model.provider_name in seen_providers: | ||
| continue | ||
| selected.append(model) | ||
| seen_providers.add(model.provider_name) |
There was a problem hiding this comment.
🔍 Production diversity selector does not collapse NIM primary/sub outage domain
select_bootstrap_discovered_agents treats nvidia_nim and nvidia_nim_sub as one outage domain via _provider_family, but the selector actually used by the durable bootstrap, select_provider_diverse_models, diversifies on raw provider_name only. The production failover pool can therefore hold two credentials for the same NVIDIA upstream, which is at odds with the 'provider-diverse failover' goal. ADR 0015 calls the keys independent accounts, so confirm which behavior is intended for the durable path.
Was this helpful? React with 👍 or 👎 to provide feedback.
| try: | ||
| return float(value) * 1000 | ||
| except (TypeError, ValueError): | ||
| per_1k = float(value) * 1000 |
There was a problem hiding this comment.
📝 Info: Docstrings dropped from two discovery parsers
_parse_openai_compatible and _parse_bytez lost their docstrings while the rest of the module keeps every function documented. Interrogate fail-under=80 likely still passes given the many new documented functions, but the removal is inconsistent with the surrounding style.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if not isinstance(raw, dict): | ||
| return None | ||
| try: | ||
| if ( | ||
| "prompt_price_per_1k" not in raw | ||
| or "completion_price_per_1k" not in raw | ||
| ): | ||
| return None | ||
| prompt_price = float(raw["prompt_price_per_1k"]) | ||
| completion_price = float(raw["completion_price_per_1k"]) | ||
| except (OverflowError, TypeError, ValueError): | ||
| return None | ||
| if ( | ||
| not math.isfinite(prompt_price) | ||
| or not math.isfinite(completion_price) | ||
| or prompt_price < 0 | ||
| or completion_price < 0 | ||
| ): | ||
| return None |
There was a problem hiding this comment.
📝 Info: get_price now returns None for partial/corrupt rows, changing compute_cost for those rows
PriceBook.get_price (cost_ledger.py) now returns None whenever a stored row is not a dict, is missing either prompt_price_per_1k/completion_price_per_1k, or has a non-finite/negative/unparseable component. Previously a partial row defaulted the missing component to 0.0 and returned a usable entry. This is a behavior change for compute_cost: a partial price row now yields the unpriced fallback (0.0) for the whole request rather than costing only the present component. In practice refresh_price_book and PriceEntry/set_price always persist both components, so only manually-written/corrupted KV rows are affected — hence not flagged as a bug. Worth confirming no production path writes single-component price rows.
Was this helpful? React with 👍 or 👎 to provide feedback.
| for candidate in list(bootstrap.candidates): | ||
| if candidate.id in selected_ids: | ||
| continue | ||
| if candidate.id == "bootstrap_agent" or "discovered" in candidate.tags: | ||
| if not candidate.disabled: | ||
| bootstrap.remove_agent("default", candidate.id) |
There was a problem hiding this comment.
📝 Info: Disabled stale discovered agents survive pool refresh
_synchronize_durable_agent_pool removes a non-selected discovered agent only when it is currently enabled (if not candidate.disabled). A disabled, previously-discovered agent absent from the new selection is left in the pool, so the documented 'withdraw agents absent from the current selection' contract holds only for enabled ones. Low impact since such agents stay inert.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if args.enable_cheapest: | ||
| for model in select_top_n_cheapest_discovered_agents(discovered, price_book, args.enable_cheapest): | ||
| for model in select_bootstrap_discovered_agents(discovered, price_book, args.enable_cheapest): | ||
| agent_id = agent_id_for(model) | ||
| bootstrap.patch_agent("default", agent_id, {"status": "active"}) | ||
| enabled_agent_ids.append(agent_id) |
There was a problem hiding this comment.
📝 Info: --enable-cheapest flag now performs provider-diverse selection despite its name
The CLI flag --enable-cheapest (main.py) now drives select_bootstrap_discovered_agents, which prefers one candidate per provider family before taking a second from an already-represented family. So --enable-cheapest N no longer strictly enables the N globally-cheapest agents; it enables a provider-diverse pool ordered by cost. The help text was updated to describe this, but the flag name retains 'cheapest', which may surprise operators expecting pure cost ordering. Behavioral change is intentional per the PR.
Was this helpful? React with 👍 or 👎 to provide feedback.
| def _deduplicate_discovered_models( | ||
| discovered: list[DiscoveredModel], | ||
| ) -> list[DiscoveredModel]: | ||
| """Collapse duplicate agent identities and withhold conflicting price evidence. | ||
|
|
||
| Exact duplicate catalog rows become one candidate. When the same provider/model | ||
| identity is repeated with conflicting metadata or prices, one deterministic | ||
| transport record is retained but its prices become unknown. Provider row order | ||
| therefore cannot fabricate a cheaper bootstrap candidate or consume failover | ||
| capacity twice. | ||
| """ | ||
| unique: dict[tuple[str, str], DiscoveredModel] = {} | ||
| for model in discovered: | ||
| identity = _serving_identity(model) | ||
| previous = unique.get(identity) | ||
| if previous is None: | ||
| unique[identity] = model | ||
| continue | ||
| if previous == model: | ||
| continue | ||
| chosen = min((previous, model), key=_source_tiebreaker) | ||
| unique[identity] = replace( | ||
| chosen, | ||
| prompt_price_per_1k=None, | ||
| completion_price_per_1k=None, | ||
| ) | ||
| return list(unique.values()) |
There was a problem hiding this comment.
📝 Info: Deduplication is invoked redundantly across the discovery pipeline
_deduplicate_discovered_models is applied in _parse_openai_compatible/_parse_bytez, again in discover_all_models, and again inside refresh_price_book and each selector. It is idempotent so this is correct, but the repeated linear passes are redundant work. Not a correctness issue; noting for future simplification (dedup once at the discovery boundary).
Was this helpful? React with 👍 or 👎 to provide feedback.
| if previous == model: | ||
| continue | ||
| chosen = min((previous, model), key=_source_tiebreaker) | ||
| unique[identity] = replace( | ||
| chosen, | ||
| prompt_price_per_1k=None, | ||
| completion_price_per_1k=None, | ||
| ) |
There was a problem hiding this comment.
📝 Info: Conflicting-duplicate resolution withholds prices even when prices agree
In _deduplicate_discovered_models (model_discovery.py:180-187), any two rows sharing an identity that are not fully == equal cause prices to be dropped to None. This means rows with identical prices but differing transport metadata (e.g. chat_base_url) lose their price evidence, not just genuinely price-conflicting rows. This is more conservative than the docstring's 'conflicting metadata or prices' phrasing implies but is deterministic and fail-safe, so not a bug — just noting the slightly broader withholding.
Was this helpful? React with 👍 or 👎 to provide feedback.
| def _deduplicate_discovered_models( | ||
| discovered: list[DiscoveredModel], | ||
| ) -> list[DiscoveredModel]: | ||
| """Collapse duplicate agent identities and withhold conflicting price evidence. | ||
|
|
||
| Exact duplicate catalog rows become one candidate. When the same provider/model | ||
| identity is repeated with conflicting metadata or prices, one deterministic | ||
| transport record is retained but its prices become unknown. Provider row order | ||
| therefore cannot fabricate a cheaper bootstrap candidate or consume failover | ||
| capacity twice. | ||
| """ | ||
| unique: dict[tuple[str, str], DiscoveredModel] = {} | ||
| for model in discovered: | ||
| identity = _serving_identity(model) | ||
| previous = unique.get(identity) | ||
| if previous is None: | ||
| unique[identity] = model | ||
| continue | ||
| if previous == model: | ||
| continue | ||
| chosen = min((previous, model), key=_source_tiebreaker) | ||
| unique[identity] = replace( | ||
| chosen, | ||
| prompt_price_per_1k=None, | ||
| completion_price_per_1k=None, | ||
| ) | ||
| return list(unique.values()) |
There was a problem hiding this comment.
📝 Info: Duplicate-identity price withholding is order-independent
_deduplicate_discovered_models (model_discovery.py) reduces conflicting rows for the same (provider_name, model_id) via repeated min((previous, model), key=_source_tiebreaker) with prices nulled each step. Because replace() only nulls the price fields (leaving the tiebreaker metadata intact) and min over the pairwise reduction yields the globally lowest-tiebreaker record, the retained transport record and the None prices are deterministic regardless of provider response order, matching the docstring's claim. Exact-duplicate rows (previous == model) correctly collapse while preserving their price. No bug found here.
Was this helpful? React with 👍 or 👎 to provide feedback.
| raw = self._config.get(_PRICE_CATEGORY, _price_key(provider, "*"), None) | ||
| if raw is None: | ||
| return None | ||
| if not isinstance(raw, dict): | ||
| return None | ||
| try: | ||
| if ( | ||
| "prompt_price_per_1k" not in raw | ||
| or "completion_price_per_1k" not in raw | ||
| ): | ||
| return None | ||
| prompt_price = float(raw["prompt_price_per_1k"]) | ||
| completion_price = float(raw["completion_price_per_1k"]) | ||
| except (OverflowError, TypeError, ValueError): | ||
| return None | ||
| if ( | ||
| not math.isfinite(prompt_price) | ||
| or not math.isfinite(completion_price) | ||
| or prompt_price < 0 | ||
| or completion_price < 0 | ||
| ): | ||
| return None |
There was a problem hiding this comment.
📝 Info: get_price does not fall back to wildcard entry when the specific row is corrupt
In PriceBook.get_price (cost_ledger.py), the provider-wildcard fallback ("{provider}:*") is only consulted when the specific provider:model key is entirely absent. If the specific key exists but is corrupt/partial, the new validation returns None without attempting the wildcard default. This matches the pre-PR behavior (which also used the specific row without consulting the wildcard) so it is not a regression, but operators relying on a wildcard default price could be surprised when a corrupt specific row shadows it.
(Refers to this code)
Was this helpful? React with 👍 or 👎 to provide feedback.
* test: reproduce embedding deployment selected for chat synthesis * fix: isolate chat discovery from embedding endpoints * test: cover chat capability isolation defenses * docs: record embedding-to-chat incident boundary * docs: normalize incident references to APA 7 * test: reproduce stale embedding agent runtime selection * fix: enforce chat capability at runtime boundaries * refactor: share chat capability classifier * fix: activate runtime chat capability guards * test: tighten runtime guard coverage surface * test: cover runtime guard installation idempotence * docs: record stale-agent runtime containment * fix: integrate chat capability checks explicitly * docs: normalize capability incident formatting * ci: repair chat capability guards with source integration * ci: remove completed one-shot repair workflow * test: reproduce embedding deployment selected for chat synthesis * fix: isolate chat discovery from embedding endpoints * test: cover chat capability isolation defenses * docs: record embedding-to-chat incident boundary * docs: normalize incident references to APA 7 * test: reproduce stale embedding agent runtime selection * fix: enforce chat capability at runtime boundaries * refactor: share chat capability classifier * fix: activate runtime chat capability guards * test: tighten runtime guard coverage surface * test: cover runtime guard installation idempotence * docs: record stale-agent runtime containment * fix: integrate chat capability checks explicitly * docs: normalize capability incident formatting * ci: verify final chat capability guards * refactor: keep ModelClient internal to orchestrator module * test: separate chat transport from general synthesis roles * ci: verify transport and synthesis capability separation * chore: remove superseded transport-role repair workflow * test: remove out-of-scope transport-role experiment * test: reproduce chat transport and role boundary conflation * ci: verify chat transport and role boundaries safely * chore: remove branch-mutating repair workflow * test: remove discarded transport-role experiment * test: reproduce transport and agent-role capability conflation * ci: verify transport and ordinary-agent capability boundaries * chore: remove branch-mutating capability workflow * fix: separate chat transport from synthesis roles * test: reproduce non-chat passthrough and batch bypasses * ci: verify passthrough and batch chat capability guards * fix: replace chat capability repair workflow with reviewed source * test: preserve unknown model identifiers without capability fabrication * fix: avoid inferring guard roles from unrelated suffixes * ci: verify final chat capability contract * chore: remove completed capability repair workflow * ci: finalize chat transport and role capability boundaries * ci: finalize endpoint transport and agent role separation * chore: remove superseded capability repair workflow v2 * chore: remove superseded capability repair workflow v3 * ci: finalize chat capability boundaries on exact branch * fix: close chat capability review gaps * test: make unknown-identifier regression directly runnable * test: make transport-role regression directly runnable * test: cover legacy completions and direct execution * test: make chat-capability regression directly runnable * docs: register chat capability regression checks * fix: exclude shieldgemma from general chat roles * fix(discovery): bootstrap an honest provider-diverse failover pool (#770) * feat: add durable provider bootstrap coordinator * test: lock atomic provider bootstrap contracts * ci: sync provider credentials and model inventory * docs: record provider bootstrap trust boundary * fix(ci): hash-pin provider sync dependencies * fix: distinguish selected and durably enabled provider agents * docs: distinguish candidate selection from durable activation * fix: exclude non-chat catalog models from serving pool * fix: keep provider bootstrap capability-neutral * test: prohibit model-name capability inference * docs: prohibit capability inference from model names * feat: persist provider models with last-known-good recovery * ci: verify provider catalog normalization repair * test: define honest provider-diverse discovery bootstrap * test: bind NIM keys to one provider family * fix: rank unknown prices honestly and diversify bootstrap providers * fix: normalize durable provider catalog authority * docs: record strict 3NF provider catalog boundary * docs: add normalized provider catalog to database design * docs: keep provider catalog ADR at 0015 * refactor: expose colocated credential database DSN safely * refactor: use credential backend database contract * fix(discovery): wire provider-diverse bootstrap into CLI * test(discovery): expose ambiguous price and duplicate pool gaps * fix(discovery): reject untrusted duplicate price evidence * fix(discovery): keep duplicate evidence comparison total * test(discovery): expose overflow, corrupt-row, and currency gaps * fix(discovery): fail closed on malformed and incomparable price evidence * fix(cost): reject corrupt persisted price rows * test(bootstrap): expose overbroad provider secret trimming * fix(bootstrap): preserve provider secret bytes outside mounted line endings * test(catalog): expose failed credential promotion and LKG mismatch * fix(bootstrap): restore credentials after failed promotion * test(bootstrap): require rollback on unexpected discovery failure * test(cost): reject persisted partial price rows * fix(bootstrap): roll back promoted credentials on unexpected failure * fix: reject incomplete persisted price rows * docs: ground provider-diverse routing in research * docs: align catalog rollback schema * fix: normalize workflow secret leak guard * fix: report durable provider registrations * docs: define durable registration evidence * docs: remove trailing blank line * fix: preserve provider cost and outage-domain boundaries * fix: share chat capability policy with provider bootstrap * fix: address Devin review notes on currency gating and price fallback Two Info-level findings from the PR #768 Devin review, both real edge cases: - provider_bootstrap._known_cost_sort_key summed prompt+completion price with no currency check, unlike model_discovery's _discovery_price_key. A future non-USD-priced catalog row could outrank a pricier USD row. Now gated through the same _currency_is_comparable helper. - CostLedger.get_price returned None as soon as the specific provider:model row was malformed, skipping the provider:* wildcard fallback a valid wildcard price should still serve. Parsing is now tried per candidate (specific, then wildcard) instead of short- circuiting on the first non-None raw row. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: address Devin review notes on currency gating and price fallback Addresses the second round of Devin/CodeRabbit findings on PR #768: - provider_catalog_store._normalize_currency no longer collapses an unrecognized currency to "USD". A priced model with an unverified currency now gets an explicit unknown marker that fails _currency_is_comparable, instead of silently ranking as a comparable USD cost. - model_discovery._price_per_1k, provider_catalog_store._normalize_price, and cost_ledger.PriceBook._parse_price_entry now parse through Decimal first, so a nonzero price that underflows to 0.0 in float (e.g. a stray "1e-10000") is rejected as unknown rather than silently accepted as a legitimate free price. - PostgresProviderCatalogStore.record_success now clears model_serving_tag rows for the whole account before a refresh, matching the in-memory store: a model that drops out of discovery entirely no longer leaves orphaned serving-tag rows behind. - Registered the six new provider/discovery test files this PR added in README's canonical Check list and gave each a `raise SystemExit(pytest.main([__file__]))` entrypoint so they run standalone, not just under `python -m pytest tests`. - Fixed a docs/ADR consistency gap surfaced alongside these: distinguished "invalid" (rejected) from "unpriced" (retained as unknown fallback) pricing language in the discovery-routing doctoring note, and marked ADR 0015 accepted now that its scope is implemented on main. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * ci: retrigger strix after transient NVIDIA NIM rate-limit exhaustion The required strix security scan on c13c6ef failed closed after litellm.RateLimitError against nvidia_nim/nvidia/nemotron-3-super-120b-a12b exhausted all 3 retries, then the fallback model nvidia_nim/nvidia/llama-3.3-nemotron-super-49b-v1.5 also errored (exit code 2) before producing a vulnerability report artifact. No finding was reported; the gate failed closed on missing evidence, not a detected vulnerability. The workflow's ephemeral per-commit definition can't be rerun via the Actions API once complete, so retriggering with a fresh commit is the available path to a clean scan. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: reject prices that overflow to inf after Decimal-to-float conversion CodeRabbit caught a real regression from the previous underflow-guard commit: a Decimal like 1e10000 is finite as a Decimal, but float(decimal) overflows to inf, and neither _normalize_price (provider_catalog_store.py) nor PriceBook._parse_price_entry's _decimal_safe_price (cost_ledger.py) checked math.isfinite() on the value *after* converting to float, only decimal_value.is_finite() on the Decimal itself. An absurdly large price string could therefore slip through as a "valid" price of inf. model_discovery._price_per_1k already caught this via its trailing _valid_price_component() guard, so it needed no change (verified above, not just assumed). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(embeddings): bypass chat-only capability filter * fix(discovery): preserve explicit non-chat capabilities --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
1 participant
Root cause
The discovery bootstrap asked
PriceBook.compute_cost()to rank every candidate. A model without a trustworthy price row was therefore treated as zero-cost and could outrank every priced model. Selecting the first N rows could also enable several models from one provider, leaving the supposed fallback pool without independent provider paths.The same trust boundary had three additional failure modes:
That is unsafe for the central reviewer sidecar: unknown price is not free, ambiguous price is not comparable, and multiple model IDs—or the primary/sub credentials—behind the same NVIDIA NIM service are not independent provider failover.
Test-first repair
The branch records failing contracts before the corresponding production fixes, then implements a narrow selector and price-evidence boundary that:
nvidia_nimandnvidia_nim_subas one outage domain for the diversity pass;The accounting ledger's historical zero-cost recording fallback is unchanged. Discovery selection validates the existence and comparability of price evidence before using it, so missing or corrupt evidence is never reinterpreted as free routing cost.
Exact current identity
main@c620cfd4e5245c673ff830cd4b71e417db083102(stale pending prerequisite integration)183e7a8be1d6ee455843e2737869f75e61059d93An isolated execution of the focused selector regressions against the exact production functions completed with 12 passing tests. This is development evidence only; repository Tests, Fuzz, Security, Security Scan, Semgrep, coverage/review gates, and protected integration remain authoritative.
Prerequisite and reconciliation
PR #768 owns the shared ordinary-chat eligibility boundary. After #768 lands, this branch must rebase onto the resulting protected
mainand apply the shared general-chat classifier before price ordering. It must not reintroduce a local capability detector. NVIDIA primary/sub outage-family collapsing and this PR's price-evidence rules remain orthogonal and must be preserved.Scope
contextual_orchestrator/model_discovery.py, CLI wiring, and focused discovery regressions only. No credential value, provider endpoint, request payload, accounting record, reviewer identity, or merge-authority change.Keep Draft until prerequisite reconciliation and exact-head full verification are complete.