feat(inference): attach existing llama.cpp servers - #8167
Conversation
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
ericksoa
added
area: inference
|
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 cooperative llama.cpp existing-server attachment. The change adds authenticated native probing, bounded HTTP handling, port and credential safeguards, provider routing, onboarding selection, endpoint compatibility, and local-inference policy support. ChangesLlama.cpp attachment
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant SetupNimFlow
participant LlamaCppSelection
participant probeLlamaCppAttachment
participant LlamaCppServer
Operator->>SetupNimFlow: select llama-cpp
SetupNimFlow->>LlamaCppSelection: pass model and selection state
LlamaCppSelection->>probeLlamaCppAttachment: probe loopback endpoint
probeLlamaCppAttachment->>LlamaCppServer: send authenticated bounded probes
LlamaCppServer-->>probeLlamaCppAttachment: return model and native metadata
probeLlamaCppAttachment-->>LlamaCppSelection: return attachment result
LlamaCppSelection-->>SetupNimFlow: return llama-cpp-local state
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 14ae94a in the TypeScript / code-coverage/cliThe overall coverage in commit 14ae94a in the Show a code coverage summary of the most impacted files.
Updated |
Signed-off-by: Aaron Erickson <aerickson@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: 1
🧹 Nitpick comments (11)
src/lib/onboard/inference-providers/remote-openai-surface.test.ts (1)
79-89: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExercise the added provider fixture.
The test supplies
endpointUrlandcredentialEnvdirectly. This bypasses the newREMOTE_PROVIDER_CONFIG["llama-cpp"]values. The test still passes if the fixture endpoint or credential environment variable is incorrect.Pass
nullfor both arguments, or add a separate case that uses configuration defaults.setupRemoteProviderInferenceresolves null arguments from the selected provider configuration.Suggested test adjustment
- endpointUrl: "http://127.0.0.1:8081/v1", - credentialEnv: "NEMOCLAW_LLAMACPP_LOCAL_TOKEN", + endpointUrl: null, + credentialEnv: null,As per path instructions, keep this test behavior-oriented and protect the new configuration contract through the setup boundary.
Also applies to: 336-344
🤖 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/inference-providers/remote-openai-surface.test.ts` around lines 79 - 89, Update the llama-cpp fixture cases around the provider setup to pass null for endpointUrl and credentialEnv, allowing setupRemoteProviderInference to resolve REMOTE_PROVIDER_CONFIG["llama-cpp"] defaults. Preserve the existing behavior assertions while ensuring the tests exercise the configuration contract through the setup boundary.Source: Path instructions
src/lib/security/credential-env.ts (1)
52-52: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe credential name matches the contract constant.
LLAMA_CPP_CREDENTIAL_ENVinsrc/lib/inference/llama-cpp/contract.tsdefines the same string. The literal is repeated here and intest/e2e/live/snapshot-credential-scanner.ts. See the consolidated note about drift risk.🤖 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/security/credential-env.ts` at line 52, Replace the repeated "NEMOCLAW_LLAMACPP_LOCAL_TOKEN" literal in the credential environment list with the shared LLAMA_CPP_CREDENTIAL_ENV constant from the llama-cpp contract, preserving the existing credential entry and avoiding further duplication.src/lib/core/ports.test.ts (1)
164-182: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a positive case for the default port set.
The suite proves each collision throws. It does not prove the default configuration passes. A single
expect(() => validateLlamaCppPortReservation(GATEWAY_VALIDATION_OPTIONS)).not.toThrow()case would guard against an over-broad reservation check. This is optional.🤖 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/core/ports.test.ts` around lines 164 - 182, Add a positive test in the validateLlamaCppPortReservation suite that calls validateLlamaCppPortReservation with GATEWAY_VALIDATION_OPTIONS and asserts it does not throw, preserving coverage that the default port configuration is accepted.src/lib/inference/llama-cpp/index.test.ts (1)
10-19: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the
as CurlProbeResultcast.
curlFailurebuilds the same shape without a cast. The cast inresponsehides future field mismatches inCurlProbeResult. Remove it so the type checker validates the fixture.♻️ Proposed change
- message: `HTTP ${httpStatus}`, - } as CurlProbeResult; + message: `HTTP ${httpStatus}`, + };🤖 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/index.test.ts` around lines 10 - 19, Remove the `as CurlProbeResult` assertion from the object returned by the `response` test helper, allowing TypeScript to structurally validate the fixture against `CurlProbeResult` like `curlFailure` does. Preserve the existing fields and values.src/lib/onboard/providers.ts (2)
181-191: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the contract constants instead of repeating literals.
The contract module already exports
LLAMA_CPP_PROVIDER_LABEL("Local llama.cpp") andLLAMA_CPP_PROVIDER_NAME("llama-cpp-local"). Line 182 repeats the label text and line 199 repeats the provider name, whileLLAMA_CPP_PROVIDER_NAMEis already imported at line 22. Reuse the constants so the contract stays the single source of truth.♻️ Proposed change
"llama-cpp": { - label: "Local llama.cpp", + label: LLAMA_CPP_PROVIDER_LABEL, providerName: LLAMA_CPP_PROVIDER_NAME,-const LOCAL_INFERENCE_POLICY_PROVIDERS = [...LOCAL_INFERENCE_PROVIDERS, "llama-cpp-local"]; +const LOCAL_INFERENCE_POLICY_PROVIDERS = [ + ...LOCAL_INFERENCE_PROVIDERS, + LLAMA_CPP_PROVIDER_NAME, +];Add
LLAMA_CPP_PROVIDER_LABELto the destructuredrequireat lines 19-23.Also applies to: 199-199
🤖 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/providers.ts` around lines 181 - 191, Update the llama-cpp provider definition to use the contract constants instead of duplicated literals: import LLAMA_CPP_PROVIDER_LABEL alongside the existing LLAMA_CPP_PROVIDER_NAME, use the label constant for label, and use LLAMA_CPP_PROVIDER_NAME for providerName. Keep the contract module as the single source of truth.
228-229: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBoth new switch branches are unreachable. The root cause is that
llama-cppis registered inREMOTE_PROVIDER_CONFIGwithproviderName: "llama-cpp-local". Both helpers consult that map before their switch statements, so neither new case can execute.
src/lib/onboard/providers.ts#L228-L229: remove thecase "llama-cpp-local"branch, because theREMOTE_PROVIDER_CONFIGscan at lines 216-218 already returns the label.src/lib/onboard/providers.ts#L249-L250: remove thecase "llama-cpp"branch, because lines 239-241 already returnREMOTE_PROVIDER_CONFIG["llama-cpp"].providerName.Keep the branches only if a caller passes these keys before registration; state that caller if so.
🤖 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/providers.ts` around lines 228 - 229, Remove the unreachable case "llama-cpp-local" branch from the provider label helper and the case "llama-cpp" branch from the corresponding provider-name helper in src/lib/onboard/providers.ts at lines 228-229 and 249-250; the preceding REMOTE_PROVIDER_CONFIG lookups already handle both keys, and no caller is identified that passes them before registration.src/lib/core/ports.ts (1)
260-303: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider narrowing the parameter type of
validateLlamaCppPortReservation.The function reads only the port fields. It ignores
dashboardRangeStartanddashboardRangeEnd, soRuntimeAdapterPortValidationOptionsoverstates the required input. A narrower parameter type would document the actual contract. The current form still works, so this is optional.🤖 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/core/ports.ts` around lines 260 - 303, Narrow the parameter type of validateLlamaCppPortReservation to include only the configurable port fields it reads, excluding dashboardRangeStart and dashboardRangeEnd; keep the existing collision validation and call-site behavior unchanged.src/lib/inference/llama-cpp/index.ts (1)
261-269: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant
modelsguard.Line 262 already returns when
modelsis null. The conditional at line 269 cannot take thenullbranch.♻️ Proposed simplification
- const modelEntry = models ? selectModelEntry(models, requestedModel) : null; + const modelEntry = selectModelEntry(models, requestedModel);🤖 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/index.ts` around lines 261 - 269, Remove the redundant null check from the model selection assignment after the validation in the llama.cpp inference flow. Update the expression involving selectModelEntry and requestedModel to pass the validated models value directly, while preserving the existing malformed-catalog failure behavior.src/lib/inference/llama-cpp/contract.ts (1)
10-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unused
LLAMA_CPP_GATEWAY_BASE_URLexport. No TypeScript consumer references it. The policy already allowshost.openshell.internal:8081; the route usesLLAMA_CPP_PORTand constructs the URL directly.🤖 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/contract.ts` around lines 10 - 12, Remove the unused LLAMA_CPP_GATEWAY_BASE_URL export from the contract module, leaving LLAMA_CPP_HOST_BASE_URL and LLAMA_CPP_HOST_OPENAI_BASE_URL unchanged.src/lib/onboard/llama-cpp-selection/index.test.ts (1)
26-41: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for the two untested credential-resolution branches.
The suite covers the non-interactive missing-token exit path (lines 83-95) but does not cover two other fail-closed branches in
createLlamaCppSelectionHandler(index.tslines 60-71):
- Interactive path where
ensureNamedCredentialresult satisfiesreturningToProviderSelection, which must return"retry-selection"without probing.- Interactive path where
ensureNamedCredentialresolves without a usable string credential, which must return"retry-selection".Add two tests using
deps({ resolveCredential: () => null, ensureNamedCredential: ... })overrides to exercise these branches and confirm the probe is never called.Also applies to: 83-95
🤖 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/llama-cpp-selection/index.test.ts` around lines 26 - 41, Add two tests for the interactive credential-resolution branches in createLlamaCppSelectionHandler, overriding resolveCredential to return null and ensureNamedCredential for each scenario: one where the result indicates returningToProviderSelection and one where it resolves without a usable string credential. Assert both return "retry-selection" and verify probeLlamaCppAttachment is never called.src/lib/onboard/machine/handlers/provider-inference-route-containment.ts (1)
66-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate the duplicated custom-route provider list.
This inline array duplicates
CUSTOM_ROUTE_PROVIDERSinsrc/lib/inference/gateway-route-compatibility.ts. Two lists now enumerate the same three provider names. If a future change updates one list and not the other,guardProviderInferenceRouteSelectionandcheckGatewayRouteCompatibilitywill disagree on which providers require a complete custom route.Export
CUSTOM_ROUTE_PROVIDERSfromgateway-route-compatibility.tsand import it here instead of re-declaring the literal array.♻️ Proposed fix
- const completeCustomRoute = - !["compatible-endpoint", "compatible-anthropic-endpoint", "llama-cpp-local"].includes( - provider, - ) || + const completeCustomRoute = + !CUSTOM_ROUTE_PROVIDERS.has(provider) || (typeof route.endpointUrl === "string" && route.endpointUrl.trim().length > 0 && typeof route.preferredInferenceApi === "string" && route.preferredInferenceApi.trim().length > 0);// gateway-route-compatibility.ts export const CUSTOM_ROUTE_PROVIDERS = new Set([ "compatible-endpoint", "compatible-anthropic-endpoint", "llama-cpp-local", ]);🤖 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/machine/handlers/provider-inference-route-containment.ts` around lines 66 - 74, Export the existing CUSTOM_ROUTE_PROVIDERS symbol from gateway-route-compatibility.ts and import it into guardProviderInferenceRouteSelection. Replace the inline provider-name array in the completeCustomRoute calculation with the shared set, preserving the current membership check and 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 `@src/lib/inference/config.ts`:
- Around line 220-226: Update the "llama-cpp-local" branch in the inference
configuration selection to require a valid operator-supplied served model alias,
removing the default "llama-cpp-local" value. Enforce rejection of missing or
invalid model input at the selection boundary before constructing the
configuration, while preserving the existing credentialEnv and providerLabel
behavior.
---
Nitpick comments:
In `@src/lib/core/ports.test.ts`:
- Around line 164-182: Add a positive test in the
validateLlamaCppPortReservation suite that calls validateLlamaCppPortReservation
with GATEWAY_VALIDATION_OPTIONS and asserts it does not throw, preserving
coverage that the default port configuration is accepted.
In `@src/lib/core/ports.ts`:
- Around line 260-303: Narrow the parameter type of
validateLlamaCppPortReservation to include only the configurable port fields it
reads, excluding dashboardRangeStart and dashboardRangeEnd; keep the existing
collision validation and call-site behavior unchanged.
In `@src/lib/inference/llama-cpp/contract.ts`:
- Around line 10-12: Remove the unused LLAMA_CPP_GATEWAY_BASE_URL export from
the contract module, leaving LLAMA_CPP_HOST_BASE_URL and
LLAMA_CPP_HOST_OPENAI_BASE_URL unchanged.
In `@src/lib/inference/llama-cpp/index.test.ts`:
- Around line 10-19: Remove the `as CurlProbeResult` assertion from the object
returned by the `response` test helper, allowing TypeScript to structurally
validate the fixture against `CurlProbeResult` like `curlFailure` does. Preserve
the existing fields and values.
In `@src/lib/inference/llama-cpp/index.ts`:
- Around line 261-269: Remove the redundant null check from the model selection
assignment after the validation in the llama.cpp inference flow. Update the
expression involving selectModelEntry and requestedModel to pass the validated
models value directly, while preserving the existing malformed-catalog failure
behavior.
In `@src/lib/onboard/inference-providers/remote-openai-surface.test.ts`:
- Around line 79-89: Update the llama-cpp fixture cases around the provider
setup to pass null for endpointUrl and credentialEnv, allowing
setupRemoteProviderInference to resolve REMOTE_PROVIDER_CONFIG["llama-cpp"]
defaults. Preserve the existing behavior assertions while ensuring the tests
exercise the configuration contract through the setup boundary.
In `@src/lib/onboard/llama-cpp-selection/index.test.ts`:
- Around line 26-41: Add two tests for the interactive credential-resolution
branches in createLlamaCppSelectionHandler, overriding resolveCredential to
return null and ensureNamedCredential for each scenario: one where the result
indicates returningToProviderSelection and one where it resolves without a
usable string credential. Assert both return "retry-selection" and verify
probeLlamaCppAttachment is never called.
In `@src/lib/onboard/machine/handlers/provider-inference-route-containment.ts`:
- Around line 66-74: Export the existing CUSTOM_ROUTE_PROVIDERS symbol from
gateway-route-compatibility.ts and import it into
guardProviderInferenceRouteSelection. Replace the inline provider-name array in
the completeCustomRoute calculation with the shared set, preserving the current
membership check and behavior.
In `@src/lib/onboard/providers.ts`:
- Around line 181-191: Update the llama-cpp provider definition to use the
contract constants instead of duplicated literals: import
LLAMA_CPP_PROVIDER_LABEL alongside the existing LLAMA_CPP_PROVIDER_NAME, use the
label constant for label, and use LLAMA_CPP_PROVIDER_NAME for providerName. Keep
the contract module as the single source of truth.
- Around line 228-229: Remove the unreachable case "llama-cpp-local" branch from
the provider label helper and the case "llama-cpp" branch from the corresponding
provider-name helper in src/lib/onboard/providers.ts at lines 228-229 and
249-250; the preceding REMOTE_PROVIDER_CONFIG lookups already handle both keys,
and no caller is identified that passes them before registration.
In `@src/lib/security/credential-env.ts`:
- Line 52: Replace the repeated "NEMOCLAW_LLAMACPP_LOCAL_TOKEN" literal in the
credential environment list with the shared LLAMA_CPP_CREDENTIAL_ENV constant
from the llama-cpp contract, preserving the existing credential entry and
avoiding further duplication.
🪄 Autofix (Beta)
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: 91fb3adb-8c35-40de-adcb-6e9d66ea99be
📒 Files selected for processing (35)
nemoclaw-blueprint/policies/presets/local-inference.yamlsrc/lib/adapters/http/curl-args.test.tssrc/lib/adapters/http/curl-args.tssrc/lib/core/ports.test.tssrc/lib/core/ports.tssrc/lib/credentials/store.tssrc/lib/inference/config.test.tssrc/lib/inference/config.tssrc/lib/inference/gateway-route-compatibility.test.tssrc/lib/inference/gateway-route-compatibility.tssrc/lib/inference/llama-cpp/contract.tssrc/lib/inference/llama-cpp/index.test.tssrc/lib/inference/llama-cpp/index.tssrc/lib/onboard.tssrc/lib/onboard/inference-providers/compatible-endpoint-gateway-route.test.tssrc/lib/onboard/inference-providers/compatible-endpoint-gateway-route.tssrc/lib/onboard/inference-providers/remote-openai-surface.test.tssrc/lib/onboard/inference-providers/types.tssrc/lib/onboard/llama-cpp-selection/index.test.tssrc/lib/onboard/llama-cpp-selection/index.tssrc/lib/onboard/machine/handlers/provider-inference-route-containment.test.tssrc/lib/onboard/machine/handlers/provider-inference-route-containment.tssrc/lib/onboard/policy-presets.tssrc/lib/onboard/provider-menu.test.tssrc/lib/onboard/provider-menu.tssrc/lib/onboard/providers.tssrc/lib/onboard/setup-nim-flow.test.tssrc/lib/onboard/setup-nim-flow.tssrc/lib/security/credential-env.tssrc/lib/validation.test.tssrc/lib/validation.tstest/e2e/live/snapshot-credential-scanner.tstest/e2e/support/snapshot-credential-scanner.test.tstest/onboard-policy-suggestions.test.tstest/onboard-selection.test.ts
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
6 terminology differences from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
2 additional E2E selections 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. 3 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: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
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.ts`:
- Around line 1093-1094: Update the credential resolution wiring used by
handleLlamaCppSelection so resolveProviderCredential preserves any existing
nonblank process.env[envName] value. Make hydration environment-first and only
load the stored or legacy credential when the environment variable is missing or
blank, before the llama.cpp probe runs.
🪄 Autofix (Beta)
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: a0d89804-65f1-4f2a-8a40-ecf3bb5e828a
📒 Files selected for processing (1)
src/lib/onboard.ts
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/adapters/http/probe.test.ts (1)
160-194: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the non-ENOBUFS oversized-response defensive check.
This test exercises the ENOBUFS/maxBuffer overflow path in
runCurlProbeImpl. The implementation also has a second, independent guard at Line 343 ofprobe.ts(Buffer.byteLength(body) > maxResponseBytes) that catches responses slightly exceedingmaxResponseByteswithout triggering Node'smaxBufferoverflow (for example when the write-out status code is shorter than the"999"placeholder used to sizemaxBuffer). No test in this file exercises that branch directly with a mockedspawnSyncImplthat returns a body over the limit without a spawn error. Add a unit test that mocksspawnSyncImplto return an over-limit body withstatus: 0and noerror, and assert the result matches{ ok: false, curlStatus: 63, body: "" }.🤖 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/adapters/http/probe.test.ts` around lines 160 - 194, Add a unit test in the runCurlProbe coverage that mocks spawnSyncImpl to return a body larger than maxResponseBytes with status 0 and no error, without triggering a spawn failure. Assert the result matches { ok: false, curlStatus: 63, body: "" } to directly cover the Buffer.byteLength(body) defensive check in runCurlProbeImpl.
🤖 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.
Nitpick comments:
In `@src/lib/adapters/http/probe.test.ts`:
- Around line 160-194: Add a unit test in the runCurlProbe coverage that mocks
spawnSyncImpl to return a body larger than maxResponseBytes with status 0 and no
error, without triggering a spawn failure. Assert the result matches { ok:
false, curlStatus: 63, body: "" } to directly cover the Buffer.byteLength(body)
defensive check in runCurlProbeImpl.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 22a0da58-c024-45f0-893c-0c8cc0763260
📒 Files selected for processing (4)
src/lib/adapters/http/probe.test.tssrc/lib/adapters/http/probe.tssrc/lib/inference/llama-cpp/index.test.tssrc/lib/inference/llama-cpp/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/inference/llama-cpp/index.ts
- src/lib/inference/llama-cpp/index.test.ts
|
🌿 Preview your docs: https://nvidia-preview-pr-8167.docs.buildwithfern.com/nemoclaw |
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Reviewed exact head a7c48d5. I found no blocking correctness, security, compatibility, or regression defect. Non-blocking fast follow: resolve the two current CodeRabbit configuration/authentication threads in one narrow follow-up before expanding the served-alias surface; they do not make the current behavior unsafe or unusable. Current exact-head CI is still completing.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Re-reviewed current head 0005dc8 after the served-alias validation commit. The shared validator now gates both probe selection and provider configuration, rejects empty, path-like, GGUF, padded, backslash, and oversized values, and preserves valid namespace-style aliases. Removing the synthetic fallback prevents an unvalidated default model from reaching the supported llama.cpp attachment workflow. The added negative matrix covers the configuration boundary. I found no blocking correctness, security, compatibility, or regression defect; earlier fast-follow suggestions remain non-blocking while exact-head CI runs.
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/inference/llama-cpp/contract.ts`:
- Around line 23-25: Update the alias validation in the contract’s allowlist
function to reject any path containing "." or ".." segments before returning
true, while preserving valid alias handling. Add regression tests covering
"models/../secret" and "foo/./bar" and assert both are rejected.
🪄 Autofix (Beta)
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: e0fc6272-c46c-444a-90a2-621e1292d144
📒 Files selected for processing (4)
src/lib/inference/config.test.tssrc/lib/inference/config.tssrc/lib/inference/llama-cpp/contract.tssrc/lib/inference/llama-cpp/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/inference/config.test.ts
- src/lib/inference/config.ts
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Re-reviewed the current revision through head 1127398. The bounded-curl helper centralization is behavior-preserving, dot-segment rejection closes the intended route ambiguity, and the final fixture update repairs the exact preflight test contract. Focused validation passed 220 tests across five files; all CLI shards, aggregate tests, build/typecheck, CodeQL, macOS, and WSL pass. Remaining dependency-audit and image failures arise from unchanged reviewed runtime graphs and are not attributable to this PR. I found no blocking correctness, security, compatibility, or regression defect.
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Approve — reviewed the mixed-catalog hardening at exact head 57164bf. The classifier now rejects contradictory non-native catalog entries before model selection while preserving all-native exact-alias behavior. The focused llama.cpp suite passed 27 of 27 tests, and no blocking correctness, security, compatibility, or regression defect was found. The current npm-audit, image-build, and E2E coordination failures are outside this source-only delta and are not attributable to it.
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
…hind (#7901) <!-- markdownlint-disable MD041 --> ## Summary Adds an explicit whole-host uninstall mode for hosts with NemoClaw environments on more than one gateway port. Ordinary uninstall remains scoped to the selected port; the opt-in sweep processes discovered ports independently, keeps shared resources after any incomplete cleanup, and reports a nonzero outcome when work remains. ## Related Issue Related to #7791. ## Changes - Add `--all-gateway-ports` and `NEMOCLAW_UNINSTALL_ALL_GATEWAY_PORTS=1` as explicit sweep controls while preserving the existing one-port default. - Run non-selected ports in isolated child processes and the selected port last so port-scoped state, gateway names, and Docker resources resolve from the correct environment. - Reject a mismatched `--gateway` before cleanup and fail closed when gateway-process cleanup is incomplete or an unenumerated environment remains. - Preserve shared host resources after partial failure, continue independent port cleanup, exit nonzero, and document that completed cleanup is not rolled back. - Update command, uninstall, state, and troubleshooting documentation for data preservation, external supervision, recovery, and one-port versus multi-port confirmation. - Ratchet the source-architecture budget to the measured current-diff values and add focused behavior/security coverage. ## Product Scope - Status: `approved` by the current maintainer in the PR preparation task on 2026-08-03. - Approved contract: the opt-in sweep may remove all discovered gateway-port environments, while ordinary uninstall remains sticky and port-scoped; incomplete or unidentified cleanup fails closed and preserves shared resources. It rejects a mismatched gateway before cleanup, respects OpenShell/external-supervisor and user-data preservation controls, and does not roll back cleanup completed before an independent port failure. - Decision record: [issue #7791 maintainer comment](#7791 (comment)). - This approval is recorded independently of `mergeStateStatus`; it is not inferred from passing tests, review, or CI. The refresh to base `a5562015029fd8cdbebdce5664e8b8bfda9d6ba8` did not change the intended behavior; the stable patch ID and all six original PR commits remain unchanged. ## Automated Review Disposition - All seven GitHub automated-review threads are resolved; six documentation findings were corrected and the CodeQL clear-text logging report was a false positive because the environment value only selects allowlisted branding and is not logged. - CodeRabbit timeout nit: declined. There is no accepted cleanup timeout contract, and terminating a child at an arbitrary deadline could interrupt destructive cleanup mid-step; the foreground process remains operator-interruptible. - CodeRabbit direct-default test nit: nonblocking. Child arguments/environment, exit mapping, state-root enumeration, coordinator behavior, and failure paths are covered without exposing internal production defaults solely for tests. - Current-commit security review: PASS in all nine categories with no PR-diff findings. Updated uninstall, dual-Station, gateway-process, and release coverage passed 111/111, and the localhost gateway identity/release runtime case passed 1/1; the three earlier findings (incomplete process cleanup, delayed gateway-name validation, and unenumerated surviving environments) remain fixed and covered. - Documentation-review blocker after #8158: resolved. A whole-host sweep selected on a non-default port now removes the host-global dual-Station bearer key only after full cleanup is authorized and any managed-pair cleanup succeeds. Scoped cleanup, failed swept ports, and failed managed-pair cleanup preserve the key. The exact reproduction now reports `keyRemains:false` and `selectedRootRemains:false` with exit code 0. - Current-commit GPT-5.6 Terra and optional Nemotron advisor jobs succeeded. Terra recommends `merge_as_is` with medium confidence, Nemotron recommends `merge_as_is` with high confidence, and both canonical finding ledgers are empty. Their runtime-validation recommendation is covered by the required E2E gates tracked below. - Documentation-writer suggestion: no change required. The two temporal uses of “once” are unambiguous, preserve the approved behavior and recovery meaning, and are advisory under the writing policy. - Documentation-writer test-title suggestion: no change required. The plural “credentials” describes the host-global credential class even though the regression asserts the current API-key artifact; the test body and failure contract are exact. - Advisor terminology candidates: no change required. `sweep`, `scoped uninstall`, `sibling gateway-port environment`, `host-global`, and `gateway port` are used consistently with the current controlled vocabulary; neither advisor recorded a terminology finding. - Base-only corporate-CA test warning: nonblocking and unrelated to this PR. On macOS, the new base's GNU-`base64` capability probe accepts FreeBSD `base64`, so three tests reach a later `awk` rejection instead of the expected early diagnostic; every invalid payload still fails nonzero. Linux CI is authoritative, and the probe should be tightened in a separate base follow-up rather than adding unrelated work here. - Base-only HTTP-probe test warning: nonblocking and unrelated to this PR. One broader focused test times out while its test server's SIGTERM handler waits for `server.close()`; the bounded-response implementation and all #7901 interaction assertions pass. This teardown issue belongs in a separate base follow-up. - Fern warnings: nonblocking and unrelated to this patch. Redirect verification was skipped because this local run had no Fern authentication, and the existing light-mode accent color has a 2.41:1 contrast ratio. Fern reported zero errors. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [x] 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] Docs updated for user-facing behavior changes - [ ] Docs 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: Codex Desktop security specialist reviewed commit `489a368a5d0ada868cd01c0dacfc1a99e64a15f1` against base `a5562015029fd8cdbebdce5664e8b8bfda9d6ba8`; no PR-diff security finding, 112 focused tests passed, and unrelated base-test portability warnings were dispositioned above. This is an agent review, not a human sensitive-path owner review or hardware review. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `docs-updated` - Evidence: `docs/manage-sandboxes/uninstall-nemoclaw.mdx`, `docs/reference/commands.mdx`, `docs/reference/host-files-and-state.mdx`, and `docs/reference/troubleshooting.mdx`; all 15 changed files and all 12 OpenClaw, Hermes, and Deep Agents generated pages were reviewed; `npm run docs` passed with 0 errors and 2 unchanged nonblocking Fern warnings; focused documentation/uninstall validation passed 28/28; the non-default-selected sweep reproduction removed the host-global dual-Station key. - Agent: Codex Desktop <!-- docs-review-head-sha: 489a368 --> <!-- docs-review-agents-blob-sha: 3dd7c24 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: - Station profile/scenario: - Result: - Supporting evidence: ## 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: the canonical 13-file uninstall and host-process regression passed 156/156 with a 30-second runner timeout for unrelated local-host CPU contention; uninstall integration passed 6 host-applicable tests with 4 platform-gated PTY skips; independent updated security coverage passed 112 tests; CLI build and post-build CLI typecheck passed. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: not applicable; the change is confined to uninstall coordination and uses focused CLI/integration suites plus repository hooks. CI is authoritative. - [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) - [x] 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) ## Current Commit and Base Evidence - Commit: `489a368a5d0ada868cd01c0dacfc1a99e64a15f1` - Base: `a5562015029fd8cdbebdce5664e8b8bfda9d6ba8` - Refresh: signed, non-force merge commit `e9b83f35b395d76409a14d4bd036f841841124a6` from current `upstream/main`, followed by signed fix commit `489a368a5d0ada868cd01c0dacfc1a99e64a15f1`; both raw commit objects contain SSH signatures. - Patch identity: stable patch ID `6e980dca7f59b4af87c4e402dffc2d0b447d3884`; current binary-diff SHA-256 `f4c2ed0334f2490f5b7fa5471f88520f679544afcc32721f41913e3c5b18cf86`. The prior six original PR commits remain unchanged in range-diff; the new fix commit resolves the #8158 composition finding without rewriting existing history. - GitHub commit audit: all 24 commits displayed by GitHub are `Verified` (`verified=true`, reason `valid`) after the non-force push. - Cross-issue sweep: no medium/high-confidence adjacent fix or contradiction was found; #7791 remains the primary issue. Merged #8158 now composes safely with the whole-host sweep: scoped or failed cleanup preserves host-global dual-Station ownership and successful full cleanup removes the bearer key. Merged #8167 does not change uninstall ownership. Open PR #8129 remains a sequencing overlap only and must preserve this invariant if it lands after #7901. --- Signed-off-by: Tinson Lai <tinsonl@nvidia.com> --------- Signed-off-by: Tinson Lai <tinsonl@nvidia.com> Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Senthil Ravichandran <senthilr@nvidia.com>
3 participants
Summary
NemoClaw can now attach an operator-run, authenticated llama.cpp server on fixed loopback port 8081 as the endpoint-bearing
llama-cpp-localprovider. The flow fails closed on missing or conflicting fingerprint evidence, preserves the generic compatible-endpoint fallback, and keeps managed serving configuration in repository-owned YAML.Related Issue
Fixes #8161
Changes
NEMOCLAW_PROVIDER=llama-cpp. Generic OpenAI compatibility cannot distinguish llama.cpp from vLLM or an arbitrary server, so the adapter requires native authentication plus corroborating/v1/models,/health,/props, and/metricsevidence. A process-level 256 KiB response cap works independently of the host curl version, including unknown-length chunked responses, and served model aliases are limited to 256 bytes. Classifier and selection tests cover positive, ambiguous, spoofed, unauthenticated, timeout, oversized-response, unsafe-alias, and interactive/non-interactive behavior.llama-cpp-localas an OpenAI Chat Completions provider throughhost.openshell.internal:8081while retaining the source loopback endpoint as durable route identity. A provider-specific bridge branch is required so port 8081 does not change the documented generic compatible-endpoint allowlist. Route, registration, containment, and recovery tests protect the source-to-gateway mapping and endpoint/API compatibility.NEMOCLAW_LLAMACPP_LOCAL_TOKENfor credential storage and snapshot scrubbing, and adds port 8081 to the declarativelocal-inferenceYAML policy. The attachment provider remains outside managed local-inference lifecycle ownership, with tests covering port conflicts, policy selection, credential handling, sandbox configuration, and Responses-probe exclusion.Type of Change
Quality Gates
14ae94a8ec7a1ccb3c1ba45f9486ce78c65758e3against current main3fb4ac1d37bec961f494ca39faf996c990b9d06b. All nine categories pass with no blockers or warnings; the final fixed-port matrix rejects gateway and every configurable installer/runtime service collision, including HTTPS Pin and leading-zero08081forms. Focused post-merge reservation checks, shell syntax, build, repository checks, and diff checks passed.Documentation Writer Review
docs-updated14ae94a8ec7a1ccb3c1ba45f9486ce78c65758e3, whose second parent is currentorigin/main3fb4ac1d37bec961f494ca39faf996c990b9d06b. Updated documentation path:docs/reference/commands.mdx. Its reviewed blob is unchanged from the prior approved feature head (1acd7964833b48fd7edccff9f54a686c54836b03).git diff --check origin/main...HEADpassed, andnpm run docspreviously passed on this identical feature-docs tree. No documentation findings remain.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 run typecheck:cli, source architecture, test-conditional, test-file-size, and test-title checks passed.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only) — the strict build passed with zero errors; Fern reported two unrelated existing warnings for redirect authentication and light-mode accent contrast.Signed-off-by: Aaron Erickson aerickson@nvidia.com
Summary by CodeRabbit
New Features
Documentation