Skip to content

fix(provider): replace the retired NVIDIA Build default - #748

Merged
rng1995 merged 21 commits into
mainfrom
yashraj/refresh-nvidia-build-default
Oct 6, 2026
Merged

rng1995 merged 21 commits into
mainfrom
yashraj/refresh-nvidia-build-default

Conversation

@yashrajp22

@yashrajp22 yashrajp22 commented Oct 5, 2026 •

Copy link
Copy Markdown
Collaborator

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 high reasoning 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.

@rng1995 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

  1. [Blocker] README.md:691, docs/DEVELOPMENT.md:303, .env.example:30-31: these still say that leaving SKILLSPECTOR_REASONING_EFFORT unset or blank "preserves provider-default behavior" ("uses the provider default" in .env.example). After this PR, the default provider and model send reasoning_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 at README.md:302-305 says 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; for nv_build with z-ai/glm-5.3, SkillSpector sends high".
  2. [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.
  3. [Non-blocking] On the default path, report provenance will record sampling.reasoning_effort with source: "provider_default" and forwarded_to_client: "high", and set determinism.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 on sanitize_llm_provenance for this path would make the intended meaning explicit.

PIC tradeoffs:

  • high effort 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 at high would 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_effort takes the user's value when it is non-blank, and the model default otherwise. requested_controls records only the user's value. Forwarded controls are read back from the request payload. _REASONING_EFFORTS accepts low, high and max.
  • No other non-historical reference to glm-5.2 as 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 matches ef6145c9. None of the newly merged main changes 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_required with no jobs run. All six checks passed on the pre-merge head ef6145c9. main has 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.md with #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)

@yashrajp22

Copy link
Copy Markdown
Collaborator Author

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 rng1995 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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_EFFORT keeps the provider default: Resolved.
    • README.md:692, docs/DEVELOPMENT.md:303 and .env.example:30-32 now say three things. Non-empty values are trimmed and passed through. When the variable is unset or blank, SkillSpector sends high for nv_build with z-ai/glm-5.3. Other provider/model combinations keep their endpoint defaults. README.md:302-305 agrees.
    • This matches the code. resolve_reasoning_effort strips the value and returns None when it is blank (src/skillspector/providers/chat_models.py:38-41). The factory sends reasoning_effort or default_reasoning_effort (:139-140). Only NvBuildProvider passes a default, and only for exactly z-ai/glm-5.3 (src/skillspector/providers/nv_build/provider.py:71).
    • It stays true after the rebase. #496's GeminiProvider calls the same factory without default_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_LENGTH of 128,000, with 25% for output).
  • [Non-blocking] The detection-first rationale was lost: Resolved. provider.py:41-45 restores 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" and provider_guarantee: False. This matches _sanitize_control (src/skillspector/llm_provenance.py:418-441) and the status ladder (:662-663). No document defines provider_default differently.

Material findings
None.

PIC tradeoffs:

  • high effort 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: 6f25ccc changes 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/max values, 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 check and ruff format --check pass on the changed Python files. I did not run the tests, per policy.
  • Rebase: git merge-tree against current main conflicts only at README.md:293, where #496 added a gemini row directly below the nv_build row. Keep both rows, with z-ai/glm-5.3 in the nv_build row. .env.example, docs/DEVELOPMENT.md and tests/unit/test_providers.py merge automatically. After #496, main has no other glm-5.2 default reference, and docs/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.example and README.md.
  • CI: all six checks passed on the author commit 6f25ccc. The head 8f8b6da adds only bot merges and shows action_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>
@yashrajp22

Copy link
Copy Markdown
Collaborator Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants