fix(policy): load shared network policy schema#7533
Conversation
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
📝 WalkthroughWalkthroughThe package now publishes ChangesNetwork policy schema integration
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
apurvvkumaria
added
v0.0.96
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit aefb9ce in the TypeScript / code-coverage/cliThe overall coverage in commit aefb9ce in the Show a code coverage summary of the most impacted files.
Updated |
|
🌿 Preview your docs: https://nvidia-preview-pr-7533.docs.buildwithfern.com/nemoclaw |
![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
🤖 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/policy/sandbox-policy-validation.ts`:
- Around line 30-32: Move the NETWORK_POLICY_SCHEMA_PATH filesystem read out of
the policy validation code and into an adapter under src/lib/adapters/**, then
inject or call that loader from the policy validation flow for both schema
reads. Update the relevant validation symbols around parsedNetworkPolicySchema
to use the adapter boundary, preserving existing parsing and validation behavior
while allowing tests to provide fakes.
🪄 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: 7b15caa6-9058-4d0c-9965-16fd832ca9d1
📒 Files selected for processing (3)
package.jsonsrc/lib/policy/sandbox-policy-validation.tstest/package-contract/openshell-policy-boundary.test.ts
| const parsedNetworkPolicySchema: unknown = JSON.parse( | ||
| fs.readFileSync(NETWORK_POLICY_SCHEMA_PATH, "utf-8"), | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Keep schema loading behind the adapter boundary.
The newly added fs.readFileSync in src/lib/policy/** couples policy validation directly to the host filesystem. Route both schema reads through an adapter or injected loader so this policy code remains testable with fakes.
As per path instructions, host-boundary work (filesystem/process/network/Docker/OpenShell) must be isolated in src/lib/adapters/** so policy code stays testable with injected fakes.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 30-30: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(NETWORK_POLICY_SCHEMA_PATH, "utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
🤖 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/policy/sandbox-policy-validation.ts` around lines 30 - 32, Move the
NETWORK_POLICY_SCHEMA_PATH filesystem read out of the policy validation code and
into an adapter under src/lib/adapters/**, then inject or call that loader from
the policy validation flow for both schema reads. Update the relevant validation
symbols around parsedNetworkPolicySchema to use the adapter boundary, preserving
existing parsing and validation behavior while allowing tests to provide fakes.
Source: Path instructions
PR Review Advisor — InformationalAdvisor assessment: Informational / high confidence Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. 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. |
|
Closing in favor of #7531. #7531 is the earlier equivalent three-file fix from Ho Lim, the original #6877 author. It covers the same acceptance criteria:
This branch adds no unique behavior or regression coverage, so preserving the original author's credit is the correct choice. The suggestion to move both existing schema filesystem reads into a new adapter is not adopted: this policy module already owns the sandbox-schema read, and that unrelated refactor would enlarge a tactical current-main repair. Validation performed on the equivalent fix shape: 318/318 package contracts, 29/29 policy persistence tests, 7/7 OpenShell policy package contracts, and the normal signed commit hooks. |
<!-- markdownlint-disable MD041 --> ## Summary Restore runtime sandbox-policy validation after #6877 extracted network policy definitions into an external schema. The runtime validator now registers that shared schema before compiling the sandbox schema, and the published package ships both files. ## Changes - Load and register `network-policy.schema.json` before compiling the sandbox policy schema. - Include the shared schema in the npm package. - Extend the out-of-tree package contract to prove the referenced schema is shipped and runtime validation succeeds. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates <!-- Check one tests line and one docs line. Check other lines when applicable. Add every requested justification or approval reference. --> - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: This restores the existing schema-validation behavior and package contents. It does not change policy syntax, CLI behavior, configuration, or a user procedure. - [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: Maintainer comparison confirmed the runtime registers only the trusted schema shipped by NemoClaw. Policy semantics, rejected input handling, and bounded error disclosure remain unchanged. Independent PR approval remains required before merge. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review <!-- Required for code and documentation changes after the changes and applicable validation are complete. Keep one review checkbox and one instance of each visible or hidden field. For Evidence, list changed documentation paths. For documentation-only changes, also state that the writing rules and documentation style were reviewed. For other results, explain why no documentation change is needed or why the review is blocked. For Agent, use a consistent product and surface name, such as Codex Desktop, Codex CLI, Claude Code, or Cursor. After committing all review changes, put `git rev-parse --short HEAD` and `git rev-parse --short HEAD:AGENTS.md` in the hidden metadata below. Rerun the review and refresh that metadata after any new commit. This receipt is advisory during the data-collection pilot. --> - [x] Documentation writer subagent reviewed the completed changes - Result: `no-docs-needed` - Evidence: The writer reviewed the live body and exact three-file diff against `WRITING.md` and `docs/CONTRIBUTING.md`. The fix restores existing policy validation and package contents without changing the schema, configuration, supported behavior, or user workflow. - Agent: Codex Desktop <!-- docs-review-head-sha: d133ec1 --> <!-- docs-review-agents-blob-sha: 9c9b36d --> ## DGX Station Hardware Evidence <!-- Required only when scripts/prepare-dgx-station-host.sh changes. Maintainers must review the linked evidence before approving or merging. This is human-reviewed evidence, not authenticated hardware provenance. Exceptional bypasses use existing repository governance and must be documented on the PR. --> - [ ] Tested on DGX Station - Tested commit: - Station profile/scenario: - Result: - Supporting evidence: ## Verification <!-- Check each applicable item only when supported by the requested evidence. Run targeted tests once per relevant change set and rerun after later edits or hook autofixes that can affect the tested behavior. Do not rerun hook-covered checks. --> - [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 check:diff` passed 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 installed package validator contract passes 1/1, and the focused policy suites pass 56/56. - [ ] 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 to this focused three-file integration repair. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) Follow-up to #6877. Supersedes duplicate #7533 and preserves Ho Lim's original-author credit. --- <!-- DCO sign-off is required in this PR description, and every commit must appear as Verified in GitHub. Run: git config user.name && git config user.email --> Signed-off-by: Ho Lim <subhoya@gmail.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved sandbox policy validation by incorporating network policy schema checks. * Added clearer handling when required policy schemas are unavailable or validation cannot be initialized. * **Chores** * Ensured the network policy schema is included in published packages. * Expanded package verification to confirm the schema is available after installation. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Ho Lim <subhoya@gmail.com>
1 participant
Summary
Restore sandbox policy validation after the shared network policy schema was extracted in #6877. The runtime now loads the referenced schema, and the npm package ships the schema that the validator consumes.
Changes
schemas/network-policy.schema.jsonwith the runtime AJV instance before compiling the sandbox policy schema.Type of Change
Quality Gates
Documentation Writer Review
no-docs-neededpackage.json,src/lib/policy/sandbox-policy-validation.ts, andtest/package-contract/openshell-policy-boundary.test.tsagainstWRITING.mdanddocs/CONTRIBUTING.md. The fix restores the intended existing policy validation and does not change user-facing behavior.DGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run test:packagepasses 318/318 across 22 files;npm run build:clipasses.npm run docsbuilds without warnings (doc changes only)Follow-up to #6877. Unblocks #7529 and other PRs synchronized to current
main.Signed-off-by: Apurv Kumaria akumaria@nvidia.com
Summary by CodeRabbit
Bug Fixes
Tests