feat(inference): persist serving profile lifecycle provenance - #8401
feat(inference): persist serving profile lifecycle provenance#8401prekshivyas wants to merge 16 commits into
Conversation
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds discoverable serving profiles, generic onboarding selection, feature-gated DGX Spark local-model runtimes, provenance persistence, secure runtime recovery, ownership-aware cleanup, and related documentation. ChangesManaged serving profiles
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
|
🌿 Preview your docs: https://nvidia-preview-pr-8401.docs.buildwithfern.com/nemoclaw |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit b235cfc in the TypeScript / code-coverage/cliThe overall coverage in commit b235cfc in the Show a code coverage summary of the most impacted files.
Updated |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
1 additional E2E selection from the second opinionAdvisory only. The primary lane did not select these E2E jobs or targets.
Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. 4 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: 3 optional E2E recommendations
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commands/profiles/list.ts`:
- Around line 10-29: Update ProfilesListCommand to record maintainer approval
for the serving-profile product surface, including ownership, compatibility,
security, lifecycle, and validation expectations associated with issue `#8384`. If
those requirements cannot be established in the command metadata or surrounding
implementation, route the surface through Community Solutions instead of leaving
it undocumented.
In `@src/lib/inference/serving/profile-list.test.ts`:
- Around line 11-12: Update the test around listServingProfiles to mock or
inject deterministic implementations for os.hostname(),
createHostReadinessReport(), and resolveManagedInferenceServing(), preventing CI
host state from affecting results. Add assertions covering both compatible and
incompatible profile entries while preserving the existing catalog-loading
setup.
In `@src/lib/onboard/command.ts`:
- Around line 180-227: Move selectedServingPreset and
validateServingProfileCompatibility out of the onboarding command into the
serving feature module, alongside catalog discovery and compatibility
evaluation. Expose a serving-module API that resolves the requested ID/display
name, handles unknown, ambiguous, and disabled profiles, and returns or rejects
incompatible profiles. Update the onboarding command to use that API while
retaining only input handling and validateServingProfileConflicts
environment-policy checks.
- Around line 365-378: The environment handling in runOnboardCommand and
applyServingProfileEnvironment must use one consistent environment object.
Select the caller-provided deps.env when available, otherwise process.env, then
pass that object through option validation, applyPortableEnvironment,
applyServingProfileEnvironment, and restoration so validation and onboarding
observe the same variables.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 542a9fb7-6a34-4c86-8a5e-e19709014bd3
📒 Files selected for processing (14)
ci/source-architecture-budget.jsondocs/inference/set-up-vllm.mdxdocs/reference/commands.mdxmanaged-inference/presets/llama-cpp.dgx-spark-gb10.single.nemotron-3-nano-30b-a3b.yamlmanaged-inference/presets/vllm.dgx-spark-gb10.dual.deepseek-v4-flash-0731.yamlmanaged-inference/schemas/preset.schema.jsonsrc/commands/profiles/list.tssrc/lib/inference/serving/profile-list.test.tssrc/lib/inference/serving/profile-list.tssrc/lib/inference/serving/types.tssrc/lib/onboard/command-support.tssrc/lib/onboard/command.test.tssrc/lib/onboard/command.tstest/package-contract/cli/command-registry.test.ts
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/onboard/agents-manifest.ts`:
- Around line 182-187: The applyAgentsManifestEnv flow must preserve the
caller’s original NEMOCLAW_EXTRA_AGENTS_JSON value. In runOnboardCommand,
capture that value before invoking applyAgentsManifestEnv, then restore it in
the existing finally cleanup, deleting the property when it was previously
unset.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 907fd589-0441-4a54-a4ed-f84e1afe109c
📒 Files selected for processing (5)
src/lib/inference/serving/profile-list.test.tssrc/lib/inference/serving/profile-list.tssrc/lib/onboard/agents-manifest.tssrc/lib/onboard/command.test.tssrc/lib/onboard/command.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/inference/serving/profile-list.test.ts
- src/lib/onboard/command.ts
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/onboard/command.ts`:
- Around line 365-367: Move the applyAgentsManifestEnv setup for
options.agentsManifest inside the existing try block so manifest-load failures
still execute finally; initialize experimentalProfile and servingProfile
restoration callbacks to no-ops before entering try, preserving cleanup of any
previously applied values in deps.env. Add a regression test covering an
unavailable or invalid manifest and verifying environment restoration.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6680dd1e-d651-47f0-9848-fd2656fd5918
📒 Files selected for processing (2)
src/lib/onboard/command.test.tssrc/lib/onboard/command.ts
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
…m/NVIDIA/NemoClaw into agent/8384-serving-profile-ux # Conflicts: # src/lib/actions/onboard.ts
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
prekshivyas
changed the title
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (12)
src/lib/inference/serving/adapter-registry.ts (1)
374-381: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSeparate the invalid-name message from the owned-name message.
The condition on lines 375-378 fails for two distinct reasons: an environment name that does not match
SAFE_ENVIRONMENT_NAME, and a name that collides withHOST_LOCAL_MATERIALIZER_OWNED_ENVIRONMENT. Both return "host-local vLLM recipe overrides a materializer-owned environment value". A recipe author who declareshf_homein lowercase receives an override message that does not describe the actual defect.♻️ Proposed split of the two failure reasons
- if ( - Object.keys(recipe.spec.runtime.environment).some( - (name) => - !SAFE_ENVIRONMENT_NAME.test(name) || HOST_LOCAL_MATERIALIZER_OWNED_ENVIRONMENT.has(name), - ) - ) { + if (Object.keys(runtime.environment).some((name) => !SAFE_ENVIRONMENT_NAME.test(name))) { + return "host-local vLLM environment names must match the registered format"; + } + if ( + Object.keys(runtime.environment).some((name) => + HOST_LOCAL_MATERIALIZER_OWNED_ENVIRONMENT.has(name), + ) + ) { return "host-local vLLM recipe overrides a materializer-owned environment value"; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/inference/serving/adapter-registry.ts` around lines 374 - 381, Update the validation around the recipe environment names to distinguish invalid names from materializer-owned names: return an invalid-environment-name message when a name fails SAFE_ENVIRONMENT_NAME, and retain the override message only for names in HOST_LOCAL_MATERIALIZER_OWNED_ENVIRONMENT. Preserve the existing validation behavior for valid, non-owned names.src/lib/inference/serving/resolver.test.ts (1)
99-113: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
satisfiesfor the preset fixture and record why non-readiness requirements are dropped.Line 112 casts the preset with
as ManagedInferenceServingPreset, while line 98 usessatisfiesfor the recipe. The cast suppresses type checking, so a future shape change inManagedInferenceServingPresetwill not fail this fixture at compile time.Lines 106-108 keep only requirements that contain a
readinesskey. Any non-readiness requirement in the shipped preset, such as a feature gate, disappears from the fixture. The explicit-selection tests at lines 283 and 304 then exercise a preset that is easier to satisfy than the shipped one. Add a short comment stating that the filter is intentional and that requirement-evaluation coverage lives in the requirements-not-met tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/inference/serving/resolver.test.ts` around lines 99 - 113, The preset fixture should use a type-checking `satisfies ManagedInferenceServingPreset` expression instead of an `as` cast, preserving compile-time validation like the recipe fixture. In the fixture-building code around `sourcePreset`, add a brief comment explaining that filtering to readiness requirements is intentional and that requirement-evaluation coverage is provided by the requirements-not-met tests.src/lib/onboard/command.test.ts (1)
124-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the
as neversession doubles with a typed session shape.Lines 125, 140, 150, and 158 build the
loadSessiondouble withas never.neveris assignable to every type, so the compiler accepts any return shape. IfResolveOnboardOptionsDeps["loadSession"]changes, these tests keep compiling while asserting against a shape that production no longer produces.
loadSessionis declared insrc/lib/onboard/command.tsas() => { servingProfileProvenance?: ServingProfileProvenance | null } | null. The literals already match that type, so the cast can be removed. Usesatisfiesif the surrounding call site needs the wider session type.♻️ Proposed removal of the type-check bypass
- loadSession: () => ({ servingProfileProvenance: recorded }) as never, + loadSession: () => ({ servingProfileProvenance: recorded }),- resolve({ resume: true }, { loadSession: () => ({}) as never }).servingProfile, + resolve({ resume: true }, { loadSession: () => ({}) }).servingProfile,Also applies to: 148-158
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/onboard/command.test.ts` around lines 124 - 127, Replace the `as never` casts on the `loadSession` doubles in the affected tests with the declared session shape `{ servingProfileProvenance?: ServingProfileProvenance | null } | null`. Update the `loadSession` fixtures around the visible dependency setup, using `satisfies` only if needed for contextual typing, while preserving their existing values and behavior.Source: Coding guidelines
src/lib/inference/serving/profile-provenance.test.ts (1)
19-26: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert every provenance field named by this test.
The assertion does not check
model,runtimeImage,estimatedImageDownloadBytes, orestimatedModelDownloadBytes. A valid but incorrect value for any of these fields passes both parsing and current-catalog validation because both operations consume the same generated object.Compare these fields with the resolved recipe and runtime metadata from
catalog.As per path instructions, “Review tests for behavioral confidence rather than implementation lock-in.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/inference/serving/profile-provenance.test.ts` around lines 19 - 26, The provenance test currently validates only a subset of fields, allowing incorrect generated values to pass. Extend the assertions around parseServingProfileProvenance and assertServingProfileProvenanceCurrent to compare model, runtimeImage, estimatedImageDownloadBytes, and estimatedModelDownloadBytes against the resolved recipe and runtime metadata from catalog, while preserving the existing behavioral assertions.Source: Path instructions
src/lib/inference/vllm-models.ts (1)
512-513: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare one port constant between the serve command and recovery.
This list hardcodes
--port 8000.src/lib/inference/serving/vllm-host-local-lifecycle.tsline 25 declaresHOST_LOCAL_VLLM_PORT = 8000independently, and line 312 of that file rejects any container whose published port differs. The two literals must stay equal, otherwise recovery silently stops matching the managed container. Export one constant and use it in both places.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/inference/vllm-models.ts` around lines 512 - 513, Replace the hardcoded 8000 in the vLLM serve command’s port arguments with an exported shared port constant, then import and reuse that constant in the local lifecycle module’s HOST_LOCAL_VLLM_PORT definition. Ensure both serving and recovery reference the same value so published-port validation remains consistent.src/lib/inference/llama-cpp/managed-installer.ts (1)
582-582: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExplain the discarded receipt read.
The return value of
readPrivateRegularFileis unused here. The call exists to fail closed when an existingruntime.jsonis not an owner-only regular file. Add a short comment so a later refactor does not delete the call as dead code.♻️ Proposed comment
+ // Fail closed when a prior receipt exists with unsafe ownership or permissions. readPrivateRegularFile(path.join(privateStateDir, MANAGED_LLAMA_CPP_RUNTIME_RECEIPT_FILE));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/inference/llama-cpp/managed-installer.ts` at line 582, Add a short explanatory comment immediately before the readPrivateRegularFile call in the managed installer, stating that the read intentionally validates the existing runtime receipt as an owner-only regular file and fails closed when validation fails. Keep the call and its discarded return value unchanged.src/lib/inference/llama-cpp/managed-installer.test.ts (1)
118-123: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant
vi.unstubAllEnvs()call.The
cliVitest project already enablesunstubEnvs, so environment stubs are restored automatically. Keep only the temporary-directory cleanup, which Vitest does not manage.♻️ Proposed cleanup
afterEach(() => { - vi.unstubAllEnvs(); for (const directory of temporaryDirectories.splice(0)) { fs.rmSync(directory, { force: true, recursive: true }); } });Based on learnings: Vitest files under
srcrun in thecliproject, which enablesclearMocks,restoreMocks,unstubEnvs, andunstubGlobals; teardown should only clean resources Vitest does not manage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/inference/llama-cpp/managed-installer.test.ts` around lines 118 - 123, Remove the redundant vi.unstubAllEnvs() call from the afterEach teardown and retain the temporaryDirectories cleanup loop, since Vitest’s cli project already restores environment stubs automatically.Source: Learnings
src/lib/inference/vllm.test.ts (1)
209-213: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe capture stub now depends on another mock's call history.
The
dockerCaptureimplementation branches onmocks.dockerRunDetached.mock.calls.length > 0. Two effects follow. First, the stub couples to an unrelated mock rather than to the inspected command. Second, everycontainercapture after launch bypassesownershipHandlers, so the "Unexpected extra ambient vLLM ownership inspection" guard at line 201 can no longer fire once a container has been launched. That reduces the protection the guard previously gave.Consider driving the running state from an explicit flag that the harness sets, and keep the ownership queue authoritative for pre-launch inspections.
As per path instructions: "Flag copied production algorithms, broad mocks that bypass the behavior under test, and conditionals that make a test pass without exercising its claim."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/inference/vllm.test.ts` around lines 209 - 213, The dockerCapture stub should not infer container state from dockerRunDetached mock history or bypass ownership inspection after launch. Add an explicit harness state flag that the launch setup updates, use it only to report the expected running container state, and keep ownershipHandlers authoritative for all pre-launch container captures so the unexpected-inspection guard remains effective.Source: Path instructions
src/lib/inference/serving/vllm-managed-support.ts (1)
17-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse one import path for each re-exported host-local vLLM function.
Import
recoverHostLocalManagedVllmEndpointfromvllm-managed-support.tsinlocal.ts. Keep direct lifecycle imports only for symbols that the barrel does not export. This keeps module mocks aligned with production imports.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/inference/serving/vllm-managed-support.ts` around lines 17 - 21, Update local.ts to import recoverHostLocalManagedVllmEndpoint from the vllm-managed-support barrel instead of importing it directly from vllm-host-local-lifecycle.js. Retain direct lifecycle imports only for functions not re-exported by the barrel, ensuring each re-exported host-local vLLM function uses a single import path.src/lib/actions/uninstall/all-gateway-ports.test.ts (1)
216-220: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the mock payload with the real result shape.
runUninstallPlanreads onlyresult.statusfromrunLocalModelRuntimeCleanup, so thisstdoutis unused. The payload also usesskipped, whilecleanupLocalModelRuntimesreturnsremovedandpreserved. A future reader can take this as the contract. Drop thestdoutpayload or use the real field names.♻️ Proposed simplification
- runLocalModelRuntimeCleanup: () => ({ - status: 0, - stdout: JSON.stringify({ ok: true, removed: [], skipped: [] }), - stderr: "", - }), + runLocalModelRuntimeCleanup: () => ({ status: 0, stdout: "", stderr: "" }),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/actions/uninstall/all-gateway-ports.test.ts` around lines 216 - 220, Update the runLocalModelRuntimeCleanup mock used by runUninstallPlan to match the real result contract: remove the unused stdout payload and replace the skipped field with preserved if the payload remains. Keep status: 0 and the existing cleanup behavior unchanged.src/lib/inference/local-model-profile/cleanup.test.ts (1)
142-175: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the negative case this title claims.
The title says removal happens "only when key and fingerprint match", but the test asserts the matching case alone. The mismatch branch at
cleanup.tsLines 387-394 is not covered by any test in this file. Add a case where the container label fingerprint or the containerVLLM_API_KEYvalue differs from the persisted key, then assertok: falseand thatforceRmis not called.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/inference/local-model-profile/cleanup.test.ts` around lines 142 - 175, Add a negative test alongside the matching case for cleanupLocalModelRuntimes, using a managed host-local vLLM container whose authentication fingerprint or VLLM_API_KEY differs from the persisted key. Assert the result has ok: false and verify the forceRm mock is not called, while preserving the existing positive-case coverage.src/lib/actions/uninstall/run-plan.ts (1)
1440-1444: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare the managed llama.cpp state directory name.
This code hardcodes
"managed-llama-cpp".src/lib/inference/local-model-profile/cleanup.tsLine 476 builds the same path from the same literal. If one side changes, uninstall silently stops invoking cleanup while the cleanup module still expects state. Export the directory name next to the otherMANAGED_LLAMA_CPP_*constants and use it in both places.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/actions/uninstall/run-plan.ts` around lines 1440 - 1444, Export a shared managed llama.cpp state-directory constant alongside the existing MANAGED_LLAMA_CPP_* constants, then replace the "managed-llama-cpp" literal in run-plan’s hasLlamaState path and cleanup.ts’s corresponding path construction with that constant. Update imports as needed while preserving existing cleanup behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/install.sh`:
- Around line 5033-5045: Update the local-model setup around the
NEMOCLAW_ENABLE_LOCAL_MODEL_PROFILE case so invalid runtimes on non-DGX Spark
hosts are rejected before NEMOCLAW_NO_EXPRESS is exported, or defer that export
until after the Station resume boundary. Preserve the existing valid vllm and
llama-cpp behavior while ensuring maybe_offer_express_install() does not clear
resume state for rejected selections.
In `@src/lib/inference/local-model-profile/cleanup.ts`:
- Around line 491-504: Set timeout: 10_000 on the Docker invocations in the
cleanup flow, including deps.run(["info"], ...) within the main try block and
the removal calls in cleanupHostLocalVllm and cleanupLlamaCpp. Preserve the
existing options and failure-handling behavior so timeout results are processed
normally.
In `@src/lib/inference/local.ts`:
- Around line 548-552: Update the hostLocalEndpoint selection to use the
injected recoverHostLocalManagedVllmEndpointImpl when provided, otherwise fall
back directly to recoverHostLocalManagedVllmEndpoint, without checking
getManagedBaseUrlImpl. Preserve injection of () => null as the mechanism for
tests that need to suppress recovery.
In `@src/lib/inference/serving/adapter-registry.ts`:
- Around line 99-114: Update the argument ownership definitions used by
buildVllmServeCommand and modelArguments to include "--model" in both host-local
materializer-owned sets. This must reject recipe-defined --model arguments and
prevent forwarding them while preserving the positional model.id emitted by
buildVllmServeCommand.
In `@src/lib/inference/serving/resolver.ts`:
- Around line 155-168: Update versionAtLeast to compare version components
exactly without converting them to Number; use BigInt or normalized digit-string
comparison after validation so values beyond Number.MAX_SAFE_INTEGER remain
distinguishable. Preserve missing components as zero and the existing
lexicographic component ordering, and add a boundary test covering adjacent
oversized components to ensure readiness qualification fails when actual is
smaller.
In `@src/lib/inference/vllm.ts`:
- Around line 2027-2046: Update the failed auth-boundary branch in the
managed-auth flow of the vLLM install logic to force-remove the container with
dockerForceRm using the start container identifier, then delete
HOST_LOCAL_VLLM_RUNTIME_RECEIPT_FILE before returning failure. Replace the
existing dockerStop cleanup so no stale container or receipt remains.
In `@src/lib/onboard/command.ts`:
- Around line 257-262: Update the comparison in the requested/current provenance
validation block to use node:util’s isDeepStrictEqual instead of comparing
JSON.stringify results. Preserve the existing mismatch failure behavior and
message, and reuse the existing utility import pattern used by
profile-provenance.ts.
In `@test/install-local-model-profile.test.ts`:
- Around line 22-31: Update the environment setup in the test harness around
INSTALLER_UNDER_TEST to clear every NEMOCLAW profile-selection variable read by
the installer, including serving preset, vLLM model and extra arguments, and
managed-cluster peer settings; then overlay only the scenario-specific values
from env so parent-process values cannot affect the branch under test.
---
Nitpick comments:
In `@src/lib/actions/uninstall/all-gateway-ports.test.ts`:
- Around line 216-220: Update the runLocalModelRuntimeCleanup mock used by
runUninstallPlan to match the real result contract: remove the unused stdout
payload and replace the skipped field with preserved if the payload remains.
Keep status: 0 and the existing cleanup behavior unchanged.
In `@src/lib/actions/uninstall/run-plan.ts`:
- Around line 1440-1444: Export a shared managed llama.cpp state-directory
constant alongside the existing MANAGED_LLAMA_CPP_* constants, then replace the
"managed-llama-cpp" literal in run-plan’s hasLlamaState path and cleanup.ts’s
corresponding path construction with that constant. Update imports as needed
while preserving existing cleanup behavior.
In `@src/lib/inference/llama-cpp/managed-installer.test.ts`:
- Around line 118-123: Remove the redundant vi.unstubAllEnvs() call from the
afterEach teardown and retain the temporaryDirectories cleanup loop, since
Vitest’s cli project already restores environment stubs automatically.
In `@src/lib/inference/llama-cpp/managed-installer.ts`:
- Line 582: Add a short explanatory comment immediately before the
readPrivateRegularFile call in the managed installer, stating that the read
intentionally validates the existing runtime receipt as an owner-only regular
file and fails closed when validation fails. Keep the call and its discarded
return value unchanged.
In `@src/lib/inference/local-model-profile/cleanup.test.ts`:
- Around line 142-175: Add a negative test alongside the matching case for
cleanupLocalModelRuntimes, using a managed host-local vLLM container whose
authentication fingerprint or VLLM_API_KEY differs from the persisted key.
Assert the result has ok: false and verify the forceRm mock is not called, while
preserving the existing positive-case coverage.
In `@src/lib/inference/serving/adapter-registry.ts`:
- Around line 374-381: Update the validation around the recipe environment names
to distinguish invalid names from materializer-owned names: return an
invalid-environment-name message when a name fails SAFE_ENVIRONMENT_NAME, and
retain the override message only for names in
HOST_LOCAL_MATERIALIZER_OWNED_ENVIRONMENT. Preserve the existing validation
behavior for valid, non-owned names.
In `@src/lib/inference/serving/profile-provenance.test.ts`:
- Around line 19-26: The provenance test currently validates only a subset of
fields, allowing incorrect generated values to pass. Extend the assertions
around parseServingProfileProvenance and assertServingProfileProvenanceCurrent
to compare model, runtimeImage, estimatedImageDownloadBytes, and
estimatedModelDownloadBytes against the resolved recipe and runtime metadata
from catalog, while preserving the existing behavioral assertions.
In `@src/lib/inference/serving/resolver.test.ts`:
- Around line 99-113: The preset fixture should use a type-checking `satisfies
ManagedInferenceServingPreset` expression instead of an `as` cast, preserving
compile-time validation like the recipe fixture. In the fixture-building code
around `sourcePreset`, add a brief comment explaining that filtering to
readiness requirements is intentional and that requirement-evaluation coverage
is provided by the requirements-not-met tests.
In `@src/lib/inference/serving/vllm-managed-support.ts`:
- Around line 17-21: Update local.ts to import
recoverHostLocalManagedVllmEndpoint from the vllm-managed-support barrel instead
of importing it directly from vllm-host-local-lifecycle.js. Retain direct
lifecycle imports only for functions not re-exported by the barrel, ensuring
each re-exported host-local vLLM function uses a single import path.
In `@src/lib/inference/vllm-models.ts`:
- Around line 512-513: Replace the hardcoded 8000 in the vLLM serve command’s
port arguments with an exported shared port constant, then import and reuse that
constant in the local lifecycle module’s HOST_LOCAL_VLLM_PORT definition. Ensure
both serving and recovery reference the same value so published-port validation
remains consistent.
In `@src/lib/inference/vllm.test.ts`:
- Around line 209-213: The dockerCapture stub should not infer container state
from dockerRunDetached mock history or bypass ownership inspection after launch.
Add an explicit harness state flag that the launch setup updates, use it only to
report the expected running container state, and keep ownershipHandlers
authoritative for all pre-launch container captures so the unexpected-inspection
guard remains effective.
In `@src/lib/onboard/command.test.ts`:
- Around line 124-127: Replace the `as never` casts on the `loadSession` doubles
in the affected tests with the declared session shape `{
servingProfileProvenance?: ServingProfileProvenance | null } | null`. Update the
`loadSession` fixtures around the visible dependency setup, using `satisfies`
only if needed for contextual typing, while preserving their existing values and
behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 61a38d80-fe57-4e35-918a-14af1933372b
📒 Files selected for processing (74)
ci/test-file-size-budget.jsondocs/inference/choose-local-inference-server.mdxdocs/inference/set-up-vllm.mdxdocs/reference/commands.mdxdocs/reference/host-files-and-state.mdxmanaged-inference/presets/local-model-profile.llama-cpp.spark.v1.yamlmanaged-inference/presets/local-model-profile.vllm.spark.v1.yamlmanaged-inference/recipes/vllm.qwen3-6-35b-a3b-nvfp4.spark-single.v1.yamlmanaged-inference/schemas/preset.schema.jsonscripts/install.shsrc/lib/actions/onboard.tssrc/lib/actions/sandbox/status-snapshot.tssrc/lib/actions/sandbox/status-text.tssrc/lib/actions/sandbox/status.test.tssrc/lib/actions/uninstall/all-gateway-ports.test.tssrc/lib/actions/uninstall/run-plan-local-model-profile.test.tssrc/lib/actions/uninstall/run-plan.tssrc/lib/adapters/docker/local-model-runtime.tssrc/lib/inference/llama-cpp/host-local-runtime.tssrc/lib/inference/llama-cpp/managed-installer.test.tssrc/lib/inference/llama-cpp/managed-installer.tssrc/lib/inference/local-model-profile/cleanup-entry.tssrc/lib/inference/local-model-profile/cleanup-path-safety.test.tssrc/lib/inference/local-model-profile/cleanup.test.tssrc/lib/inference/local-model-profile/cleanup.tssrc/lib/inference/local.tssrc/lib/inference/serving/adapter-registry.tssrc/lib/inference/serving/catalog-loader.test.tssrc/lib/inference/serving/catalog.test.tssrc/lib/inference/serving/catalog.tssrc/lib/inference/serving/host-local-vllm-selection.tssrc/lib/inference/serving/profile-list.test.tssrc/lib/inference/serving/profile-list.tssrc/lib/inference/serving/profile-provenance.test.tssrc/lib/inference/serving/profile-provenance.tssrc/lib/inference/serving/resolver.test.tssrc/lib/inference/serving/resolver.tssrc/lib/inference/serving/runtime-auth-fingerprint.tssrc/lib/inference/serving/types.tssrc/lib/inference/serving/vllm-host-local-lifecycle.test.tssrc/lib/inference/serving/vllm-host-local-lifecycle.tssrc/lib/inference/serving/vllm-managed-support.tssrc/lib/inference/vllm-models.tssrc/lib/inference/vllm.test.tssrc/lib/inference/vllm.tssrc/lib/onboard.tssrc/lib/onboard/command-agents.test.tssrc/lib/onboard/command-support.tssrc/lib/onboard/command.test.tssrc/lib/onboard/command.tssrc/lib/onboard/local-model-profile/integration.tssrc/lib/onboard/local-model-profile/onboarder.test.tssrc/lib/onboard/local-model-profile/onboarder.tssrc/lib/onboard/local-model-profile/plan.test.tssrc/lib/onboard/local-model-profile/plan.tssrc/lib/onboard/machine/handlers/provider-inference.tssrc/lib/onboard/sandbox-registration.test.tssrc/lib/onboard/sandbox-registration.tssrc/lib/onboard/session-bootstrap.test.tssrc/lib/onboard/session-bootstrap.tssrc/lib/onboard/session-updates.tssrc/lib/onboard/setup-nim-flow.test.tssrc/lib/onboard/setup-nim-flow.tssrc/lib/onboard/summary.test.tssrc/lib/onboard/summary.tssrc/lib/onboard/types.tssrc/lib/state/onboard-session.test.tssrc/lib/state/onboard-session.tssrc/lib/state/registry-normalization.test.tssrc/lib/state/registry.tssrc/lib/state/registry/persistence.tssrc/lib/state/registry/types.tstest/install-local-model-profile.test.tstest/onboard-selection.test.ts
💤 Files with no reviewable changes (1)
- test/onboard-selection.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/onboard/command-agents.test.ts
- src/lib/onboard/command-support.ts
- src/lib/inference/serving/profile-list.ts
| if (requested && JSON.stringify(requested) !== JSON.stringify(current)) { | ||
| fail( | ||
| deps, | ||
| ` --profile ${requested.preset.id} does not match resumed profile ${current.preset.id}.`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Compare provenance records structurally, not by JSON.stringify.
Line 257 compares requested and current by serialized string. The two records reach this point by different routes. requested is built directly by servingProfileProvenance, so its key order follows the object literal in src/lib/inference/serving/profile-provenance.ts. current is the value returned by assertServingProfileProvenanceCurrent, which returns structuredClone(recorded) and therefore preserves the key order of the persisted session JSON.
Today both orders agree. If any future writer, normalizer, or persistence path reorders keys, the strings differ while the objects stay deeply equal. The user is then blocked from resuming with the same --profile they originally selected, and the error text names the same preset ID on both sides, which gives no way to diagnose the failure.
node:util already provides isDeepStrictEqual, and profile-provenance.ts uses it for the same comparison.
♻️ Proposed structural comparison
+import { isDeepStrictEqual } from "node:util";- if (requested && JSON.stringify(requested) !== JSON.stringify(current)) {
+ if (requested && !isDeepStrictEqual(requested, current)) {
fail(
deps,
` --profile ${requested.preset.id} does not match resumed profile ${current.preset.id}.`,
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (requested && JSON.stringify(requested) !== JSON.stringify(current)) { | |
| fail( | |
| deps, | |
| ` --profile ${requested.preset.id} does not match resumed profile ${current.preset.id}.`, | |
| ); | |
| } | |
| if (requested && !isDeepStrictEqual(requested, current)) { | |
| fail( | |
| deps, | |
| ` --profile ${requested.preset.id} does not match resumed profile ${current.preset.id}.`, | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/onboard/command.ts` around lines 257 - 262, Update the comparison in
the requested/current provenance validation block to use node:util’s
isDeepStrictEqual instead of comparing JSON.stringify results. Preserve the
existing mismatch failure behavior and message, and reuse the existing utility
import pattern used by profile-provenance.ts.
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
🛑 Comments failed to post (7)
scripts/install.sh (1)
5033-5045: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Confirm the Station resume clearing path reachable from NEMOCLAW_NO_EXPRESS=1. rg -n -C 12 'clear_station_express_resume\(\)' scripts/install.sh rg -n -C 6 'NEMOCLAW_NO_EXPRESS' scripts/install.sh # Confirm the call ordering between the exports in main and maybe_offer_express_install. rg -n 'prepare_installer_host|maybe_offer_express_install|NEMOCLAW_ENABLE_LOCAL_MODEL_PROFILE' scripts/install.sh # Confirm the local model profile requires the DGX Spark qualification. rg -n -C 4 'dgx_spark' managed-inference/presetsRepository: NVIDIA/NemoClaw
Length of output: 14504
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the host-preparation call path and the local-profile selection path. sed -n '4688,4710p' scripts/install.sh sed -n '4838,4905p' scripts/install.sh sed -n '5018,5120p' scripts/install.sh rg -n -C 8 'LOCAL_MODEL_RUNTIME|local-model-profile|host.platform.dgx_spark|resolve.*profile|profile.*resolve' scripts managed-inferenceRepository: NVIDIA/NemoClaw
Length of output: 26301
🏁 Script executed:
#!/bin/bash set -euo pipefail # Locate the code that consumes the local-model feature gate and runtime. rg -n -C 10 'NEMOCLAW_ENABLE_LOCAL_MODEL_PROFILE|NEMOCLAW_LOCAL_MODEL_RUNTIME|local-model-profile-v1|local-model-profile\.(vllm|llama-cpp)' . # Inspect profile loading and readiness/error handling without executing repository code. rg -n -C 12 'ServingPreset|requirements|qualification|profile.*selection|selection.*disabled|featureGate' --glob '*.ts' --glob '*.js' --glob '*.sh' --glob '*.yaml' . # Use a read-only static verifier to report the relevant ordering and branch predicates. python3 - <<'PY' from pathlib import Path p = Path("scripts/install.sh") s = p.read_text() needles = [ 'export NEMOCLAW_NO_EXPRESS=1', 'prepare_installer_host', 'maybe_offer_express_install', 'clear_station_express_resume', 'if [ "$platform" = "DGX Station" ]; then', ] for needle in needles: print(f"{needle!r}: {[i + 1 for i, line in enumerate(s.splitlines()) if needle in line]}") start = s.index("maybe_offer_express_install()") end = s.index("\n}", start) + 2 body = s[start:end] print("single_station_clear_branch:", 'clear_station_express_resume' in body and 'NEMOCLAW_NO_EXPRESS' in body and 'station_dual_pair_resume_pending' in body) PYRepository: NVIDIA/NemoClaw
Length of output: 50372
🏁 Script executed:
#!/bin/bash set -euo pipefail # Extract only the static-verifier results and the production consumers from the # previous output, avoiding test fixtures and broad catalog matches. rg -n 'single_station_clear_branch|export NEMOCLAW_NO_EXPRESS=1|prepare_installer_host|maybe_offer_express_install|clear_station_express_resume' \ /tmp/coderabbit-shell-logs/shell-output-FE1PVE rg -l --glob '!**/*.test.ts' --glob '!**/*.test-support.ts' \ 'NEMOCLAW_ENABLE_LOCAL_MODEL_PROFILE|NEMOCLAW_LOCAL_MODEL_RUNTIME|local-model-profile-v1' \ src scripts # Inspect the resolver implementation and the local-profile integration points. for f in $(rg -l --glob '!**/*.test.ts' --glob '!**/*.test-support.ts' \ 'NEMOCLAW_ENABLE_LOCAL_MODEL_PROFILE|NEMOCLAW_LOCAL_MODEL_RUNTIME|local-model-profile-v1' src scripts); do echo "===== $f =====" rg -n -C 8 'NEMOCLAW_ENABLE_LOCAL_MODEL_PROFILE|NEMOCLAW_LOCAL_MODEL_RUNTIME|local-model-profile-v1|no-match|requirements-not-met|selection' "$f" doneRepository: NVIDIA/NemoClaw
Length of output: 15183
🏁 Script executed:
#!/bin/bash set -euo pipefail # Read the local-profile planning path and the resolver result handling. sed -n '1,190p' src/lib/onboard/local-model-profile/plan.ts rg -n -C 12 'buildLocalModelProfilePlan|resolveManagedInferenceServing|requirements-not-met|no-match|local model profile' \ src/lib/onboard src/lib/inference --glob '!**/*.test.ts' # Show the exact Spark qualification in both local-model presets. for f in managed-inference/presets/local-model-profile.*.yaml; do echo "===== $f =====" sed -n '1,45p' "$f" doneRepository: NVIDIA/NemoClaw
Length of output: 50371
Preserve Station resume state for invalid local-model selections. With
--local-model-runtime=vllm,NEMOCLAW_NO_EXPRESS=1causesmaybe_offer_express_install()to clear the single-Station resume state before the local-model onboarder rejects the non-Spark host. Reject local-model runtimes when the detected platform is not DGX Spark, or defer this export until after the Station resume boundary.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/install.sh` around lines 5033 - 5045, Update the local-model setup around the NEMOCLAW_ENABLE_LOCAL_MODEL_PROFILE case so invalid runtimes on non-DGX Spark hosts are rejected before NEMOCLAW_NO_EXPRESS is exported, or defer that export until after the Station resume boundary. Preserve the existing valid vllm and llama-cpp behavior while ensuring maybe_offer_express_install() does not clear resume state for rejected selections.src/lib/inference/local-model-profile/cleanup.ts (1)
491-504: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Description: Check whether the docker adapter applies a default timeout. set -euo pipefail fd 'run.ts|container.ts' src/lib/adapters/docker --exec cat -nRepository: NVIDIA/NemoClaw
Length of output: 4273
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- cleanup symbols and Docker calls ---' rg -n -C 5 'inspectOwnedResource|cleanupHostLocalVllm|cleanupLlamaCpp|runLocalModelRuntimeCleanup|deps\.run|docker' src/lib/inference/local-model-profile/cleanup.ts printf '%s\n' '--- runner implementation and timeout semantics ---' fd -t f . src/lib | rg '(^|/)runner(\.|/)|runner' | head -80 rg -n -C 8 'timeout|kill|spawn|ignoreError|suppressOutput' src/lib --glob '*.ts' | head -240 printf '%s\n' '--- cleanup callers ---' rg -n -C 6 'runLocalModelRuntimeCleanup|local-model-profile/cleanup|cleanup\.ts' src --glob '*.{ts,tsx}'Repository: NVIDIA/NemoClaw
Length of output: 32539
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- runner.ts ---' cat -n src/lib/runner.ts printf '%s\n' '--- cleanup implementation ---' sed -n '60,95p;350,470p;470,530p' src/lib/inference/local-model-profile/cleanup.ts printf '%s\n' '--- cleanup tests and timeout assertions ---' rg -n -C 5 'timeout|info|forceRm|network.*rm|container.*removal|unresponsive|Docker is unavailable' src/lib/inference src/lib/actions/uninstall --glob '*.{ts,tsx}'Repository: NVIDIA/NemoClaw
Length of output: 50372
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- runner.ts timeout implementation ---' rg -n -C 12 'export function run|function run|spawnSync|timeout|timedOut|status' src/lib/runner.ts printf '%s\n' '--- cleanup implementation ---' sed -n '60,92p' src/lib/inference/local-model-profile/cleanup.ts sed -n '350,470p' src/lib/inference/local-model-profile/cleanup.ts sed -n '470,525p' src/lib/inference/local-model-profile/cleanup.ts printf '%s\n' '--- cleanup test files ---' fd -t f . src/lib/inference/local-model-profile | sort rg -n -C 4 'cleanupLocalModelRuntimes|deps\.run|deps\.forceRm|timeout|Docker is unavailable|removal failed' src/lib/inference/local-model-profile --glob '*.{ts,tsx}'Repository: NVIDIA/NemoClaw
Length of output: 41757
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' from pathlib import Path import re cleanup = Path("src/lib/inference/local-model-profile/cleanup.ts").read_text() runner = Path("src/lib/runner.ts").read_text() checks = { "inspect calls have timeout": bool(re.search( r'capture\(\[kind,\s*"inspect",\s*name\],\s*\{\s*ignoreError:\s*true,\s*timeout:\s*10_000', cleanup, re.S)), "docker info has no timeout": not bool(re.search( r'deps\.run\(\["info"\],\s*\{[^}]*\btimeout\s*:', cleanup, re.S)), "host-local force removal has no timeout": not bool(re.search( r'deps\.forceRm\(inspected\.id,\s*\{[^}]*\btimeout\s*:', cleanup, re.S)), "llama.cpp force removal has no timeout": not bool(re.search( r'deps\.forceRm\(container\.id,\s*\{[^}]*\btimeout\s*:', cleanup, re.S)), "network removal has no timeout": not bool(re.search( r'deps\.run\(\["network",\s*"rm",\s*network\.id\],\s*\{[^}]*\btimeout\s*:', cleanup, re.S)), "runner forwards timeout to spawnSync": bool(re.search( r'spawnSync\(exe,\s*args,\s*\{\s*\.\.\.spawnOpts', runner, re.S)), "runner documents timeout as spawn error": "call times out" in runner, } for name, value in checks.items(): print(f"{name}: {value}") if not all(checks.values()): raise SystemExit("unexpected source shape") PYRepository: NVIDIA/NemoClaw
Length of output: 436
Set an explicit timeout on all Docker cleanup calls.
deps.run(["info"], ...)and the removal calls at lines 399, 441, and 461 lack timeouts. An unresponsive Docker daemon can block synchronous uninstall indefinitely. Passtimeout: 10_000to each call so timeout results reach the existing failure handling.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/inference/local-model-profile/cleanup.ts` around lines 491 - 504, Set timeout: 10_000 on the Docker invocations in the cleanup flow, including deps.run(["info"], ...) within the main try block and the removal calls in cleanupHostLocalVllm and cleanupLlamaCpp. Preserve the existing options and failure-handling behavior so timeout results are processed normally.src/lib/inference/local.ts (1)
548-552: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Description: Locate callers of the managed vLLM provider state/binding APIs and check which pass getManagedBaseUrlImpl. set -euo pipefail rg -nP --type=ts -C6 '\bgetManagedBaseUrlImpl\b' -g '!**/*.test.ts' echo '--- all call sites of the provider state/binding entrypoints ---' rg -nP --type=ts -C4 '\bgetManagedVllmProviders?(State|Binding)\s*\(' -g '!**/*.test.ts'Repository: NVIDIA/NemoClaw
Length of output: 153
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- candidate files ---' fd -t f 'local\.ts$|README\.md$|CONTRIBUTING\.md$' src printf '%s\n' '--- local.ts structure ---' ast-grep outline src/lib/inference/local.ts printf '%s\n' '--- relevant definitions and references ---' rg -n -C5 'recoverHostLocalManagedVllmEndpoint|getManagedBaseUrlImpl|getManagedVllmProviderState|getManagedVllmProviderBinding|getManagedVllmProviders' src --glob '*.ts' --glob '*.tsx'Repository: NVIDIA/NemoClaw
Length of output: 50371
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- provider-state implementation ---' sed -n '420,595p' src/lib/inference/local.ts printf '%s\n' '--- production option plumbing ---' rg -n -C8 'getManagedVllmBaseUrlImpl|getManagedBaseUrlImpl|probeLocalProviderHealth\(' src/lib --glob '*.ts' --glob '!*.test.ts' printf '%s\n' '--- focused tests for provider-state precedence ---' sed -n '100,210p' src/lib/inference/local-vllm-auth.test.ts sed -n '360,400p' src/lib/inference/local-vllm-auth.test.ts printf '%s\n' '--- relevant documentation ---' rg -n -C4 'getManagedBaseUrlImpl|Station lifecycle|host-local|managed vLLM' src/lib/README.md src/lib/inference/README.mdRepository: NVIDIA/NemoClaw
Length of output: 21031
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- all host-local seam uses ---' rg -n -C5 'recoverHostLocalManagedVllmEndpointImpl' src --glob '*.ts' --glob '*.tsx' printf '%s\n' '--- all Station seam uses, excluding tests ---' rg -n -C5 'getManagedBaseUrlImpl|getManagedVllmBaseUrlImpl' src --glob '*.ts' --glob '*.tsx' --glob '!*.test.ts' printf '%s\n' '--- focused host-local/provider-state tests ---' rg -n -C8 'host.local|host-local|multiple managed|present|recoverHost' \ src/lib/inference/local-vllm-auth.test.ts \ src/lib/inference/serving/vllm-host-local-lifecycle.test.tsRepository: NVIDIA/NemoClaw
Length of output: 18366
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' def current_host_local_endpoint(options, default_endpoint): if options.get("recoverHostLocalManagedVllmEndpointImpl") is not None: return options["recoverHostLocalManagedVllmEndpointImpl"] if options.get("getManagedBaseUrlImpl") is not None: return None return default_endpoint def provider_state(options, station_present, cluster_present, host_present): host = current_host_local_endpoint( options, "host-local-endpoint" if host_present else None, ) present_count = int(cluster_present) + int(host is not None) + int(station_present) return host, present_count options = {"getManagedBaseUrlImpl": "station-test-seam"} host, present_count = provider_state(options, station_present=True, cluster_present=False, host_present=True) assert host is None assert present_count == 1 host, present_count = provider_state(options, station_present=True, cluster_present=False, host_present=True) assert host is None assert present_count != 2 options = { "getManagedBaseUrlImpl": "station-test-seam", "recoverHostLocalManagedVllmEndpointImpl": "host-test-seam", } host, present_count = provider_state(options, station_present=True, cluster_present=False, host_present=True) assert host == "host-test-seam" assert present_count == 2 print("current implementation suppresses host-local recovery when only getManagedBaseUrlImpl is supplied") print("the suppressed endpoint is absent from presentCount and cannot trigger the multiple-runtime conflict") print("an explicit host-local seam preserves independent injection and conflict detection") PYRepository: NVIDIA/NemoClaw
Length of output: 435
Do not gate host-local recovery on
getManagedBaseUrlImpl.
getManagedBaseUrlImplis the Station compatibility seam. It must not suppress host-local detection or conflict checks. UserecoverHostLocalManagedVllmEndpointImpl ?? recoverHostLocalManagedVllmEndpoint; tests that need suppression can inject() => null.Proposed change
- const hostLocalEndpoint = options.recoverHostLocalManagedVllmEndpointImpl - ? options.recoverHostLocalManagedVllmEndpointImpl({ loadApiKey }) - : options.getManagedBaseUrlImpl - ? null - : recoverHostLocalManagedVllmEndpoint({ loadApiKey }); + const hostLocalEndpoint = ( + options.recoverHostLocalManagedVllmEndpointImpl ?? recoverHostLocalManagedVllmEndpoint + )({ loadApiKey });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const hostLocalEndpoint = ( options.recoverHostLocalManagedVllmEndpointImpl ?? recoverHostLocalManagedVllmEndpoint )({ loadApiKey });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/inference/local.ts` around lines 548 - 552, Update the hostLocalEndpoint selection to use the injected recoverHostLocalManagedVllmEndpointImpl when provided, otherwise fall back directly to recoverHostLocalManagedVllmEndpoint, without checking getManagedBaseUrlImpl. Preserve injection of () => null as the mechanism for tests that need to suppress recovery.src/lib/inference/serving/adapter-registry.ts (1)
99-114: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Show how the host-local vLLM materializer builds the serving command line. fd -t f 'vllm-host-local-lifecycle.ts' -x cat -n {} # Find every place the materializer emits model-selection arguments. rg -n -C 6 -- '--model|--served-model-name|servedName|spec\.model\.id' src/lib/inference/servingRepository: NVIDIA/NemoClaw
Length of output: 50371
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- candidate materializer and selection files ---' fd -t f -i 'vllm|material|selection|adapter-registry' src/lib/inference/serving printf '%s\n' '--- host-local materializer symbols and command construction ---' rg -n -C 12 --glob '*.ts' \ 'materializ|VllmModelDef|modelArgs|servedModelId|command|docker.*run|vllm.*serve|structuredArguments' \ src/lib/inference/serving printf '%s\n' '--- argument ownership and validation definitions ---' sed -n '1,140p' src/lib/inference/serving/adapter-registry.ts sed -n '330,390p' src/lib/inference/serving/adapter-registry.ts sed -n '1,180p' src/lib/inference/serving/host-local-vllm-selection.tsRepository: NVIDIA/NemoClaw
Length of output: 50372
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- host-local serving files ---' fd -t f -i 'host-local|vllm' src/lib/inference/serving | grep -v '\.test\.ts$' printf '%s\n' '--- host-local command builders ---' rg -n -C 10 --glob '!*.test.ts' \ 'HOST_LOCAL|host-local|modelArgs|servedModelId|recipe\.spec\.model\.id|["'\'']--model["'\'']|vllm serve|arguments:' \ src/lib/inference/serving printf '%s\n' '--- all host-local lifecycle/materializer references ---' rg -n -C 8 --glob '!*.test.ts' \ 'vllm\.host-local|host-local\.lifecycle|materializerRef|startContainer|command\.arguments' \ src/lib/inference/servingRepository: NVIDIA/NemoClaw
Length of output: 50372
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- host-local selection materialization ---' sed -n '1,170p' src/lib/inference/serving/host-local-vllm-selection.ts printf '%s\n' '--- vLLM model definition and command assembly ---' fd -t f 'vllm-models.ts|vllm.ts|vllm-prompt.ts' src/lib -x sh -c ' echo "--- $1" rg -n -C 14 -- "modelArgs|servedModelId|model\.id|--model|serve" "$1" ' sh {} printf '%s\n' '--- lifecycle calls into the host-local serving path ---' rg -n -C 16 --glob '!*.test.ts' \ 'resolveHostLocalVllmSelection|materializeHostLocalVllmSelection|VllmModelDef|modelArgs|serveEnvironment|install.*Vllm|start.*Vllm' \ src/libRepository: NVIDIA/NemoClaw
Length of output: 50372
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- vLLM run argv construction ---' sed -n '507,620p' src/lib/inference/vllm.ts printf '%s\n' '--- host-local install launch path ---' sed -n '1740,1915p' src/lib/inference/vllm.ts printf '%s\n' '--- command assembly references ---' rg -n -C 10 --glob '!*.test.ts' \ 'buildVllmRunArgs|model\.modelArgs|model\.servedModelId|model\.id|fixedServeCommand|--served-model-name' \ src/lib/inference/vllm.ts src/lib/inference/vllm-models.tsRepository: NVIDIA/NemoClaw
Length of output: 38671
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' from pathlib import Path import re registry = Path("src/lib/inference/serving/adapter-registry.ts").read_text() selection = Path("src/lib/inference/serving/host-local-vllm-selection.ts").read_text() models = Path("src/lib/inference/vllm-models.ts").read_text() def set_entries(source, name): match = re.search( rf"{re.escape(name)}\s*=\s*new Set\(\[(.*?)\]\);", source, re.S, ) if not match: raise SystemExit(f"missing set: {name}") return re.findall(r'"([^"]+)"', match.group(1)) owned_registry = set_entries(registry, "HOST_LOCAL_MATERIALIZER_OWNED_ARGUMENTS") owned_selection = set_entries(selection, "MATERIALIZER_OWNED_ARGUMENTS") model_arguments = ["--model", "recipe-model", "--max-model-len", "4096"] filtered = [ token for i, token in enumerate(model_arguments) if not (i == 0 and token in owned_selection) ] argv = ["serve", "recipe-model", "--max-model-len", "262144", *filtered] print("registry owns --model:", "--model" in owned_registry) print("selection owns --model:", "--model" in owned_selection) print("materialized argv:", argv) print("model-selection occurrences:", [ (i, token) for i, token in enumerate(argv) if token in ("recipe-model", "--model") ]) assert "--model" not in owned_registry assert "--model" not in owned_selection assert argv == [ "serve", "recipe-model", "--max-model-len", "262144", "--model", "recipe-model", "--max-model-len", "4096", ] PY printf '%s\n' '--- exact command builder ---' sed -n '683,721p' src/lib/inference/vllm-models.tsRepository: NVIDIA/NemoClaw
Length of output: 2067
Reserve
--modelfor the host-local materializer.
buildVllmServeCommandemitsmodel.idpositionally, whilemodelArguments()forwards a recipe-defined--model. This creates conflicting model selectors. Add--modelto both host-local materializer-owned argument sets so validation rejects it and materialization cannot forward it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/inference/serving/adapter-registry.ts` around lines 99 - 114, Update the argument ownership definitions used by buildVllmServeCommand and modelArguments to include "--model" in both host-local materializer-owned sets. This must reject recipe-defined --model arguments and prevent forwarding them while preserving the positional model.id emitted by buildVllmServeCommand.src/lib/inference/serving/resolver.ts (1)
155-168: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use exact numeric comparison for version components.
Line 159 converts arbitrary-length version components to
Number. This loses precision. For example, actual9007199254740992incorrectly satisfies minimum9007199254740993because both values round to the same number.Compare validated components as
BigIntvalues, or compare normalized digit strings. Add a boundary test for components larger thanNumber.MAX_SAFE_INTEGER. This breaks the fail-closed readiness qualification required before profile selection.Proposed fix
- const actualParts = actual.split(".").map(Number); - const minimumParts = minimum.split(".").map(Number); + const actualParts = actual.split(".").map((part) => BigInt(part)); + const minimumParts = minimum.split(".").map((part) => BigInt(part)); const width = Math.max(actualParts.length, minimumParts.length); for (let index = 0; index < width; index += 1) { - const actualPart = actualParts[index] ?? 0; - const minimumPart = minimumParts[index] ?? 0; + const actualPart = actualParts[index] ?? 0n; + const minimumPart = minimumParts[index] ?? 0n;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.function versionAtLeast(actual: unknown, minimum: string): boolean { if (typeof actual !== "string") return false; const versionPattern = /^\d+(?:\.\d+)*$/u; if (!versionPattern.test(actual) || !versionPattern.test(minimum)) return false; const actualParts = actual.split(".").map((part) => BigInt(part)); const minimumParts = minimum.split(".").map((part) => BigInt(part)); const width = Math.max(actualParts.length, minimumParts.length); for (let index = 0; index < width; index += 1) { const actualPart = actualParts[index] ?? 0n; const minimumPart = minimumParts[index] ?? 0n; if (actualPart !== minimumPart) return actualPart > minimumPart; } return true; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/inference/serving/resolver.ts` around lines 155 - 168, Update versionAtLeast to compare version components exactly without converting them to Number; use BigInt or normalized digit-string comparison after validation so values beyond Number.MAX_SAFE_INTEGER remain distinguishable. Preserve missing components as zero and the existing lexicographic component ordering, and add a boundary test covering adjacent oversized components to ensure readiness qualification fails when actual is smaller.src/lib/inference/vllm.ts (1)
2027-2046: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Description: Inspect host-local receipt lifecycle and restart-policy handling. set -euo pipefail fd 'vllm-host-local-lifecycle.ts' src | xargs -r cat -n rg -n -C 4 'unless-stopped|dockerStop|dockerForceRm' --type=ts src/lib/inference/vllm.tsRepository: NVIDIA/NemoClaw
Length of output: 18172
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '1900,2060p' src/lib/inference/vllm.ts rg -n -C 5 'HOST_LOCAL_VLLM_RUNTIME_RECEIPT_FILE|persistHostLocalVllmRuntimeReceipt|unlink|rmSync|writeLocalAdapterJsonFile|managedVllmStateDir' src/lib/inference src/libRepository: NVIDIA/NemoClaw
Length of output: 50372
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- vllm install lifecycle ---' sed -n '1935,2055p' src/lib/inference/vllm.ts printf '%s\n' '--- receipt references ---' rg -n 'HOST_LOCAL_VLLM_RUNTIME_RECEIPT_FILE|persistHostLocalVllmRuntimeReceipt|runtimeReceiptPath|unlinkSync|rmSync' \ src/lib/inference/vllm.ts src/lib/inference/vllm-host-local-lifecycle.ts src/lib/local-adapter-lifecycle.ts src/lib/vllm-api-key.ts 2>/dev/null || true printf '%s\n' '--- relevant imports ---' sed -n '1,35p' src/lib/inference/vllm.tsRepository: NVIDIA/NemoClaw
Length of output: 5458
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- lifecycle module path ---' fd -i 'vllm.*lifecycle.*\.ts' src printf '%s\n' '--- vllm imports and lifecycle API ---' sed -n '30,60p' src/lib/inference/vllm.ts for f in $(fd -i 'vllm.*lifecycle.*\.ts' src); do printf '\n--- %s ---\n' "$f" rg -n -C 4 'export|receipt|unlink|rmSync|cleanup|recover|docker(Start|Stop|ForceRm)' "$f" done printf '%s\n' '--- recovery callers ---' rg -n -C 5 'recoverHostLocalManagedVllmEndpoint|validateHostLocalVllmRuntimeReceipt' src --glob '*.ts'Repository: NVIDIA/NemoClaw
Length of output: 50371
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- host-local cleanup implementation ---' sed -n '1,55p' src/lib/inference/local-model-profile/cleanup.ts sed -n '330,430p' src/lib/inference/local-model-profile/cleanup.ts printf '%s\n' '--- auth-boundary implementation ---' rg -n -C 12 'function verifyDualStationVllmAuthBoundary|verifyDualStationVllmAuthBoundary' src/lib/inference/vllm.ts src/lib/inference/serving --glob '*.ts' printf '%s\n' '--- host-local lifecycle test coverage for stopped containers ---' rg -n -C 6 'Running|stopped|restart|receipt|cleanup' src/lib/inference/serving/vllm-host-local-lifecycle.test.tsRepository: NVIDIA/NemoClaw
Length of output: 13184
🌐 Web query:
Docker official documentation unless-stopped manually stopped container daemon restart behavior💡 Result:
When you use the
unless-stoppedrestart policy, the Docker daemon will automatically restart a container if it exits, unless it was explicitly stopped by a user or process [1][2]. Regarding the interaction between manual stops and daemon restarts: - If you manually stop a container (e.g., viadocker stop), the Docker daemon will not restart it, even if the Docker daemon itself is subsequently restarted [1][2]. The container remains in a stopped state until it is manually started again [1][3]. - If the container was running when the Docker daemon was stopped or restarted (i.e., it was not manually stopped first), theunless-stoppedpolicy will cause the container to automatically start when the daemon comes back online [1][4]. In essence, theunless-stoppedpolicy effectively "remembers" that you intentionally stopped the container and respects that intent by preventing the daemon from automatically bringing it back up [1][5]. If the container stops for any other reason (such as a crash or a system reboot that kills the daemon), it will restart [1][3]. Note that certain commands, such asdocker kill, may be interpreted by the daemon as a request to stop the container, which can result in the container failing to restart after a daemon or host reboot [6].Citations:
- 1: https://docs.docker.com/engine/containers/start-containers-automatically/
- 2: https://docs.docker.com/reference/cli/docker/container/run
- 3: https://www.baeldung.com/ops/docker-containers-start-automatically
- 4: docker/docs#6287
- 5: docker/for-linux#652
- 6: moby/moby#47792
Security Misconfiguration (CWE-459)
Reachability: Internal · Exploitability: Difficult
Remove the container and receipt when the auth-boundary check fails.
dockerStopprevents automatic restart but leaves the container and receipt. A later explicit start can pass recovery using the stale receipt without rerunning the auth-boundary check. UsedockerForceRm(start.containerId, ...)and deleteHOST_LOCAL_VLLM_RUNTIME_RECEIPT_FILE.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/inference/vllm.ts` around lines 2027 - 2046, Update the failed auth-boundary branch in the managed-auth flow of the vLLM install logic to force-remove the container with dockerForceRm using the start container identifier, then delete HOST_LOCAL_VLLM_RUNTIME_RECEIPT_FILE before returning failure. Replace the existing dockerStop cleanup so no stale container or receipt remains.test/install-local-model-profile.test.ts (1)
22-31: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Clear all profile-selection inputs from the parent environment.
Lines 22-31 retain unspecified
NEMOCLAW_*values fromprocess.env.
An ambient serving preset, vLLM model, vLLM extra-arguments value, or managed-cluster peer value can change the installer branch before the harness runs.
Clear every profile-selection input that the installer reads, then add only the value under test.Proposed fix
NEMOCLAW_LOCAL_MODEL_RUNTIME: "", NEMOCLAW_MODEL: "", + NEMOCLAW_MANAGED_CLUSTER_PEERS: "", NEMOCLAW_NO_EXPRESS: "", NEMOCLAW_PROVIDER: "", + NEMOCLAW_SERVING_PRESET: "", + NEMOCLAW_VLLM_EXTRA_ARGS_JSON: "", + NEMOCLAW_VLLM_MODEL: "", NEMOCLAW_VLLM_PORT: "",As per coding guidelines, deterministic tests must undo environment stubs.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.env: { ...process.env, INSTALLER_UNDER_TEST: INSTALLER, NEMOCLAW_ENABLE_LOCAL_MODEL_PROFILE: "", NEMOCLAW_LOCAL_MODEL_RUNTIME: "", NEMOCLAW_MODEL: "", NEMOCLAW_MANAGED_CLUSTER_PEERS: "", NEMOCLAW_NO_EXPRESS: "", NEMOCLAW_PROVIDER: "", NEMOCLAW_SERVING_PRESET: "", NEMOCLAW_VLLM_EXTRA_ARGS_JSON: "", NEMOCLAW_VLLM_MODEL: "", NEMOCLAW_VLLM_PORT: "", ...env,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/install-local-model-profile.test.ts` around lines 22 - 31, Update the environment setup in the test harness around INSTALLER_UNDER_TEST to clear every NEMOCLAW profile-selection variable read by the installer, including serving preset, vLLM model and extra arguments, and managed-cluster peer settings; then overlay only the scenario-specific values from env so parent-process values cannot affect the branch under test.Source: Coding guidelines
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
2 participants
Summary
Adds discoverable serving profiles and carries the exact selected catalog preset and recipe identity through onboarding review, resume, status, diagnostics, recovery, and uninstall. Existing automatic defaults remain unchanged when no profile is selected, and legacy sessions and unlabeled runtimes remain compatible.
Related Issues
Fixes #8246
Fixes #8384
Parent epic: #8379
Changes
profiles listdiscovery with stable IDs, display names, backend, model, topology, selection mode, support state, download estimates, and compatibility reasons.onboard --profile <id-or-display-name>selection, with terminal-safe errors for unknown, ambiguous, disabled, incompatible, or conflicting selections before effects.Station catalog migration and physical hardware qualification remain separately tracked under the parent epic.
Type of Change
Quality Gates
Documentation Writer Review
docs-updated7c25b6175for profile discovery, review, immutable provenance, resume drift rejection, status/diagnostics, authenticated recovery, legacy compatibility, and ownership-aware uninstall were reviewed.npm run docs, focused profile tests,git diff --check, and privacy checks passed; no private or unannounced model details or credentials were added.DGX Station Hardware Evidence
Verification
Signed-off-by:line and every authored commit is signed and includes DCO sign-offnpm run docsbuilds without errors (Fern reported 2 existing unprinted warnings)Signed-off-by: Prekshi Vyas prekshiv@nvidia.com
Summary by CodeRabbit
New Features
profiles:listto discover installed serving profiles, view compatibility, and output results as text or JSON.Documentation
Bug Fixes