fix(inference): accept catalog-selected vLLM model - #9839
Merged
Conversation
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 WalkthroughWalkthroughFixed-command vLLM profiles now accept ChangesFixed-catalog vLLM behavior
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The change restores catalog-selected vLLM model onboarding while preserving rejection of direct fixed-profile overrides and extra serve arguments. No actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
prekshivyas
marked this pull request as ready for review
August 21, 2026 00:41
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall line coverage in commit 8e17960 in the TypeScript / code-coverage/cliThe overall line coverage in commit 8e17960 in the Show a line coverage summary of the most impacted files.
Updated |
Contributor
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 1 semantic terminology decisionTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite for the commit under review. Recommended E2E: Manual-only E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
senthilr-nv
added
provider: vllm
Collaborator
Author
|
Live Brev validation completed on commit Hardware/topology:
Before fix (
With this PR:
The failed v0.0.112 onboarding left a resumable session and its managed gateway service. The PR validation used the documented |
cv
approved these changes
Aug 21, 2026
cv
left a comment
cv
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed commit 8e17960e77b01e84d26fa1e9225e5f4909c48c9e. The exception is bound to a materialized catalog selection passed through the existing typed resolver; a direct fixed-profile model override remains rejected because it has no hostLocalSelection, and extra serve arguments remain rejected before Docker work. After building generated CLI artifacts, the new catalog install tests and the full vLLM source suite passed 73 tests. The primary advisor reported no findings; the failed second-opinion lane did not identify a code defect.
20 tasks
senthilr-nv
pushed a commit
that referenced
this pull request
Aug 21, 2026
<!-- markdownlint-disable MD041 --> ## Summary Rebuild now verifies the final OpenClaw configuration and hash after all post-restore mutations and Shields relock. A rebuild fails before it reports success when that final pair is invalid. ## Related Issue Fixes #9530 ## Changes E2E root cause: `rebuild-openclaw / final OpenClaw config-hash authority / rebuild exits 0 but the independent final pair is invalid` Source run: https://github.com/NVIDIA/NemoClaw/actions/runs/32393515799 (run 32393515799, attempt 1) Failed job: `Rebuild: preserves OpenClaw state and rotates the gateway token / NVIDIA inference API key` (https://github.com/NVIDIA/NemoClaw/actions/runs/32393515799/job/96508062674) Signature: phase 7 rebuild exited 0; approximately three seconds later, phase 8 reported `hashValid=false` while `tokenPresent`, `tokenRotated`, `runtimeMatchesConfig`, `hashChanged`, and `hashReferencesConfig` were true; cleanup passed. Scope: one root cause - Keep the existing early mutable config-hash refresh after OpenClaw post-restore writes. - Reuse the existing config-hash pair comparison in a read-only final verification command. Rebuild runs it after forwarding and Shields relock, immediately before summary publication. - Report a final post-restore verification failure separately when the early refresh succeeded, so the incomplete report names the operation that failed. - Fail with `OpenClaw config integrity verification failed after rebuild.` when the final pair is invalid. Preserve the incomplete post-restore report and suppress the success summary. - Add deterministic coverage that passes the early refresh, models a later finalization mutation, and requires final verification to fail after Shields relock. - Require the final verifier to reject a missing or non-regular config/hash pair while the earlier refresh retains missing-config tolerance. Direct tests cover missing pair members, a missing config directory, a dangling config symlink, and a config directory without changing `.config-hash`. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Nine-category security review passed for commit under review `0f8869033525371b7dd70733039fbdb333116c46`. The final verifier now rejects absent and non-regular pair members without printing file contents or changing `.config-hash`. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: not applicable - Station profile/scenario: not applicable - Result: not applicable - Supporting evidence: not applicable ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run validate:pr` passed after refreshing `origin/main` when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: - Fail-first commit `73af169ec0b2dc447974debb5bcde3d67fd52b32`: `npm exec -- vitest run --project cli src/lib/actions/sandbox/rebuild-post-restore-phase.test.ts` — 23 passed and the new regression test failed because the phase performed only one config-hash verification. - PR Review Advisor finding PRA-1 fail-first evidence: source blob `b157dca943b90ee60455b3942dd4bfa9911cb9fa` with test blob `3ec97bfdb613247490d2bd889f58245834d5dd0a`; `npm exec -- vitest run --project cli src/lib/actions/sandbox/rebuild-post-restore-phase.test.ts -t "fails when finalization invalidates the OpenClaw config hash after the early refresh"` failed 1/1 because the final-verification message was absent. - PR Review Advisor finding PRA-1 corrected evidence: source blob `d5ce1898766e7d04f75224de419877a6769eeafa` with the same test blob, committed as `09b99c17944076aa6789b64560e789329c1b7d39`; the same focused command passed 1/1. - Commit under review `0f8869033525371b7dd70733039fbdb333116c46` contains command blob `8d7fe890b1b2be8ea1b12dede6eb1fcc797f586a` and test blob `1b24b4ab52496f043b91ce3aaa288d49d7c1c230`. The final verifier rejects a missing directory, missing pair members, a dangling config symlink, and a config directory without changing `.config-hash`. - Retained commit `e9f672cd32e3dc9f95f1527b483aee0e9f514f22` evidence: `npm exec -- vitest run --project cli src/lib/actions/sandbox/rebuild-post-restore-phase.test.ts src/lib/actions/sandbox/rebuild-config-hash.test.ts src/lib/actions/sandbox/rebuild-flow-recovery.test.ts src/lib/actions/sandbox/mcp-bridge-provider.test.ts src/lib/actions/sandbox/rebuild-mcp-phase.test.ts` — 156 passed with nine expected macOS skips. - Retained commit `09b99c17944076aa6789b64560e789329c1b7d39` evidence: `npm exec -- vitest run --project integration test/mcp-add-crash-consistency.test.ts` — 21 passed. - Retained commit `09b99c17944076aa6789b64560e789329c1b7d39` evidence: `npm exec -- vitest run --project package-contract test/package-contract/rebuild-loader-boundary.test.ts` — two passed in an isolated process. - Retained commit `e9f672cd32e3dc9f95f1527b483aee0e9f514f22` evidence: `npm run build:cli`, `npm --prefix nemoclaw run build`, `npm run typecheck:cli`, `npm run source-shape:check`, `npm run checks:repository`, `git diff --check`, and 32 growth-guardrail tests passed. - Retained commit `84a9f6c013e37b6e1b9bfd0be0293f52735568e4`: `npm exec -- vitest run --project cli src/lib/inference/vllm-fixed-catalog-install.test.ts` — 2 passed for exact-base #9839 composition. - Commit under review `0f8869033525371b7dd70733039fbdb333116c46`: `npm exec -- vitest run --project cli src/lib/actions/sandbox/rebuild-config-hash.test.ts src/lib/actions/sandbox/rebuild-post-restore-phase.test.ts` — 24 passed with 14 expected macOS skips. The generated-command boundary separately returned statuses 16, 17, 18, 11, and 17 for the five absent or non-regular states. Existing `.config-hash` bytes were unchanged, and the missing-hash case did not create the file. - Commit under review `0f8869033525371b7dd70733039fbdb333116c46`: `npm exec -- vitest run --project cli src/commands/sandbox/oclif-command-adapters.test.ts --cache=false` — 18 passed in one isolated test file after the unchanged test timed out in the combined process. - Retained commit `84a9f6c013e37b6e1b9bfd0be0293f52735568e4`: `npm exec -- vitest run --changed=0a87614c738eb08c954dee37757c96ad4f7a6b95 --project cli --project plugin --project e2e-support` — 121 passed with nine expected macOS skips. - Commit under review `0f8869033525371b7dd70733039fbdb333116c46`: 32 growth-guardrail tests, `npm run typecheck:cli`, `npm run checks:repository`, `git diff --check`, and `npm run validate:pr` passed. - Retained commit `04dfe8164137350cb5192f97abe42f3782bc12ef` evidence: `npm run build:cli` passed before the PRA-1 follow-up. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: Not run; the five-file change is covered by focused rebuild, MCP composition, compiled-loader, tests selected against base SHA `0a87614c738eb08c954dee37757c96ad4f7a6b95`, and repository validation gates. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- Signed-off-by: Julie Yaunches <jyaunches@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added final verification of OpenClaw configuration integrity after post-restore finalization. - Detects stale or modified configuration hashes without changing stored hash data. - Validates required configuration files and directories and rejects symlinked configuration files. - **Bug Fixes** - Prevents rebuild completion when final configuration integrity checks fail. - Ensures failed verification triggers appropriate relocking and safe bailout behavior. - Provides distinct diagnostics for missing or invalid configuration and hash files. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Julie Yaunches <jyaunches@nvidia.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
3 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
Restore the documented
NEMOCLAW_VLLM_MODEL=muse-glimmer-30bonboarding path. A catalog-validated model selector can now enter its fixed vLLM recipe, while direct fixed-profile model overrides and all extra serve arguments remain rejected.Changes
NEMOCLAW_VLLM_MODELthrough the fixed-profile guard only when the host-local catalog resolver already materialized the selected recipe.NEMOCLAW_VLLM_EXTRA_ARGS_JSONstill fails before Docker work.The product root cause was the recursive install call: after catalog resolution materialized a fixed recipe,
runVllmInstallreread the original selector and treated it as a new, unvalidated override. The detection gap was that model resolution and fixed-profile rejection had separate coverage, but no test carried a supported selector through both stages. The smallest prevention evidence is the paired positive and negative install tests: the catalog-selected model succeeds, while arbitrary serve arguments remain denied without side effects.No documentation changes are needed because the v0.0.112 documentation already describes
muse-glimmer-30bas a supported selector; this restores that behavior.Type of Change
Quality Gates
DGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpm exec -- vitest run --project cli src/lib/inference/vllm-fixed-catalog-install.test.ts(2 passed); fullvllm.test.tssuite (73 passed); related vLLM suites (173 passed)npm run test:changed(32 growth checks and 291 affected tests passed),npm run lint, andnpm run typecheck:clinpm run docsbuilds without warnings (doc changes only)Additional reproduction evidence: on the v0.0.112 tag, the focused positive scenario failed with
this local model profile does not accept NEMOCLAW_VLLM_MODEL or NEMOCLAW_VLLM_EXTRA_ARGS_JSON, while its companion negative case passed. The same positive and negative scenarios pass with this change.Signed-off-by: Prekshi Vyas prekshiv@nvidia.com
Summary by CodeRabbit
Bug Fixes
Tests