Repository navigation
fix(provider): replace the retired NVIDIA Build default - #748
Conversation
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Hi @yashrajp22, thank you for replacing the retired NVIDIA Build default so quickly, and for being clear about what the live run did and did not prove!
Value and readiness: nv_build is the default provider, and its default model z-ai/glm-5.2 now returns HTTP 410. This PR switches the default to a model the endpoint still serves. The change is well contained. default_reasoning_effort="high" applies only when NvBuildProvider builds exactly z-ai/glm-5.3, and a non-blank SKILLSPECTOR_REASONING_EFFORT still wins. Other models, other providers and per-slot overrides are unaffected, and the tests pin each of these cases. The 128k/32k budgets stay well below the advertised window, which follows the registry's existing advice that underestimating is the safe direction. One documentation contradiction must be fixed before merge.
Material findings
- [Blocker]
README.md:691,docs/DEVELOPMENT.md:303,.env.example:30-31: these still say that leavingSKILLSPECTOR_REASONING_EFFORTunset or blank "preserves provider-default behavior" ("uses the provider default" in.env.example). After this PR, the default provider and model sendreasoning_effort: "high". SkillSpector chooses that value; it is not the endpoint's default, which the PR's own comment describes as maximum effort. The new paragraph atREADME.md:302-305says this correctly, so the README now contradicts itself in the table users check for this variable. Please update all three, for example: "unset or blank uses the provider's default; fornv_buildwithz-ai/glm-5.3, SkillSpector sendshigh". - [Non-blocking]
src/skillspector/providers/nv_build/provider.py:41: the removed comment explained that the default is picked for detection, not latency. It recorded the bait skill on which a faster model returned a confident false negative. The one-line replacement loses that rule. Please keep it, and record the evidence for 5.3: the credential-disclosure control produced SSD-3, while the prompt-injection control got no LLM response. The next replacement should be held to the same bar. - [Non-blocking] On the default path, report provenance will record
sampling.reasoning_effortwithsource: "provider_default"andforwarded_to_client: "high", and setdeterminism.control_status: "provider_defaults"(llm_provenance.py:422,:663). That reading holds if "provider" means SkillSpector's adapter. But this is the first time an unset control is forwarded with a value. A test onsanitize_llm_provenancefor this path would make the intended meaning explicit.
PIC tradeoffs:
higheffort versus reliability. In the PR's own live run, later requests got HTTP 504 after about 302 s. The retry used up the 600 s workflow budget, and the prompt-injection control received no LLM response. A gateway limit near 300 s combined with long reasoning athighwould explain this. Merging now replaces a default that fails on every call with one that completes some calls.low, or another served model, might complete more scans but could detect less. Measuring both options on the bait and control skills would settle it. Until then, users should expect some partial LLM scans on the default path.- Interaction with #747. The OpenCode extension's new 630 s limit assumes the CLI finishes within its 600 s budget. That still holds, but slow 504s mean extension LLM scans will often use the full ten minutes.
Verification and gaps:
- Traced
NvBuildProvider.create_chat_model→create_openai_compatible_chat_model.reasoning_efforttakes the user's value when it is non-blank, and the model default otherwise.requested_controlsrecords only the user's value. Forwarded controls are read back from the request payload._REASONING_EFFORTSacceptslow,highandmax. - No other non-historical reference to
glm-5.2as a default remains. Its registry entry stays for explicit pins. - The head moved several times during this review, each time through a bot merge of
main(the latest,a96b7c64, brings in #727, #731, #722 and #593). None of these merges changes this PR's lines, and the authored diff still matchesef6145c9. None of the newly mergedmainchanges touch these files, so the line numbers cited above are still current. - I could not verify the 410, the model card values or the hosted endpoint's limits. Per policy, I did not run the tests or call the endpoint.
- CI: the current head shows
action_requiredwith no jobs run. All six checks passed on the pre-merge headef6145c9.mainhas moved since then, so the current head needs a fresh run. - Overlaps: merges cleanly with #713, #716, #745 and #747. It shares
docs/OPENCODE_EXTENSION.mdwith #747, in a different section. Because this PR restores the default LLM path, it should land first once the doc fix is in.
Decision: Changes Requested (reviewed head a96b7c648291802ee77c575d5062e436db6d929f)
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
|
Thanks for catching the conflicting wording! The README, development guide and environment example now say that an unset or blank value sends high for nv_build with GLM-5.3. I restored the detection-first rationale and added a provenance test for the adapter default. All 211 targeted tests pass, with 9 optional-provider skips. The earlier live timeout remains an open validation gap; this update does not claim full live-provider acceptance. |
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Hi @yashrajp22, thank you for fixing the reasoning-effort wording in all three places and for restoring the detection-first rationale with the new evidence!
Value and readiness: nv_build is the default provider, and its old default model returns HTTP 410. This PR moves the default to z-ai/glm-5.3 and sends high reasoning effort only for that model on that provider. The README, the development guide and .env.example now describe exactly what the code sends, and all three earlier findings are resolved. I found no new code or documentation problem. The branch needs one mechanical rebase: #496 (Gemini provider) landed at 16:27Z and conflicts only in the README provider table. After the rebase and a CI run, it is ready for the PIC, who still has to accept the live-timeout tradeoff below.
Previous findings:
- [Blocker] The environment docs said an unset
SKILLSPECTOR_REASONING_EFFORTkeeps the provider default: Resolved.README.md:692,docs/DEVELOPMENT.md:303and.env.example:30-32now say three things. Non-empty values are trimmed and passed through. When the variable is unset or blank, SkillSpector sendshighfornv_buildwithz-ai/glm-5.3. Other provider/model combinations keep their endpoint defaults.README.md:302-305agrees.- This matches the code.
resolve_reasoning_effortstrips the value and returnsNonewhen it is blank (src/skillspector/providers/chat_models.py:38-41). The factory sendsreasoning_effort or default_reasoning_effort(:139-140). OnlyNvBuildProviderpasses a default, and only for exactlyz-ai/glm-5.3(src/skillspector/providers/nv_build/provider.py:71). - It stays true after the rebase. #496's
GeminiProvidercalls the same factory withoutdefault_reasoning_effort, and no other provider sets a reasoning default. The 128,000/32,000 budgets in the README match the registry entry and the unknown-model fallback (DEFAULT_CONTEXT_LENGTHof 128,000, with 25% for output).
- [Non-blocking] The detection-first rationale was lost: Resolved.
provider.py:41-45restores the rule and records the evidence. GLM-5.3 produced SSD-3 on the credential-disclosure control, the prompt-injection control is still unverified, and both must be rechecked before the next replacement. - [Non-blocking] The provenance meaning of the adapter default was implicit: Resolved.
test_nv_build_adapter_default_is_recorded_without_a_user_override(tests/unit/test_llm_provenance.py:267) pins the unset and blank cases:requested: None,source: "provider_default"(commented as SkillSpector's adapter default),forwarded_to_client: "high",control_status: "provider_defaults"andprovider_guarantee: False. This matches_sanitize_control(src/skillspector/llm_provenance.py:418-441) and the status ladder (:662-663). No document definesprovider_defaultdifferently.
Material findings
None.
PIC tradeoffs:
higheffort versus reliability (unchanged). The author confirms the live gap is still open. Later requests got HTTP 504 after about 302 s, the retry used up the 600 s workflow budget, and the prompt-injection control got no LLM response. Merging replaces a default that fails every call with one that completes some.low, or another served model, might complete more scans but could detect less. Measuring both on the bait and control skills would settle it, and the new code comment asks for that check.- Interaction with #747 (now merged). #747 gives LLM scans in the OpenCode and Pi extensions 630 s, assuming the CLI ends within its 600 s budget. That still holds: each LLM request's timeout is the remaining workflow time, and retries re-check the shared deadline (
llm_analyzer_base.py). With slow 504s, though, default LLM scans through the extensions will often take close to ten minutes and may return partial LLM coverage.
Verification and gaps:
- New commit:
6f25cccchanges only the three documentation entries, the provider comment and the provenance test. The four bot merges after it leave the authored diff unchanged (identical per-file patch-ids). - Tests: they cover unset, blank, trimmed and explicit
low/high/maxvalues,z-ai/glm-5.2,z-ai/glm-5.3-flash, another model, the OpenAI provider with the same model name, a per-slot override and provenance.ruff checkandruff format --checkpass on the changed Python files. I did not run the tests, per policy. - Rebase:
git merge-treeagainst currentmainconflicts only atREADME.md:293, where #496 added ageminirow directly below thenv_buildrow. Keep both rows, withz-ai/glm-5.3in thenv_buildrow..env.example,docs/DEVELOPMENT.mdandtests/unit/test_providers.pymerge automatically. After #496,mainhas no otherglm-5.2default reference, anddocs/OPENCODE_EXTENSION.md(with #747's text) is already in this branch. The rebase is mechanical. - Overlaps: merges cleanly with #737 and #572, which also edit
.env.exampleandREADME.md. - CI: all six checks passed on the author commit
6f25ccc. The head8f8b6daadds only bot merges and showsaction_required. CI must run again after the rebase. - Not verified: the 410, the model card values and the hosted endpoint's limits.
Decision: Approved (reviewed head 8f8b6da0effa26b1699baa4f588e8b53500495e0)
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
|
Thanks for the review! I merged the latest main and resolved the README conflict, keeping both the GLM-5.3 default and the new Gemini entry. The provider, default-model, metadata, provenance, Gemini, and CLI help checks pass: 273 passed and 12 skipped, with lint and formatting checks passing too. The earlier live-provider timeout remains a separate validation gap; this merge does not claim to resolve it. |
NVIDIA Build's default GLM-5.2 endpoint returns HTTP 410, preventing default LLM scans from completing. Select GLM-5.3 and request its supported
highreasoning effort by default, while preserving explicit user overrides and other providers/models. Retain conservative 128k context / 32k output application budgets and update the existing configuration examples.Validation: 218 provider/configuration/provenance tests passed (12 optional-provider skips); Ruff checks passed. The fresh installed wheel completed a benign live scan and all three semantic analyzers completed on the credential-disclosure control, which produced SSD-3. Full live acceptance remains incomplete: subsequent requests received HTTP 504 after about 302 seconds and exhausted the remaining 600-second workflow budget on retry. The prompt-injection control received no successful LLM responses in that run.
NVIDIA documents the model's default maximum reasoning effort and supported values in the model card.