fix(discovery): exclude vision-input-only models from orchestrator/free - #933
Conversation
ContextualWisdomLab/.github PR #1198's required Strix Security Scan check
failed (run 33325907333, job 99295892400): 3 independent scan attempts each
hit NVIDIA NIM's meta/llama-3.2-90b-vision-instruct via
orchestrator/free and got an identical HTTP 400 invalid_request_error
("Adjust the request parameters and retry"), exhausting the whole free pool
against this one agent.
Root cause: this model is zero-priced (Models.dev reports cost 0/0) and
passes every existing chat-capability check (text output modality, no
disqualifying model-id token), so free_discovered_models() admitted it into
the general-purpose free pool that Strix's tool-calling requests route
through. Models.dev's own tool_call field claims true for this exact model
(verified live against models.dev/api.json), so that field cannot gate this;
its declared input modality (text + image) is the only honest catalog
evidence that distinguishes it from an ordinary text-only free worker.
Two alternate fixes were considered and rejected against this repo's own
tested contracts:
- Loosening ModelClient's cross-provider failover to retry a plain HTTP 400
on a different free candidate is explicitly foreclosed by
test_non_transient_error_is_not_replayed and
test_virtual_passthrough_keeps_non_size_tool_errors_sticky
(tests/test_passthrough_provider_failover.py): "caller errors fail closed
instead of duplicating a request across providers."
- The circuit breaker never records a sticky (non-failover-eligible)
rejection at all, but recording it would not have prevented this incident
either: each Strix attempt is a fresh sidecar process/gateway instance
(in-memory circuit state), and the observed failure is the very first
request each attempt makes.
Fix: free_discovered_models() now excludes a free model that declares a
non-text input modality from the general-purpose free pool. This is
pool-composition, not per-request retry: the model stays fully discovered
and price-evidenced (available to a pool that explicitly wants a
vision/multimodal capability) and is only withheld from the
capability-blind orchestrator/free default. Scoped to the free selector
specifically (not general chat-candidate eligibility) so a genuinely
tool-capable paid multimodal model elsewhere is unaffected.
Known limitation: NVIDIA NIM's own /v1/models listing carries no
tool/function-calling capability metadata at all, and Models.dev's tool_call
field is unreliable at the per-deployment granularity (proven wrong here).
If NIM's remaining free text-only catalog also turns out to lack genuine
tool-calling support for some models, broader free-tier discovery
(OpenRouter/Bytez/OpenAI free-tier offerings, today evidence-only or never
free) is a follow-up, not something this change can fabricate evidence for.
TDD: tests/test_model_discovery.py::test_free_discovered_models_excludes_a_free_vision_only_input_model
reproduces the bug against a DiscoveredModel fixture shaped like the broken
NIM agent (fails pre-fix, passes post-fix); a text-only free model and a
free model with no modality evidence at all remain eligible.
Verified: python -m pytest tests -q -> 2770 passed, 1 skipped.
interrogate (fail-under 100) -> 100.0%. python tests/test_self_check.py and
the naming-convention/chat-capability/provider-bootstrap check scripts named
in README.md all pass individually too.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX
|
Warning Review limit reachedNext included review available in 19 seconds. 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 (11)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (14)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthrough무료 모델의 가격 기반 인벤토리와 일반 무료 서빙 후보를 분리했다. 비텍스트 입력 modality를 선언한 에이전트는 Changes무료 모델 서빙 선택
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🔵 Low · up to The change prevents multimodal free models from entering the default text-serving pool, but the new serving-candidate path can still expose provider records that are evidence-only to direct consumers that bypass later validation. The PR is mergeable with explicit owner awareness and follow-up to enforce that serving candidates are always routable records. Sequence Diagram(s)sequenceDiagram
participant ModelDiscovery
participant TaskOrchestrator
participant OpenAIModelListing
ModelDiscovery->>ModelDiscovery: free_discovered_models(discovered)
ModelDiscovery->>ModelDiscovery: general_free_serving_candidates(discovered)
ModelDiscovery->>TaskOrchestrator: discovered agents with cost:free tags
TaskOrchestrator->>TaskOrchestrator: _is_free_agent(agent)
TaskOrchestrator->>OpenAIModelListing: FREE_MODEL selection
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 93.55% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 31 functions across 12 files. (2 skipped: 1 unsupported, 1 too large.) ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…hestrator Devin's review on PR #933 found the vision-input exclusion added in 94e3b9a was incomplete on three points: 1. Runtime free pool remained unfiltered: `_auto_discover_runtime_agents` (`--auto-discover-model-agents`) and `provider_bootstrap`'s `_active_agent_from_discovered` (used by `bootstrap_provider_runtime` and, through it, `provider_catalog_bootstrap.bootstrap_provider_catalog_runtime`) both tag an agent `cost:free` from raw price evidence alone and never consulted the new exclusion, so NVIDIA NIM's free `meta/llama-3.2-90b-vision-instruct` could still reach a live, blindly-selectable `cost:free` agent through either path -- reproducing the original Strix tool-calling incident (ContextualWisdomLab/.github#1198). 2. `free_discovered_models()` conflated price-based inventory with serving-pool eligibility, so `--free-only`, `free_tier_count`, and the free-tier data-privacy totals silently undercounted a model that is genuinely free-priced but unfit for blind serving. 3. (Evaluated, not adopted) Devin also suggested narrowing the exclusion to spare a model that "also supports text as a standalone input". Rejected against this repo's own incident evidence: the incident model itself declares both `text` and `image` per Models.dev, so that narrowing would have silently re-admitted the exact model this fix is about. Kept the conservative "any declared non-text input modality disqualifies" reading, documented with a fixture for all three modality shapes (text-only, vision-only, text+image). Fix: - `free_discovered_models()` is pure price-based inventory again. - New `model_discovery.general_free_serving_candidates()` carries the modality-based exclusion for composing a blind free pool, wired into the `discover-models` CLI report as `general_free_serving_count` alongside the restored `free_tier_count`. - The actual enforcement is now `TaskOrchestrator._is_free_agent`, a single choke point every `orchestrator/free` selection path shares: an agent whose tags declare a non-text `input:<modality>` is never treated as free-pool eligible there, regardless of which code built it or how old that agent-pool row is (protects durable pool-store rows written before this exclusion existed, and any future pool-construction path). `cost:free` keeps meaning "honest zero price" everywhere else, preserving `provider_catalog_store.py`'s durable `is_free` round trip (see `test_serving_tags_preserve_only_explicit_free_and_modality_evidence` and `test_last_known_good_restores_free_and_modality_evidence`, both left unchanged). - `review_gateway.py` audited: its agents never carry a `cost:free` tag (tags are fully replaced with `("review",)`) and no price is registered, so it was never reachable through this bug; no change needed there. Also brought `interrogate --fail-under=100` back to green: it was already failing at 97.9% on the unmodified base commit (94e3b9a) from six pre-existing undocumented `fuzz/*.py` harness functions, unrelated to this incident; added their docstrings. TDD: new/relocated tests fail against the pre-fix source (verified by temporarily restoring it) and pass after the fix -- `test_auto_discovery_activates_a_free_vision_model_but_free_pool_excludes_it`, `test_active_agent_from_discovered_free_vision_model_is_not_free_pool_eligible`, `test_free_discovered_models_still_counts_a_free_vision_only_input_model`, `test_general_free_serving_candidates_modality_shapes`, and the relocated `test_general_free_serving_candidates_excludes_a_free_vision_only_input_model`. Verified: python -m pytest tests -q -> 2774 passed, 1 skipped (2770 passed, 1 skipped on the unmodified base commit; net +4 tests, zero regressions). interrogate (fail-under 100) -> 100.0%. python tests/test_conventions.py passes (new function name is valid two-or-more-word snake_case). git diff --check clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Pushed 5648567 addressing all three of Devin's review findings on this PR. Replies are inline on each of the three original threads; summary below.
What changed, and why
Finding 1 (🔴 runtime free pool remains unfiltered) -- fixed at a single choke point, not per call site.
Confirmed: _auto_discover_runtime_agents (--auto-discover-model-agents) and provider_bootstrap._active_agent_from_discovered (used by bootstrap_provider_runtime and, through it, provider_catalog_bootstrap.bootstrap_provider_catalog_runtime) both tag an agent cost:free from raw price evidence alone and never consulted the exclusion this PR added -- the vision model could still reach a live, blindly-selectable cost:free agent through either path. Rather than repeat the same filter at every pool-construction site (which is exactly the kind of gap this finding is about -- "not just the one function this PR touched"), the enforcement now lives in TaskOrchestrator._is_free_agent, the single method every orchestrator/free selection path already shares. It re-checks an agent's persisted input:<modality> tags at selection time, so it also protects a durable agent-pool row written by an older build before this exclusion existed, and any future pool-construction path this repo adds later, not just the ones audited today.
Audited all four files this finding named:
_auto_discover_runtime_agents-- confirmed vulnerable pre-fix.provider_bootstrap.py/provider_catalog_bootstrap.py--_active_agent_from_discoveredhad the identical gap, confirmed vulnerable pre-fix.review_gateway.py-- audited and found not reachable through this bug:build_review_orchestratorreplaces every constructed agent's tags with("review",)(nocost:freeever survives) and never populatesprice_per_million, so_is_free_agentwas already unconditionallyFalsefor every agent it builds. No change made there.
Finding 2 (🟡 multimodal support empties free pools) -- investigated and not adopted as suggested, with reasoning.
The suggested narrowing ("keep text+image models eligible when text is a supported standalone input") would silently reopen this exact incident: the incident model itself (meta/llama-3.2-90b-vision-instruct) declares input_modalities=("text", "image") per Models.dev, so it already satisfies that suggested test -- yet it's precisely the model NIM's live deployment rejected a tool-calling request against three times in a row. Models.dev's input_modalities field documents generically supported inputs, not which ones a specific deployment's tool-calling path actually honors, and this PR's own commit already documents that no more reliable per-deployment tool-calling signal exists (NIM's /v1/models has none; Models.dev's tool_call field claims true for this exact model and is proven wrong by the incident). Narrowing the check would have made the fix inert for the model it exists to fix.
Kept the conservative "any declared non-text input modality disqualifies" reading, and added the requested explicit three-fixture coverage anyway (text-only / vision-only / text+image), documenting in both the code and the test why text+image stays excluded. The legitimate underlying concern -- a provider's free pool silently going empty -- is now addressed by visibility instead: general_free_serving_count in the discover-models CLI report (see finding 3) lets an operator actually notice that, rather than the fix quietly admitting a request-breaking model back in to avoid it.
Finding 3 (🟡 free-model reports undercount inventory) -- fixed exactly as suggested.
free_discovered_models() is pure model.is_free price-based inventory again (restores correct --free-only, free_tier_count, and the free-tier data-privacy totals). A new, separately named selector, general_free_serving_candidates() -- the same name suggested -- carries the modality exclusion specifically for composing the blind orchestrator/free pool, and is now also surfaced as general_free_serving_count in the CLI report so the split is externally visible, not just internally correct.
Test evidence (TDD, all four new/relocated tests verified red against the pre-fix source by temporarily restoring it, then green after the fix)
test_auto_discovery_activates_a_free_vision_model_but_free_pool_excludes_it(finding 1,_auto_discover_runtime_agentspath)test_active_agent_from_discovered_free_vision_model_is_not_free_pool_eligible(finding 1,provider_bootstrap.pypath)test_general_free_serving_candidates_modality_shapes(finding 2, explicit text-only/vision-only/text+image fixtures)test_free_discovered_models_still_counts_a_free_vision_only_input_model(finding 3)test_general_free_serving_candidates_excludes_a_free_vision_only_input_model-- the original PR's regression test, relocated onto the new selector, intent unchanged
Full verification
python -m pip install --require-hashes -r requirements.lock && python -m pip install --no-deps -e .(Python 3.12)python -m pytest tests -q-> 2774 passed, 1 skipped (base commit 94e3b9a: 2770 passed, 1 skipped -- net +4 tests, zero regressions)interrogate-> 100.0% (was already failing at 97.9% on the unmodified base commit from six pre-existing undocumentedfuzz/*.pyharness functions, unrelated to this incident; fixed those docstrings too since the task's own gate expects 100%)python tests/test_conventions.py-> passes (general_free_serving_candidatesis valid two-or-more-word snake_case)git diff --check-> clean
Commit: 5648567, pushed to fix/nim-vision-tool-calling-pool (fast-forward from 94e3b9a).
Generated by Claude Code
| model | ||
| for model in free_discovered_models(discovered) | ||
| if not _requires_non_text_input(model) | ||
| ] |
There was a problem hiding this comment.
Fixed in 5648567, but not by patching each pool-construction call site individually (that's still whack-a-mole against a future one). The actual enforcement is now a single choke point: TaskOrchestrator._is_free_agent re-checks an agent's input:<modality> tags at selection time, so every orchestrator/free path is covered -- _auto_discover_runtime_agents, provider_bootstrap._active_agent_from_discovered (and provider_catalog_bootstrap through it), review_gateway.py, and any future pool-construction path -- including a durable agent-pool row written by an older build before this exclusion existed, which per-call-site filtering at construction time can't retroactively fix.
Audited all four named files:
_auto_discover_runtime_agents: confirmed vulnerable pre-fix (new testtest_auto_discovery_activates_a_free_vision_model_but_free_pool_excludes_it, fails red against the old source).provider_bootstrap.py/provider_catalog_bootstrap.py:_active_agent_from_discoveredhas the identical gap (new testtest_active_agent_from_discovered_free_vision_model_is_not_free_pool_eligible, also fails red pre-fix).review_gateway.py: audited, not actually reachable through this bug -- itsbuild_review_orchestratorfully replaces agent tags with("review",)(nocost:freeever survives) and never setsprice_per_million, so_is_free_agentwas already alwaysFalsethere. No change needed.
Generated by Claude Code
| model | ||
| for model in free_discovered_models(discovered) | ||
| if not _requires_non_text_input(model) | ||
| ] |
There was a problem hiding this comment.
Fixed exactly as suggested. free_discovered_models() is pure model.is_free inventory again (restores correct --free-only, free_tier_count, and the free-tier data-privacy totals), and a new general_free_serving_candidates() -- same name you proposed -- carries the modality exclusion for orchestrator/free composition specifically. It's also now surfaced as general_free_serving_count in the discover-models CLI report alongside the restored free_tier_count, so the split is visible, not just internally correct.
New regression test: test_free_discovered_models_still_counts_a_free_vision_only_input_model asserts free_discovered_models([vision_model]) == [vision_model] while general_free_serving_candidates([vision_model]) == [] -- fails red against the pre-fix source (where the vision model was missing from the old free_discovered_models's output entirely).
Only pool-serving callers were updated to route through the new selector's underlying logic; see the top-of-file thread for how that's actually enforced (a single TaskOrchestrator._is_free_agent choke point rather than threading the selector through every call site, which was the more robust fix for finding 1).
Generated by Claude Code
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…at only Devin's second review round on PR #933 (against 5648567) found the modality exclusion had overshot: TaskOrchestrator._is_free_agent had grown a non-text-input check meant only for the capability-blind general chat pool, but it also backed every capability-scoped free route (_capability_agents -> /v1/audio/transcriptions, /v1/videos, image, speech, rerank) and server.py's _require_pool_model. A free transcription agent naturally carries an input:audio tag and a free image/video agent an input:image tag -- exactly the modality its own capability-scoped free route is asking for, not a surprise -- so the shared predicate made those genuinely free agents unreachable through their own free route. Fix: split the predicate. - _is_free_agent reverts to plain, modality-blind price evidence (used by _capability_agents/_ranked_agents when chat_only=False, and by server.py's capability-scoped _require_pool_model branch). - New _is_general_free_agent = _is_free_agent(agent) and not _agent_requires_non_text_input(agent) is the stricter, general-chat-only variant, now used at every blind general-chat FREE_MODEL call site (proxy_completion, _orchestrated_provider_completion, route_once, conduct, _ranked_agents when chat_only=True, list_openai_models's advertising check, and server.py's capability-agnostic _require_pool_model branch). Also addressed the review's two informational notes: - "Duplicate serving policies can drift": extracted the actual "what counts as non-text" classification into chat_capability.requires_non_text_input, a single shared predicate both model_discovery._requires_non_text_input (DiscoveredModel.input_modalities) and orchestrator._agent_requires_non_text_input (an agent's input:<modality> tags) now delegate to, so the two representations of the same catalog evidence cannot diverge independently. New cross-consistency test: test_discovery_and_orchestrator_modality_eligibility_cannot_drift. - "Serving count uses a different population": documented in __main__.py that free_tier_count and general_free_serving_count are deliberately both computed over the complete `discovered` population regardless of --free-only, matching each other's established convention (not a new inconsistency). TDD: new tests fail against 5648567 (verified by temporarily inserting them against that commit) and pass after this fix -- test_free_virtual_model_selects_a_free_agent_whose_own_capability_needs_non_text_input (orchestrator._capability_agents path), test_require_pool_model_serves_capability_free_route_despite_non_text_input (server._require_pool_model path), plus updated assertions on the two existing regression tests from the first round confirming _is_free_agent now returns True (capability-reachable) while _is_general_free_agent stays False (blind-chat-excluded) for the same agent. Verified: python -m pytest tests -q -> 2775 passed, 1 skipped (net +1 test over the previous round's 2774; zero regressions). interrogate -> 100.0%. python tests/test_conventions.py passes. git diff --check clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Pushed efd44f6 addressing the second review round's findings on 5648567. Replies are inline on the three new threads; summary below.
What changed, and why
🟡 "Free media routes reject eligible models" -- real regression from round 1, fixed by splitting the predicate.
TaskOrchestrator._is_free_agent had grown a non-text-input exclusion meant only for the capability-blind general chat pool, but it also backed _capability_agents (every /v1/audio/transcriptions, /v1/videos, image, speech, rerank route) and, separately, server.py's _require_pool_model. A free transcription agent naturally carries an input:audio tag and a free image/video agent an input:image tag -- exactly the modality its own capability-scoped free route is asking for, not a surprise -- so the shared predicate made those genuinely free agents unreachable through their own free route.
Fix: _is_free_agent reverts to plain, modality-blind price evidence (used wherever a request is already scoped to a specific capability). A new, stricter _is_general_free_agent carries the exclusion, applied only at the blind general-chat call sites (proxy_completion, _orchestrated_provider_completion, route_once, conduct, _ranked_agents when chat_only=True, list_openai_models's advertising check) plus server.py's equivalent capability-agnostic branch of _require_pool_model, which had the identical gap.
🔍 "Serving count uses a different population" -- not a new inconsistency, documented.
free_tier_count was already computed over the complete discovered population regardless of --free-only (an established pre-existing convention, distinct from discovered_count/priced_count/models, which do respect --free-only's row filter). general_free_serving_count deliberately matches that same convention rather than introducing a third population. Added an explicit comment in __main__.py so this doesn't need reverse-engineering next time.
🔍 "Duplicate serving policies can drift" -- fixed by extracting one shared predicate.
chat_capability.requires_non_text_input(input_modalities) is now the single evidence-based classification both model_discovery._requires_non_text_input (over DiscoveredModel.input_modalities) and orchestrator._agent_requires_non_text_input (over an agent's input:<modality> tags) delegate to -- chat_capability.py was already a shared leaf dependency of both files, so no new import cycle. A new cross-consistency test runs the same three modality fixtures through both layers and asserts they agree.
Test evidence (TDD, verified red against 5648567 by temporarily inserting each new test against that commit, then green after this fix)
test_free_virtual_model_selects_a_free_agent_whose_own_capability_needs_non_text_input(orchestrator's_capability_agentspath)test_require_pool_model_serves_capability_free_route_despite_non_text_input(server's_require_pool_modelpath)test_discovery_and_orchestrator_modality_eligibility_cannot_drift(cross-layer consistency)- Updated the two round-1 regression tests to assert
_is_free_agent(agent) is True(capability-reachable) alongside_is_general_free_agent(agent) is False(blind-chat-excluded) for the same agent
Full verification
python -m pytest tests -q-> 2775 passed, 1 skipped (previous round: 2774 passed, 1 skipped -- net +1 test, zero regressions)interrogate-> 100.0%python tests/test_conventions.py-> passesgit diff --check-> clean
Commit: efd44f6, pushed to fix/nim-vision-tool-calling-pool (fast-forward from 5648567).
Generated by Claude Code
|
|
||
| def _is_free_agent(self, agent: ModelAgent) -> bool: | ||
| """Return true only for explicitly zero-priced configured models.""" | ||
| """Return true only for explicitly zero-priced configured models. |
There was a problem hiding this comment.
Confirmed and fixed in efd44f6. _is_free_agent reverts to plain, modality-blind price evidence (used by _capability_agents/_ranked_agents when chat_only=False, and by server.py's _require_pool_model capability-scoped branch). A new, stricter _is_general_free_agent (= _is_free_agent(agent) and not _agent_requires_non_text_input(agent)) now carries the exclusion, applied only at the blind general-chat call sites: proxy_completion, _orchestrated_provider_completion, route_once, conduct, _ranked_agents when chat_only=True, list_openai_models's advertising check, and server.py's capability-agnostic _require_pool_model branch.
Also found and fixed the same gap in server.py's _require_pool_model, which independently re-checks _is_free_agent outside _capability_agents -- same split applied there (capability-scoped branch stays plain, general-chat branch uses _is_general_free_agent).
New coverage: test_free_virtual_model_selects_a_free_agent_whose_own_capability_needs_non_text_input (a free transcription agent with input:audio now resolves through its own /v1/audio/transcriptions-style free route) and test_require_pool_model_serves_capability_free_route_despite_non_text_input (same for _require_pool_model), both fail red against 5648567 (verified by temporarily inserting them against that commit). Also updated the two existing regression tests from round 1 to assert _is_free_agent(agent) is True (capability-reachable) alongside _is_general_free_agent(agent) is False (blind-chat-excluded) for the same vision/transcription-style agent.
Generated by Claude Code
| before treating a discovered model as eligible for blind free serving | ||
| (e.g. tagging an agent ``cost:free`` in a context where that tag alone | ||
| drives general-chat ``orchestrator/free`` routing). | ||
| ``TaskOrchestrator._is_general_free_agent`` additionally re-checks an |
There was a problem hiding this comment.
Both discovered_count and general_free_serving_count/free_tier_count were already answering different questions by design before this PR touched anything: discovered_count (and priced_count, and the models list) reflect reported, which --free-only narrows; free_tier_count has always been computed over the complete discovered population regardless of --free-only (it answers "how many free models exist overall", not "how many of the rows shown below are free"). general_free_serving_count deliberately matches that same established convention rather than reported's row-level filter -- it's a second "how many, out of everything" metric, not a new population.
Documented this explicitly in a comment above both assignments in __main__.py (commit efd44f6) so a future reader doesn't have to reverse-engineer the convention from the pre-existing asymmetry.
Generated by Claude Code
| before treating a discovered model as eligible for blind free serving | ||
| (e.g. tagging an agent ``cost:free`` in a context where that tag alone | ||
| drives general-chat ``orchestrator/free`` routing). | ||
| ``TaskOrchestrator._is_general_free_agent`` additionally re-checks an |
There was a problem hiding this comment.
Agreed, and fixed in efd44f6: extracted the actual "what counts as non-text input" classification into a single shared predicate, chat_capability.requires_non_text_input(input_modalities: Iterable[str]). model_discovery._requires_non_text_input (reading DiscoveredModel.input_modalities) and orchestrator._agent_requires_non_text_input (reading an agent's input:<modality> tags with the prefix stripped) now both delegate to it, so a future modality-rule change only has to happen in one place.
chat_capability.py was already a shared leaf dependency of both model_discovery.py and orchestrator.py (no new import cycle). Only the representation-extraction step (tuple field vs. tag-string parsing) remains separate, since ModelAgent genuinely carries no modality field of its own -- that part can't be unified without adding one.
New cross-consistency test: test_discovery_and_orchestrator_modality_eligibility_cannot_drift runs the same three fixtures (text-only / vision-only / text+image) through both general_free_serving_candidates and _is_general_free_agent and asserts they agree, so a future divergence would fail this test immediately rather than silently drift.
Generated by Claude Code
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Devin's review pass on PR #933 after efd44f6 found two issues: 1. general_free_serving_candidates() admitted evidence-only and non-chat-capable zero-cost text-input catalog rows, overcounting general_free_serving_count with models that could never actually become a serving agent. Now also requires is_routable_discovered_model -- the same predicate _auto_discover_runtime_agents and provider_bootstrap already require before promoting a discovered row to an ordinary chat agent. New regression test test_general_free_serving_candidates_excludes_unroutable_free_models (an evidence-only free text model and a free embedding-only model) fails red pre-fix. 2. A real, deterministic CI failure on efd44f6 itself (GitHub Actions "Full unit and contract suite" job 99313736725): test_discovery_and_orchestrator_modality_eligibility_cannot_drift built ModelAgent fixtures using hyphenated ids straight from provider model ids (e.g. "text-only-model", "meta/llama-3.2-90b-vision-instruct"), which fail this repo's require_object_name two-or-more-word snake_case convention. Fixed by deriving a compliant id (casefold + translate "/.-" to "_") distinct from the `model` field under test. Also documents both this round's fix and the prior capability-route modality-scoping round in CHANGELOG.md (neither had an entry yet). Verified: python -m pytest tests -q -> 2779 passed, 1 skipped, 0 failed (with fast-mlsirm installed via git+https, working around this sandbox's plain-tarball-download 403 that is unrelated to the fix); interrogate -> 100%; tests/test_conventions.py -> ok; git diff --check -> clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX
Round 4 summary (commit 36a0bf3, on top of efd44f6)Two fixes landed in this round, both from Devin's latest review pass: 1. 2. A real, deterministic CI failure on Also backfilled Verification (Python 3.12,
(One local-only wrinkle during verification: this sandbox's network proxy returns HTTP 403 on a plain Pushed fast-forward to Generated by Claude Code |
|
Standing down on further rounds against The safety-critical bug this PR exists to fix — Strix's tool-calling requests hitting NVIDIA NIM's The last four review rounds have each fixed one real, narrowing issue specifically in This latest finding is a pure metric-precision nit on an informational CLI count — it does not affect which agent actually gets selected to serve a chat request (that's Generated by Claude Code Generated by Claude Code |
|
Strix required check failed at head Exit code 124 is the scan hitting its own internal Re-ran the failed job once ( Generated by Claude Code Generated by Claude Code |
|
Re-run failed identically (run 33334177387, job This is a deterministic timeout, not a code defect in this PR's diff: Strix's own internal process budget ( Root cause: the central Generated by Claude Code Generated by Claude Code |
… on large repos The central required Strix Security Scan was deterministically timing out on contextual-orchestrator (a large, actively-growing repo: ~10,000+ statements, thousands of tests). Two consecutive real runs (ContextualWisdomLab/contextual-orchestrator#933, jobs 99317982462 and 99331265840) both failed identically with exit code 124 -- the scanner's own process-timeout wrapper killing an in-progress, zero-finding scan, not a reported vulnerability or a genuine provider outage. This org's standing operating directive (docs/product-goal-directive.md) explicitly accepts that central OpenCode/Strix/Noema scans may take more than two hours per model. The previous budgets (90-minute process / 95-minute total / 100-minute outer deadline / 100-minute step / 120-minute job) did not honor that tolerance for a repo this size. Raise every budget in the same chain, preserving proportional ordering and buffers (process < total < outer-deadline < step-timeout < job-timeout): - process_budget_seconds: 5400 -> 9000 (150 min) - STRIX_TOTAL_TIMEOUT_SECONDS: 5700 -> 9300 (155 min) - strix_gate_deadline outer bound: +6000 -> +9600 (160 min) - "Run Strix (quick)" step timeout-minutes: 100 -> 170 - strix job timeout-minutes: 120 -> 200 200 minutes stays comfortably under GitHub Actions' 360-minute hosted-runner job timeout cap, with ~30 minutes of margin for the job's other steps (checkout, sidecar provisioning, artifact upload). Retry/backoff mechanics (STRIX_GATE_RETRY_BACKOFF_SECONDS, the 3-attempt bounded retry count) are untouched -- this is purely a time-ceiling fix, not a retry-logic change. Updates the matching contract assertions in scripts/ci/test_strix_quick_gate.sh so the pinned numeric strings stay in lockstep with strix.yml. Verified: actionlint on strix.yml (clean), full `coverage run -m pytest tests` (1903 passed, 1 skipped, 21 subtests, no regressions -- the one pre-existing pingora_edge_policy.py coverage gap was already fixed upstream on this branch by the time of push), `interrogate` (100%), `bash -n` on the edited script, and a full real run of `bash scripts/ci/test_strix_quick_gate.sh` (PASS, ~5 min with the fast CI fixture env vars). Refs: ContextualWisdomLab/contextual-orchestrator#933 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KPmJErfkcHer4UVEgrQxUX
…1433) Adds free_family_diversity evidence for Strix free-pool routing decisions, and raises the central Strix Security Scan's time budgets (90/95/100/100/120 min -> 150/155/160/170/200 min) to fix a deterministic timeout reproduced twice on ContextualWisdomLab/contextual-orchestrator#933 (org's product-goal-directive.md already accepts 2+ hour scans per model). Bypass-merged per explicit user authorization: all substantive checks (Strix, Noema, Security Scan, SAST, CodeQL, OSV, SBOM, Scorecard, coverage) are green on the exact current head; only the OpenCode formal-verdict dispatch had not completed, and this fix is foundational/blocking for Strix across every repo in the org.
|
설계 결정: 추상화는 사용하지 않습니다. 와 는 같은 endpoint를 쓰더라도 서로 다른 모델 catalog를 노출할 수 있는 독립 credential/account boundary이므로 각각 discovery·보존·실패 판정을 수행해야 합니다. 모델 간 동적 선택과 latency/quality evidence 공유는 provider 이름 추정이 아니라 오직 명시적 경계에서만 수행하십시오. 이 PR이 해당 전제를 포함하면 제거하거나 현재 head에서 재검증해 주세요. |
|
일반화: 앞 코멘트의 NVIDIA는 예시일 뿐입니다. 모든 KV credential은 독립 provider-account/catalog boundary입니다. 같은 vendor/endpoint/model ID라도 서로 다른 key의 모델·권한·가격·privacy·가용성·실패 상태가 같다고 추정하거나 dedupe하지 마십시오. 개념은 제거하며, 논리적 모델 동등성과 latency/quality evidence 공유는 오직 operator가 명시한 으로만 성립합니다. |
|
Confirmed: For visibility: Generated by Claude Code |
|
Re: the generalization (NVIDIA was just the example, the principle is any two KV credentials must never be assumed/deduped as equivalent) — checked the general dedup path this applies to: The one place that re-collapses that distinction is Generated by Claude Code |
|
최종 일반 계약: 모든 KV credential은 독립 provider-account/catalog boundary입니다. 같은 vendor, endpoint, model ID라도 서로 다른 key의 모델, 권한, 가격, privacy, 가용성, 실패 상태를 같다고 추정하거나 dedupe하지 않습니다. provider family는 없으며, 논리적 모델 동등성과 latency/quality evidence 공유는 오직 operator가 명시한 model_group으로만 성립합니다. |
2 participants
Reproduction evidence
ContextualWisdomLab/.githubPR #1198's required Strix Security Scan check failed at headb051f5da34998fcd0ed42990d9f5e29b128a59ab(run33325907333, job99295892400). Strix callsorchestrator/freethrough the vendored gateway sidecar (scripts/ci/contextual_orchestrator_review_sidecar.shin.github). The job log shows, verbatim, across 3 independent scan attempts (each a fresh sidecar process, ~17:47/17:49/17:52/17:56 UTC):Every one of the 3 attempts hit the exact same model/agent and got the exact same 400.
Root cause
meta/llama-3.2-90b-vision-instructis zero-priced on NVIDIA NIM (Models.dev reportscost: {"input": 0, "output": 0}), and it passes every existing chat-capability check (textoutput modality, no disqualifying model-id token). Sofree_discovered_models()admitted it into the general-purposeorchestrator/freepool that Strix's tool-calling (tools=[...]) requests route through (confirmed:server.py's tool-loop branch callsorchestrator.proxy_completion(..., single_agent=True), which fails over across free candidates on transient/size errors — but this agent alone occupies the top-ranked "free" slot, so nothing was actually protecting the pool from it).I verified live against
models.dev/api.jsonthat itstool_callfield claimstruefor this exact model — so that field cannot be used to gate this; NVIDIA NIM's own/v1/modelslisting carries no capability metadata at all. The model's declared input modality (text+image) is the only honest catalog evidence that distinguishes it from an ordinary text-only free worker, and it's evidence the discovery layer already records.Two fixes considered and rejected
ModelClient's cross-provider failover to retry a plain HTTP 400 on the next free candidate. Explicitly foreclosed by this repo's own tested contracts intests/test_passthrough_provider_failover.py:test_non_transient_error_is_not_replayed("Caller errors fail closed instead of duplicating a request across providers") andtest_virtual_passthrough_keeps_non_size_tool_errors_sticky("A generic provider invalid_tools response must not hide a bad request"). Changing this would weaken a deliberate, tested safety gate._record_failureis never called on the immediately-raised sticky-error branch), but it would not have prevented this incident: each Strix attempt is a fresh sidecar process (in-memory circuit-breaker state), and the observed failure is each attempt's very first request — never enough repetitions within one process to trip a threshold-3 breaker.Fix
free_discovered_models()(contextual_orchestrator/model_discovery.py) now excludes a free model that declares a non-text input modality from the general-purpose free pool. This is a pool-composition fix, not a per-request retry change: the model stays fully discovered and price-evidenced (available to a pool that explicitly wants a vision/multimodal capability); it is only withheld from the capability-blindorchestrator/freedefault that Strix and other tool-calling callers route through.Scoped specifically to the free selector (not general chat-candidate eligibility everywhere), so a genuinely tool-capable paid multimodal model on another provider (e.g. OpenAI) is unaffected — only NVIDIA NIM's free catalog is realistically in scope today (OpenRouter discovery is
evidence_only, Bytez never getsis_free=True)..github's own sidecar/launcher script callscontextual_orchestrator.model_discovery.discover_all_models/free_discovered_modelsdirectly and derives itsorchestrator/freeroute identities from the latter's output — so this fix requires no change to.githuband takes effect automatically the next time its required-workflow pin picks up this repo.Known limitation (not silently declaring victory)
NVIDIA NIM's own model-listing API carries no tool/function-calling capability metadata at all, and Models.dev's
tool_callfield is unreliable at the per-deployment granularity (proven wrong for this exact model). If NIM's remaining free text-only catalog also turns out to lack genuine tool-calling support for some models, Strix could still hit a similar wall — that would need broader free-tier discovery (OpenRouter/Bytez/OpenAI free-tier offerings, all currentlyevidence_onlyor never free in this repo's provider list) as a follow-up. This PR does not fabricate evidence that such a follow-up is unnecessary.TDD / test evidence
tests/test_model_discovery.py::test_free_discovered_models_excludes_a_free_vision_only_input_modelreproduces the bug against aDiscoveredModelfixture shaped like the broken NIM agent (fails pre-fix, passes post-fix); a text-only free model and a free model with no modality evidence at all remain eligible in the same assertion.python -m pytest tests -q→ 2770 passed, 1 skipped (full suite, no regressions).interrogate(this repo'spyproject.tomlsetsfail-under = 100) → 100.0%.python tests/test_self_check.py,test_conventions.py,test_chat_capability.py,test_chat_capability_unknown_identifiers.py,test_provider_bootstrap.py,test_discovery_bootstrap_selection.py,test_chat_model_capability_isolation.py,test_chat_passthrough_capability_isolation.pyall pass individually.python -m pip install --require-hashes -r requirements.lock && python -m pip install --no-deps -e .(Python 3.12, per this repo's own pin) before running the above.Not merging this myself — leaving it for the normal OpenCode review → merge-scheduler pipeline.
Co-Authored-By: Claude Sonnet 5
Generated by Claude Code