fix(policy): allow local inference host gateway - #2993
Conversation
ericksoa
added
bug
NV QA
📝 WalkthroughWalkthroughThe PR updates the ChangesNetwork Policy & Validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 8/10 reviews remaining, refill in 10 minutes and 2 seconds. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
nemoclaw-blueprint/policies/presets/local-inference.yaml (1)
8-53: Run targeted network-policy e2e before merge.Since this directly changes sandbox egress/SSRF behavior, running the focused network-policy E2E job is a good safety check for runtime environments.
As per coding guidelines: "E2E test recommendation: network-policy-e2e — deny-by-default, whitelist, hot-reload, SSRF".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw-blueprint/policies/presets/local-inference.yaml` around lines 8 - 53, Run the targeted network-policy e2e job for the network_policies.local_inference changes before merging; execute the "network-policy-e2e" suite and validate deny-by-default behavior, whitelist enforcement for allowed_ips on each endpoints entry (host.openshell.internal ports 11434, 11435, 8000), SSRF protections (resolved address allowlisting), and policy hot-reload behavior after applying this preset; report any failing checks and fix policy rules in the local_inference block if egress, SSRF allowlisting, or hot-reload tests fail.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@nemoclaw-blueprint/policies/presets/local-inference.yaml`:
- Around line 8-53: Run the targeted network-policy e2e job for the
network_policies.local_inference changes before merging; execute the
"network-policy-e2e" suite and validate deny-by-default behavior, whitelist
enforcement for allowed_ips on each endpoints entry (host.openshell.internal
ports 11434, 11435, 8000), SSRF protections (resolved address allowlisting), and
policy hot-reload behavior after applying this preset; report any failing checks
and fix policy rules in the local_inference block if egress, SSRF allowlisting,
or hot-reload tests fail.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: be127d6f-b232-4910-95a4-35a1f9c10491
📒 Files selected for processing (2)
nemoclaw-blueprint/policies/presets/local-inference.yamltest/policies.test.ts
f0a108c to
478cbad
Compare
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
478cbad to
e0b1523
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/policies.test.ts`:
- Line 17: Replace the CommonJS-style require used to load YAML with an ESM
import: remove the requireForTest("yaml") call that assigns to the YAML variable
and instead import the package using ESM syntax (e.g., import YAML from 'yaml')
at the top of the test file so the test uses the package's native ESM export;
update any references to the YAML symbol unchanged.
🪄 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: 38f54001-a66a-4509-a3a5-5585e6a80668
📒 Files selected for processing (2)
nemoclaw-blueprint/policies/presets/local-inference.yamltest/policies.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- nemoclaw-blueprint/policies/presets/local-inference.yaml
|
|
||
| const requireForTest = createRequire(import.meta.url); | ||
| const readline = requireForTest("node:readline") as typeof import("node:readline"); | ||
| const YAML = requireForTest("yaml"); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does the yamlnpm package support ESM usage asimport YAML from "yaml"withYAML.parse(...) in TypeScript projects?
💡 Result:
Yes. The yaml npm package supports ESM-style default import in TypeScript projects, e.g. import YAML from 'yaml', and then calling YAML.parse(...). Evidence: the package’s official docs show import YAML from 'yaml' followed by YAML.parse('3.14159') (and similar examples).
Citations:
🏁 Script executed:
# Check the actual content of test/policies.test.ts, focusing on imports and the yaml usage
head -30 test/policies.test.ts | cat -nRepository: NVIDIA/NemoClaw
Length of output: 1933
🏁 Script executed:
# Check if there are other uses of yaml in the file
rg "yaml|YAML" test/policies.test.ts -nRepository: NVIDIA/NemoClaw
Length of output: 1933
🏁 Script executed:
# Check the full import section to understand the file structure
grep -n "^import\|^const\|^require\|requireForTest" test/policies.test.ts | head -20Repository: NVIDIA/NemoClaw
Length of output: 1207
Use an ESM import for the yaml package in this test file.
Line 17 introduces requireForTest("yaml"), which conflicts with the test ESM-import requirement. The yaml npm package natively supports ESM import syntax.
Suggested change
import { spawnSync } from "node:child_process";
+import YAML from "yaml";
import policies from "../dist/lib/policies";
import { execTimeout } from "./helpers/timeouts";
@@
-const YAML = requireForTest("yaml");📝 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 YAML = requireForTest("yaml"); | |
| import { spawnSync } from "node:child_process"; | |
| import YAML from "yaml"; | |
| import policies from "../dist/lib/policies"; | |
| import { execTimeout } from "./helpers/timeouts"; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/policies.test.ts` at line 17, Replace the CommonJS-style require used to
load YAML with an ESM import: remove the requireForTest("yaml") call that
assigns to the YAML variable and instead import the package using ESM syntax
(e.g., import YAML from 'yaml') at the top of the test file so the test uses the
package's native ESM export; update any references to the YAML symbol unchanged.
Selective E2E Results — ✅ All requested jobs passedRun: 25334892463
|
wscurran
added
area: local-models
3 participants
Summary
local-inferencepreset endpoints for Ollama, the auth proxy, and vLLMhost.openshell.internalallowed_ipsentries silentlyRoot cause
PR #2295 added the missing binaries, but OpenShell v0.0.36 performs a second SSRF check after the policy endpoint/binary match.
host.openshell.internalresolves to a Docker/Colima/WSL host-gateway address, which is private by design. Withoutallowed_ips, OpenShell still returns403 ssrf_deniedforhost.openshell.internal:11434and:11435even whenlocal-inferenceappears active.Live OpenShell validation
Tested with OpenShell
0.0.36on dedicated gatewayissue2199test. Host services were reachable before sandbox testing:127.0.0.1:11434/api/tags->200127.0.0.1:11435/api/tags->200Using the
origin/mainlocal-inference policy reproduced the reopened issue:host.openshell.internal:11434/api/tags->403,{"error":"ssrf_denied"}host.openshell.internal:11435/api/tags->403,{"error":"ssrf_denied"}Using this PR's local-inference policy fixed the same calls:
host.openshell.internal:11434/api/tags->200host.openshell.internal:11435/api/tags->200Other validation
npm ci --ignore-scriptsnpm run build:clinpx vitest run test/policies.test.ts test/validate-blueprint.test.tsgit diff --checkCloses #2199
Summary by CodeRabbit
New Features
Tests