fix(discovery): keep credential catalogs independent - #941
Merged
Conversation
|
Warning Review limit reachedNext included review available in 13 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (17)
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 |
Contributor
Author
|
일반 계약: 모든 KV credential은 독립 provider-account/catalog boundary입니다. 같은 vendor/endpoint/model ID라도 서로 다른 key의 모델·권한·가격·privacy·가용성·실패 상태를 같다고 추정하거나 dedupe하지 않습니다. provider family는 없으며, 논리적 모델 동등성과 latency/quality evidence 공유는 오직 operator가 명시한 model_group으로만 성립합니다. |
Contributor
Author
|
최종 일반 계약: 모든 KV credential은 독립 provider-account/catalog boundary입니다. 같은 vendor, endpoint, model ID라도 서로 다른 key의 모델, 권한, 가격, privacy, 가용성, 실패 상태를 같다고 추정하거나 dedupe하지 않습니다. provider family는 없으며, 논리적 모델 동등성과 latency/quality evidence 공유는 오직 operator가 명시한 model_group으로만 성립합니다. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Comment on lines
+447
to
+449
| def _serving_identity(model: DiscoveredModel) -> tuple[str, str, str]: | ||
| """Return the account-scoped identity used by discovery synchronization.""" | ||
| return (model.provider_name, model.credential_name, model.model_id) |
Contributor
There was a problem hiding this comment.
🔴 Account-specific models share serving identities
When two credentials expose one provider and model, _serving_identity retains both but agent_id_for gives them one ID. Synchronization overwrites one account, and either account’s pricing can govern both.
Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
+447
to
+449
| def _serving_identity(model: DiscoveredModel) -> tuple[str, str, str]: | ||
| """Return the account-scoped identity used by discovery synchronization.""" | ||
| return (model.provider_name, model.credential_name, model.model_id) |
Contributor
There was a problem hiding this comment.
🔴 Account prices overwrite each other
When those accounts report different prices, refresh_price_book stores both under one provider-model key. The final row prices every account, corrupting selection and spend calculations.
Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
+234
to
+237
| if model.provider_name in seen_providers: | ||
| continue | ||
| selected.append(model) | ||
| seen_providers.add(provider_family) | ||
| seen_providers.add(model.provider_name) |
Contributor
There was a problem hiding this comment.
🟡 Credential diversity still collapses accounts
Both bootstrap selectors use provider names as diversity keys, so same-provider credential accounts compete for one first-pass slot. Tight limits exclude independent accounts.
Prompt for agents
Use the full provider-account identity, at minimum provider_name plus credential_name, as the diversity key in both select_provider_diverse_models and select_bootstrap_discovered_agents. Add tests with two sources sharing provider_name but using different credential names, plus another provider and a limit smaller than the full catalog, to verify each credential account receives an independent first-pass slot.
Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
+447
to
+449
| def _serving_identity(model: DiscoveredModel) -> tuple[str, str, str]: | ||
| """Return the account-scoped identity used by discovery synchronization.""" | ||
| return (model.provider_name, model.credential_name, model.model_id) |
Contributor
There was a problem hiding this comment.
🔍 Failure attribution remains provider-scoped
Same-provider accounts cannot report mixed discovery outcomes independently. One account failure marks successful siblings failed and rolls back their valid credentials.
Was this helpful? React with 👍 or 👎 to provide feedback.
…ry-latency-routing
Signed-off-by: Seongho Bae <me@seonghobae.me> Commit-Message-Assisted-by: Claude (via Claude Code)
This was referenced Aug 31, 2026
seonghobae
pushed a commit
that referenced
this pull request
Aug 31, 2026
Resolves the --verbose/logging.basicConfig conflict with #941's "secret-free provider discovery diagnostics" against __main__.py: keeps one --verbose/--debug flag per parser (mine, with the CONTEXTUAL_ORCHESTRATOR_VERBOSE env default and --debug alias) and folds #941's model_discovery.py DEBUG diagnostics into _configure_logging's audited logger scope now that they're verified secret-free (account/error_code/model_count only, never a raw exception or credential). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
seonghobae
pushed a commit
that referenced
this pull request
Aug 31, 2026
…R 0125 Devin's review (PR #942) caught that the "already-deployed server can turn it on without a CLI edit" phrasing could be misread as live/dynamic toggling; the env var is read once at process startup like this file's four existing env-backed CLI defaults and still requires a restart. Makes that explicit everywhere the claim appears (README, CHANGELOG fragment, the --serve --help text), and rewrites ADR 0125's Decision/Excluded sections to describe the corrected _configure_logging design (scoped per-logger DEBUG, not the root logger) plus #941's folded-in model_discovery diagnostics and the server.py streaming-latency fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
seonghobae
pushed a commit
that referenced
this pull request
Aug 31, 2026
…removal #941 removed _provider_family() (nvidia_nim/nvidia_nim_sub collapsing) but didn't touch tests/test_discovery_bootstrap_selection.py, leaving test_bootstrap_selector_treats_nim_primary_and_sub_as_one_outage_domain asserting the now-removed collapsing behavior. Updated to assert the correct independent-provider behavior. Also folds this PR's logging additions to match #941's "account="-prefixed message convention. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kj32ABZLZ2a6TPTyvYrRkg
This was referenced Aug 31, 2026
seonghobae
pushed a commit
that referenced
this pull request
Aug 31, 2026
…ndent-account contract test_bootstrap_selector_treats_nim_primary_and_sub_as_one_outage_domain asserted the pre-#941 behavior (select_bootstrap_discovered_agents collapsing nvidia_nim/nvidia_nim_sub into one provider family). 5224a4c (part of #941) deliberately removed that grouping -- documented in docs/product-technical-gap-baseline.md and ADR 0032 as superseded, since each credential's API key may be entitled to a different model catalog -- but left this one test asserting the old behavior, making it fail unconditionally on main. Verified this fails identically on a clean origin/main checkout with none of this PR's changes present. No production code changed; the test now asserts the documented, already-shipped independent-account contract.
seonghobae
pushed a commit
that referenced
this pull request
Aug 31, 2026
Full-suite run surfaced two failures #941 (which removed model_discovery._provider_family) missed: - test_discovery_bootstrap_selection.py's nim-primary/sub test asserted the old family-collapsing outcome for select_bootstrap_discovered_agents (a sibling of provider_bootstrap.py's select_provider_diverse_models, which #941 did update) -- nvidia_nim and nvidia_nim_sub are independent providers now, so both occupy first-pass diversity slots directly. - test_model_discovery_boundaries.py's "unclassified failure" contract test used a plain RuntimeError as its example of an unclassified type, but RuntimeError is now deliberately classified (transport_error, for the configured-gateway DNS/validation wrapper). Switched to KeyError, a type genuinely outside every classified branch.
seonghobae
added a commit
that referenced
this pull request
Aug 31, 2026
…-domain-test fix(test): update stale NIM outage-domain test to match #941's independent-account contract
seonghobae
pushed a commit
that referenced
this pull request
Aug 31, 2026
PR #942's "Full unit and contract suite" check was failing at every push -- not from anything #942 touches, but from a pre-existing stale test on main itself: tests/test_discovery_bootstrap_selection.py:: test_bootstrap_selector_treats_nim_primary_and_sub_as_one_outage_domain still asserted the collapsed-family behavior PR #941 (merged) deliberately removed from select_bootstrap_discovered_agents. #941 updated every test file its own _provider_family grep found; this one was missed because it asserts the collapsed behavior by outcome ([nim_primary, openrouter]), not by referencing _provider_family by name. Independently verified before porting: _provider_family no longer exists anywhere in contextual_orchestrator/model_discovery.py on this branch, and running this exact test against this branch (before this commit) reproduces the failure -- confirming it is a pre-existing gap on main, not caused by this PR's diff, and identical to what was failing on every other open PR merging against current main. Ports the identical fix already reviewed and verified in contextual-orchestrator#947 (branch fix/discovery-bootstrap-selection-stale- test, commit 90c5689) rather than blocking on that PR merging first, per the drive-to-green policy: renames the test to test_bootstrap_selector_keeps_nim_primary_and_sub_independent and corrects the assertion to [nim_primary, nim_sub], plus the matching gap-baseline doc entry. No production code touched. This will no-op once #947 lands on main. tests/test_discovery_bootstrap_selection.py -q: 14 passed (was 13 passed, 1 failed before this commit). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
seonghobae
added a commit
that referenced
this pull request
Aug 31, 2026
) * fix(discovery): retry a provider's transient model-list fetch once discover_provider_models made exactly one HTTP attempt per provider and raised ProviderDiscoveryError immediately on any transient failure, zeroing out that whole provider's contribution to the discovery pass. Observed live: a single Bytez HTTP 500 during a hosted noema-review run propagated all the way to the org's shared orchestrator/free review pool failing closed for every consumer repo, even though Bytez models were never free-eligible in the first place (a separate, pre-existing fact unrelated to this specific incident). Reuse the existing is_transient_error classifier (already trusted for completion-call retries) to add one bounded retry -- short fixed delay, shortened timeout -- for exactly the 5xx/timeout/connection-reset class that a retry can actually fix. Non-transient failures (bad credential, malformed response) are never retried, matching existing behavior. Deliberately out of scope: ModelClient.proxy_send_once's single-shot completion-call guarantee ("cross-provider failover cannot amplify load") is left untouched. Retrying there too was considered and rejected after review found it risks stacking latency past CI callers' own time budgets and reintroducing exactly the request amplification that guarantee exists to prevent. * fix(discovery): cap retry timeout at the caller's own budget Devin review on #923: the retry attempt hardcoded _DISCOVERY_RETRY_TIMEOUT_SECONDS (5.0s) regardless of what timeout the caller requested, so a caller budgeting e.g. 2s per attempt could see the retry alone exceed that budget. Use min(timeout, _DISCOVERY_RETRY_TIMEOUT_SECONDS) instead. * fix(server): allow stream_options.include_usage with tools passthrough Root-caused a live Strix required-check failure blocking contextual-orchestrator#923: openai.BadRequestError 400 invalid_stream_options, "stream_options.include_usage=true is not supported with tools or response_format" -- raised by this gateway's own /v1/chat/completions validation, not an upstream provider limitation. Strix's openai-agents SDK always sends tools + stream_options.include_usage=true together, which is normal agentic-client behavior. proxy_completion's single-agent tool passthrough always fetches a complete, non-streamed upstream response (upstream["stream"] = False) and frames it locally via _chat_response_sse_chunks, which already emits a real, honestly-labeled usage chunk (usage_source: reported/estimated) alongside tool-call deltas -- the combination was already fully supported downstream, so the upfront rejection was a stale, avoidable restriction. Narrowed the check to still reject response_format's separate multi-agent "conduct" path, which has no equivalent aggregate-usage story yet. Updated the two tests whose docstrings encoded the old (incorrect) assumption that "structured passthrough cannot emit usage SSE" -- both now verify the tools case returns a real 200 SSE with a usage chunk, while response_format-only still fails closed with invalid_stream_options. Verified: targeted files 15 passed; full suite run in progress. * fix(orchestrator): unwrap URLError-wrapped TLS cert failures in is_transient_error Devin review on #923 found: urlopen wraps a TLS handshake's ssl.SSLCertVerificationError as URLError(reason=...), not as a bare ssl.SSLError. is_transient_error's blanket "any URLError is transient" branch matched first and returned True before the existing bare-SSLError unwrap could ever see it, so a permanently invalid certificate was being retried as if it were a network blip. Fixed the shared classifier itself (not just the discovery retry call site Devin's suggested diff targeted), so every current and future caller benefits. Added a regression test covering both the previously-broken case and that an ordinary URLError(ConnectionResetError(...)) is still correctly transient. Verified: targeted test_provider_reliability.py 25 passed; full suite run in progress. * fix(discovery): isolate DNS/body-read failures instead of aborting Two real gaps Devin's review found in #923's own retry loop, both capable of crashing the entire discover_all_models sweep instead of isolating a single provider's failure: - provider_error_body's exc.read() had no guard, so a stalled/dropped connection raising http.client.IncompleteRead (not an OSError subclass) during is_transient_error's HTTP-error-body inspection would escape classification entirely. Now degrades to an empty body on any read failure, matching safe_provider_message's existing "bodies are untrusted input" handling. - discover_provider_models's retry loop didn't catch RuntimeError, but ModelClient._resolve_addresses (used by the configured_gateway transport) wraps a DNS resolution failure as plain RuntimeError. Now caught and classified as transport_error, isolated to that provider. * fix(discovery): retry a temporary DNS failure through its RuntimeError wrapper Devin's follow-up on the prior DNS-isolation fix: ModelClient._resolve_addresses wraps socket.gaierror as plain RuntimeError, so is_transient_error rejected it outright -- a genuinely temporary DNS hiccup (EAI_AGAIN) was isolated instead of retried, unlike every other transient failure this PR retries. is_transient_error now unwraps a RuntimeError's __cause__ and defers to the existing EAI_AGAIN check when it's a socket.gaierror; any other RuntimeError (malformed URL, no resolvable address) still falls through to non-transient. * test(discovery): fix two tests left stale by the provider-family removal Full-suite run surfaced two failures #941 (which removed model_discovery._provider_family) missed: - test_discovery_bootstrap_selection.py's nim-primary/sub test asserted the old family-collapsing outcome for select_bootstrap_discovered_agents (a sibling of provider_bootstrap.py's select_provider_diverse_models, which #941 did update) -- nvidia_nim and nvidia_nim_sub are independent providers now, so both occupy first-pass diversity slots directly. - test_model_discovery_boundaries.py's "unclassified failure" contract test used a plain RuntimeError as its example of an unclassified type, but RuntimeError is now deliberately classified (transport_error, for the configured-gateway DNS/validation wrapper). Switched to KeyError, a type genuinely outside every classified branch. --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: opencode-agent[bot] <219766164+opencode-agent[bot]@users.noreply.github.com>
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\n\n- Treat every KV credential as an independent provider-account/catalog boundary, including identical vendor endpoints and model IDs.\n- Remove provider-family collapsing; only explicit model_group membership asserts logical model equivalence and shares measured routing evidence.\n- Add secret-free verbose discovery diagnostics.\n- Report per-member max_observed_rpm and max_observed_tpm from real 60-second completion windows; TPM uses provider-reported total_tokens only and creates no probe traffic.\n- Keep the paper-grounded routing score: Beta-Bernoulli stability divided by Jacobson EWMA latency, with RouteLLM and FrugalGPT evidence already committed under docs/papers/.\n\n## Validation\n\n- Routing, failover, discovery focused suite: 216 passed\n- Discovery/bootstrap/CLI focused suite: 152 passed\n- Ruff E9,F on changed Python/test files: passed\n- git diff --check: passed