test(discovery): fix stale test left behind by #941's nvidia_nim/nvidia_nim_sub independence fix - #947
Closed
seonghobae wants to merge 1 commit into
Closed
Conversation
…vidia_nim_sub independence fix PR #941 (merged) removed model_discovery._provider_family and its nvidia_nim/nvidia_nim_sub collapsing, correctly updating every test that referenced _provider_family by name. tests/test_discovery_bootstrap_selection.py was missed because its test asserts the collapsed behavior by outcome, not by name: test_bootstrap_selector_treats_nim_primary_and_sub_as_one_outage_domain still asserted select_bootstrap_discovered_agents(...) == [nim_primary, openrouter], i.e. that nim_sub gets displaced by an unrelated provider to preserve "family" diversity -- the exact pool-shrinking result #941 was meant to eliminate. This left the test failing on main itself (verified directly against commit 42da1d5, the #941 merge commit, in an isolated worktree) and therefore failing the full-suite check on every open PR that merges/rebases onto current main. Root of the original premise: nvidia_nim and nvidia_nim_sub were collapsed into one "outage domain" on the unverified assumption that both KV credentials (NVIDIA_NIM_API_KEY / NVIDIA_NIM_API_KEY_SUB) expose the identical NVIDIA NIM model catalog. That was never checked against live evidence -- two different API keys hitting the same /v1/models URL does not guarantee the same result set -- and docs/planning/adrs/0015 already recorded the opposite: "NVIDIA primary and secondary keys are independent provider accounts." #941 independently reached the same conclusion and fixed the production code and five other test files; this fixes the sixth. Renamed to test_bootstrap_selector_keeps_nim_primary_and_sub_independent and corrected the assertion to [nim_primary, nim_sub], matching select_bootstrap_discovered_agents' actual behavior since #941 merged. Verify-revert checked: reverting this test to main's current content reproduces the exact failure; the fix passes. Full suite on this worktree (origin/main @ 42da1d5 + this one-file change): 2817 passed, 1 skipped, 1 failed (tests/test_psychometric_routing.py's fast_mlsirm ModuleNotFoundError -- a pre-existing, unrelated environment gap, not caused by this change). interrogate: 100%. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
|
Warning Review limit reachedNext included review available in 37 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 (2)
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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
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
Contributor
Author
|
Superseded — closing without merging. A concurrent PR, #945 ("fix(test): update stale NIM outage-domain test to match #941's independent-account contract"), fixed the exact same stale test and merged to No further action needed here; the underlying CI-red issue this PR targeted is already resolved on Generated by Claude Code |
seonghobae
pushed a commit
that referenced
this pull request
Aug 31, 2026
Brings in #945's already-merged, authoritative fix for the stale test_bootstrap_selector_treats_nim_primary_and_sub_as_one_outage_domain (main @ bffbc72, merged via 0adca47) instead of keeping this branch's own port of the equivalent #947 fix, which #945 beat to main and #947 was then closed as superseded. Conflict in tests/test_discovery_bootstrap_selection.py resolved by taking main's version of that one test function entirely (git checkout --theirs) rather than reconciling two different renames of the same fix. Added a short "Update" note to this branch's earlier docs/product-technical-gap-baseline.md entry recording that #945 landed first; no other file conflicted. tests/test_discovery_bootstrap_selection.py -q: 14 passed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
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
Follow-up to #941 ("fix(discovery): keep credential accounts independent"), which removed
model_discovery._provider_family's collapsing ofnvidia_nim/nvidia_nim_subinto one "outage domain" family. That collapse was itself an unverified premise (no evidence in this repo ever compared what the two NVIDIA NIM KV credentials'/v1/modelscalls actually return;docs/planning/adrs/0015-durable-provider-catalog.md, accepted 2026-08-22, already recorded the opposite: "NVIDIA primary and secondary keys are independent provider accounts"). #941 fixed the production code and every test file its own_provider_familygrep found.One test was missed because it asserts the old collapsed behavior by outcome, not by referencing
_provider_familyby name:tests/test_discovery_bootstrap_selection.py::test_bootstrap_selector_treats_nim_primary_and_sub_as_one_outage_domainstill assertedselect_bootstrap_discovered_agents([nim_sub, openrouter, nim_primary], price_book, 2) == [nim_primary, openrouter]— i.e. thatnim_sub(the second-cheapest candidate overall) gets displaced by an unrelated, costlier provider to preserve "family" diversity. That's exactly the pool-shrinking result fix(discovery): keep credential catalogs independent #941 was meant to eliminate.This left
mainitself red — verified directly against commit42da1d53(the #941 merge commit) in an isolatedgit worktree, independent of any other local branch state:Since it's inherited breakage on
main, it fails the full-suite check on every open PR that merges/rebases onto currentmain, not just this repo's own CI.Fix
Renamed to
test_bootstrap_selector_keeps_nim_primary_and_sub_independentand corrected the assertion to[nim_primary, nim_sub], matchingselect_bootstrap_discovered_agents's actual behavior since #941 merged (both NVIDIA credentials now win their own diversity-guaranteed slot ahead of the unrelated, costlieropenroutercandidate). Also appended a dated entry todocs/product-technical-gap-baseline.mdrecording the gap and the fix, per this repo's append-only convention.No production code changed —
_provider_familyno longer exists anywhere in the source tree per #941, and a full-repo grep confirms no other test still references the removed collapsing behavior.Verification
main's current content reproduces the exact failure above; the fix passes.tests/test_discovery_bootstrap_selection.py— 14 passed.origin/main@42da1d53+ only this change):PYTHONPATH=. coverage run -m pytest tests -q→ 2817 passed, 1 skipped, 1 failed. The one remaining failure (tests/test_psychometric_routing.py::test_fast_mlsirm_fit_uses_judge_acceptance_item_for_context_score,ModuleNotFoundError: No module named 'fast_mlsirm') is a pre-existing, unrelated sandbox/environment gap (missingfast_mlsirm/numpy), not caused by this change.interrogate: 100%.Test plan
tests/test_discovery_bootstrap_selection.pypassespython -m pytest tests -qclean except the pre-existingfast_mlsirmenvironment gapinterrogate100%main, new assertion passes_provider_family/outage-domain references remain🤖 Generated with Claude Code
https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
Generated by Claude Code