feat: add github network policy preset#146
Conversation
|
Thanks for adding the github network policy preset, this will be helpful for users who need to access GitHub APIs from within their sandboxes. |
|
Hey @Ryuketsukami, thanks for putting together the GitHub network policy preset — this is a really useful addition! Just a heads-up: the repo has been moving pretty fast lately, and we've landed a bunch of new features plus CI checks since this PR was opened. Would you mind rebasing onto the latest main? That way we can give it a proper review with everything up to date. Appreciate your contribution! |
📝 WalkthroughWalkthroughAdds a GitHub policy preset, documentation for "Policy Presets", and test updates. The preset defines network policies for GitHub endpoints ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
docs/reference/network-policies.md (1)
128-129: One sentence per line.Per coding guidelines, each sentence should be on its own line to make diffs readable. Line 129 contains two sentences.
📝 Suggested fix
Presets extend the baseline network policy with additional endpoints for common services. -Apply a preset with `nemoclaw <name> policy-add` and list available presets with `nemoclaw <name> policy-list`. +Apply a preset with `nemoclaw <name> policy-add`. +List available presets with `nemoclaw <name> policy-list`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/network-policies.md` around lines 128 - 129, The paragraph "Presets extend the baseline network policy with additional endpoints for common services. Apply a preset with `nemoclaw <name> policy-add` and list available presets with `nemoclaw <name> policy-list`." contains two sentences on one line; split them so each sentence is on its own line (e.g., keep the first sentence as its own line starting with "Presets extend..." and move the second sentence to the next line starting with "Apply a preset..."), preserving the existing wording and backticks for the commands.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nemoclaw-blueprint/policies/presets/github.yaml`:
- Around line 23-30: The github.com entry in the preset uses tls: terminate
which conflicts with the baseline's openclaw-sandbox.yaml that expects access:
full; update the github.com block (host: github.com) to use access: full instead
of tls: terminate (or remove the github.com block entirely to inherit the
baseline), leaving api.github.com and other REST-only entries with tls:
terminate unchanged so REST API endpoints keep their current behavior.
In `@test/policies.test.js`:
- Around line 10-12: The test is using Node's assert but that symbol isn't
imported; update the "returns all 10 presets" test to use vitest's expect API
instead of assert — call policies.listPresets() and replace
assert.equal(presets.length, 10) with expect(presets).toHaveLength(10) (or
expect(presets.length).toBe(10)); ensure no leftover assert usage remains in
this test.
- Around line 24-25: The test currently calls assert.deepEqual(names, expected)
but assert is not imported; change the assertion to use the test framework's
expect API by replacing assert.deepEqual(names, expected) with
expect(names).toEqual(expected) and remove any unused assert import or require;
ensure the variables names and expected remain unchanged so the test compares
the same arrays.
- Around line 62-69: The test "extracts hosts from github preset" uses assert.ok
which is undefined in our environment; update the assertions to use Jest's
expect and toContain for clarity: call policies.loadPreset and
policies.getPresetEndpoints as before, then replace each
assert.ok(hosts.includes("...")) with expect(hosts).toContain("api.github.com"),
expect(hosts).toContain("github.com"),
expect(hosts).toContain("raw.githubusercontent.com"), and
expect(hosts).toContain("uploads.github.com"); also remove or stop using the
unused assert import if present.
---
Nitpick comments:
In `@docs/reference/network-policies.md`:
- Around line 128-129: The paragraph "Presets extend the baseline network policy
with additional endpoints for common services. Apply a preset with `nemoclaw
<name> policy-add` and list available presets with `nemoclaw <name>
policy-list`." contains two sentences on one line; split them so each sentence
is on its own line (e.g., keep the first sentence as its own line starting with
"Presets extend..." and move the second sentence to the next line starting with
"Apply a preset..."), preserving the existing wording and backticks for the
commands.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5b5ff5de-8a32-4825-b35f-74e7b8fe89a0
📒 Files selected for processing (3)
docs/reference/network-policies.mdnemoclaw-blueprint/policies/presets/github.yamltest/policies.test.js
Signed-off-by: ryuketsukami <ryuketsukami@gmail.com>
b4b7cb6 to
f621396
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
test/policies.test.js (3)
24-25:⚠️ Potential issue | 🔴 Critical
assert.deepEqualis undefined — useexpectinstead.Same issue:
assertis not imported. The preset list update to include"github"is correct, but the assertion method will fail.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/policies.test.js` around lines 24 - 25, The test currently calls assert.deepEqual which fails because assert isn't imported; replace that assertion with the test framework's expect style (e.g., assert -> expect) by asserting that the names variable equals the expected array (use expect(names).to.deep.equal(expected) or equivalently expect(names).to.eql(expected)), and remove any leftover/assert import references so the test uses expect exclusively (refer to the names and expected variables in the test/policies.test.js assertion).
62-69:⚠️ Potential issue | 🔴 Critical
assert.okis undefined — useexpectfor consistency.The new test for the github preset correctly validates all four expected hosts, but uses
assert.okwhich is not imported. Use vitest'sexpectAPI consistently with other tests in this file (e.g., lines 47-54 useexpect(...).toBeTruthy()).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/policies.test.js` around lines 62 - 69, The test "extracts hosts from github preset" uses undefined assert.ok; update it to use vitest's expect API for consistency by replacing the four assert.ok(hosts.includes(...)) calls with expect(hosts).toContain(...) (or expect(hosts.includes("...")).toBeTruthy()) so the assertions call expect and validate the results of policies.loadPreset and policies.getPresetEndpoints.
10-12:⚠️ Potential issue | 🔴 Critical
assertis not imported — tests will fail at runtime.The test uses
assert.equalbut onlyexpectis imported from vitest on line 4. This will throwReferenceError: assert is not defined.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/policies.test.js` around lines 10 - 12, The test calls assert.equal on the result of policies.listPresets but never imports assert, causing a ReferenceError; fix by importing Node's assert at the top of test/policies.test.js (e.g., add "import assert from 'assert'") or alternatively replace assert.equal(presets.length, 10) with a vitest assertion like expect(presets).toHaveLength(10) so the test uses the already-imported expect.
🧹 Nitpick comments (1)
docs/reference/network-policies.md (1)
128-129: Multiple sentences on one line — use one sentence per line.Line 129 contains two sentences. Per the style guide, each sentence should be on its own line to make diffs more readable.
Presets extend the baseline network policy with additional endpoints for common services. -Apply a preset with `nemoclaw <name> policy-add` and list available presets with `nemoclaw <name> policy-list`. +Apply a preset with `nemoclaw <name> policy-add`. +List available presets with `nemoclaw <name> policy-list`.As per coding guidelines: "One sentence per line in source (makes diffs readable). Flag paragraphs where multiple sentences appear on the same line."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/network-policies.md` around lines 128 - 129, The sentence pair in the paragraph describing presets is on one line; split them so each sentence is its own line (e.g., break "Presets extend the baseline network policy with additional endpoints for common services." and "Apply a preset with `nemoclaw <name> policy-add` and list available presets with `nemoclaw <name> policy-list`." into separate lines) to follow the "one sentence per line" style guide.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/policies.test.js`:
- Around line 24-25: The test currently calls assert.deepEqual which fails
because assert isn't imported; replace that assertion with the test framework's
expect style (e.g., assert -> expect) by asserting that the names variable
equals the expected array (use expect(names).to.deep.equal(expected) or
equivalently expect(names).to.eql(expected)), and remove any leftover/assert
import references so the test uses expect exclusively (refer to the names and
expected variables in the test/policies.test.js assertion).
- Around line 62-69: The test "extracts hosts from github preset" uses undefined
assert.ok; update it to use vitest's expect API for consistency by replacing the
four assert.ok(hosts.includes(...)) calls with expect(hosts).toContain(...) (or
expect(hosts.includes("...")).toBeTruthy()) so the assertions call expect and
validate the results of policies.loadPreset and policies.getPresetEndpoints.
- Around line 10-12: The test calls assert.equal on the result of
policies.listPresets but never imports assert, causing a ReferenceError; fix by
importing Node's assert at the top of test/policies.test.js (e.g., add "import
assert from 'assert'") or alternatively replace assert.equal(presets.length, 10)
with a vitest assertion like expect(presets).toHaveLength(10) so the test uses
the already-imported expect.
---
Nitpick comments:
In `@docs/reference/network-policies.md`:
- Around line 128-129: The sentence pair in the paragraph describing presets is
on one line; split them so each sentence is its own line (e.g., break "Presets
extend the baseline network policy with additional endpoints for common
services." and "Apply a preset with `nemoclaw <name> policy-add` and list
available presets with `nemoclaw <name> policy-list`." into separate lines) to
follow the "one sentence per line" style guide.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aab3a41d-931d-4be3-a2a7-350bfa81e435
📒 Files selected for processing (3)
docs/reference/network-policies.mdnemoclaw-blueprint/policies/presets/github.yamltest/policies.test.js
✅ Files skipped from review due to trivial changes (1)
- nemoclaw-blueprint/policies/presets/github.yaml
|
Hey @cv, done! I've rebased onto the latest main — the branch is now clean with just my commit on top. Sorry for the mess, not used to doing rebases! if there is an issue with anything let me know! |
…move implicit catch-all (NVIDIA#146) Remove multi-route CRUD system and replace with single managed cluster route (inference.local). Key changes: - Remove inference route CRUD RPCs and CLI commands - Remove InspectForInference OPA action; policy is binary allow/deny - Introduce AuthHeader enum and InferenceProviderProfile in navigator-core - Router is now provider-agnostic: auth style carried on ResolvedRoute - Replace InferenceRouteSpec with ClusterInferenceConfig (2 fields vs 8) - Rename proto: routing_hint->name, SandboxResolvedRoute->ResolvedRoute, GetSandboxInferenceBundle->GetInferenceBundle, drop sandbox_id param - Rename RouteConfig.route -> RouteConfig.name; use inference.local - Add 'nemoclaw cluster inference update' for partial config changes - Delete stale navigator.inference.v1.rs checked-in proto file - Update architecture docs, agent skills, and CLI reference Closes NVIDIA#133
NVIDIA#158) * feat(proxy): support plain HTTP forward proxy for private IP endpoints Add forward proxy mode to the sandbox proxy so that standard HTTP libraries (httpx, requests, etc.) work with HTTP_PROXY for plain HTTP calls to private IP endpoints. Previously, non-CONNECT methods were unconditionally rejected with 403. The forward proxy path requires all three conditions to be met: - OPA policy explicitly allows the destination - The matched endpoint has allowed_ips configured - All resolved IPs are RFC 1918 private This ensures plain HTTP never reaches the public internet while enabling seamless access to internal services without custom CONNECT tunnel code. Implementation: - parse_proxy_uri(): parses absolute-form URIs into components - rewrite_forward_request(): rewrites to origin-form, strips hop-by-hop headers, adds Via and Connection: close - handle_forward_proxy(): full handler with OPA eval, SSRF checks, private-IP gate, upstream connect, and bidirectional relay - Updated dispatch in handle_tcp_connection to route non-CONNECT methods Includes 14 unit tests and 6 E2E tests (FWD-1 through FWD-6). CONNECT path remains completely untouched. Closes NVIDIA#155 * fix(proxy): remove InspectForInference match arm removed by NVIDIA#146 The inference routing simplification in NVIDIA#146 reduced NetworkAction to Allow/Deny, removing InspectForInference. Drop the dead match arm from handle_forward_proxy. * fix(sandbox): restore BestEffort as default Landlock compatibility The Landlock V2 upgrade in NVIDIA#151 changed the default from BestEffort to HardRequirement. This causes all proxy-mode sandboxes to crash with Permission denied when the policy omits the landlock field, because the child process gets locked to only /etc/navigator-tls and /sandbox. Restore BestEffort as the default so policies without an explicit landlock field degrade gracefully. Fixes NVIDIA#161 * fix(sandbox): inject baseline filesystem paths for proxy-mode sandboxes Proxy-mode sandboxes need baseline filesystem paths (/usr, /lib, /etc, /app, /var/log read-only; /sandbox, /tmp read-write) for the child process to function under Landlock. Without these, the child can't exec binaries, resolve DNS, or load shared libraries. The supervisor now enriches the policy with these baseline paths at startup, covering both standalone (file) and gateway (gRPC) modes. For gateway mode, the enriched policy is synced back so users see the effective policy via 'nemoclaw sandbox get'. The gateway validation is relaxed to allow additive filesystem changes (new paths can be added, existing paths cannot be removed) to support the supervisor's enrichment sync-back. Includes 2 E2E tests: BFS-1 (missing filesystem_policy) and BFS-2 (incomplete filesystem_policy). Fixes NVIDIA#161 * fix(e2e): update assertion for relaxed filesystem validation message --------- Co-authored-by: John Myers <johntmyers@users.noreply.github.com>
Adds a
githubnetwork policy preset coveringapi.github.com(full REST CRUD),github.com,raw.githubusercontent.com, anduploads.github.com.GitHub API access is the most commonly requested missing preset for a coding assistant
running in a sandboxed environment — a natural addition alongside the existing
huggingface,npm,pypi, anddockerpresets.Updates the preset count and endpoint assertions in
test/policies.test.jsand addsgithubto the presets table indocs/reference/network-policies.md.Summary by CodeRabbit
Documentation
New Features
Tests