Skip to content

fix: add binaries to pypi/npm policy presets to prevent 403 - #418

Closed
alphadl wants to merge 1 commit into
NVIDIA:mainfrom
alphadl:fix/pypi-npm-binaries
Closed

fix: add binaries to pypi/npm policy presets to prevent 403#418
alphadl wants to merge 1 commit into
NVIDIA:mainfrom
alphadl:fix/pypi-npm-binaries

Conversation

@alphadl

@alphadl alphadl commented Mar 19, 2026

Copy link
Copy Markdown

Summary

Test plan

  • node --test test/policies.test.js

Docs

  • No documentation changes.

Summary by CodeRabbit

Release Notes

  • New Features

    • Network policy presets for npm and yarn now include executable path allowlists.
    • Network policy presets for Python and pip now include executable path allowlists, supporting common installation paths.
  • Tests

    • Added tests to validate that executable path allowlists are properly defined in package manager policy presets.

@coderabbitai

coderabbitai Bot commented Mar 19, 2026

Copy link
Copy Markdown
Contributor
📝 Walkthrough

Walkthrough

This PR adds binaries sections to the npm and pypi package manager policy presets, explicitly specifying the filesystem paths for Node.js, npm, yarn, Python, and pip executables. A test is added to validate these binaries are present in the presets.

Changes

Cohort / File(s) Summary
Package Manager Policy Presets
nemoclaw-blueprint/policies/presets/npm.yaml, nemoclaw-blueprint/policies/presets/pypi.yaml
Added binaries allowlists specifying executable paths: npm preset includes /usr/local/bin/{node,npm,yarn}; pypi preset includes /usr/local/bin/{python,python3,pip} and /usr/bin/{python3,pip}.
Preset Validation Test
test/policies.test.js
Added test to validate that pypi and npm presets include a binaries: section with at least one entry matching the { path: ... } pattern.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 Hop, hop, through the bundled code we go,
Binaries blessed in paths we now bestow,
npm and pip may dance both left and right,
Inside the sandbox—finally, a perfect sight! 📦✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The PR adds binaries to pypi/npm presets and includes a test, but does not fully address acceptance criteria requiring pip/npm install to succeed or validating the access model matches GitHub's approach. Verify that pip install and npm install now succeed with these changes. Confirm the binaries approach matches the proven GitHub access model or document the complete solution.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: adding binaries to pypi/npm presets to fix 403 errors. It is concise, specific, and directly related to the changeset.
Out of Scope Changes check ✅ Passed All changes are directly related to adding binaries to pypi/npm presets and adding validation tests. No out-of-scope changes detected.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
📝 Coding Plan
  • Generate coding plan for human review comments

Comment @coderabbitai help to get the list of available commands and usage tips.

Tip

You can customize the tone of the review comments and chat replies.

Configure the tone_instructions setting to customize the tone of the review comments and chat replies. For example, you can set the tone to Act like a strict teacher, Act like a pirate and more.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
nemoclaw-blueprint/policies/presets/npm.yaml (1)

27-30: Consider adding /usr/bin/ paths for broader compatibility.

The pypi preset includes both /usr/local/bin/ and /usr/bin/ paths, but npm only uses /usr/local/bin/. On many Linux systems (Debian/Ubuntu packages, some container images), Node.js is installed at /usr/bin/node. This could leave some users still encountering 403 errors.

♻️ Proposed fix to add common system paths
     binaries:
       - { path: /usr/local/bin/node }
       - { path: /usr/local/bin/npm }
       - { path: /usr/local/bin/yarn }
+      - { path: /usr/bin/node }
+      - { path: /usr/bin/npm }
+      - { path: /usr/bin/yarn }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@nemoclaw-blueprint/policies/presets/npm.yaml` around lines 27 - 30, The
binaries list currently only includes /usr/local/bin/* which misses common
system installs; update the binaries block (the entries under "binaries") to
also include /usr/bin/node, /usr/bin/npm, and /usr/bin/yarn so Node/npm/Yarn
installed in /usr/bin are recognized; keep the existing /usr/local/bin entries
and append the three /usr/bin/* entries to the list.
nemoclaw-blueprint/policies/presets/pypi.yaml (1)

28-33: Consider adding pip3 paths.

Many systems use pip3 as the primary pip executable (especially where Python 2 and 3 coexist). Adding these paths would improve coverage:

♻️ Proposed addition for pip3
     binaries:
       - { path: /usr/local/bin/python }
       - { path: /usr/local/bin/python3 }
       - { path: /usr/local/bin/pip }
+      - { path: /usr/local/bin/pip3 }
       - { path: /usr/bin/python3 }
       - { path: /usr/bin/pip }
+      - { path: /usr/bin/pip3 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@nemoclaw-blueprint/policies/presets/pypi.yaml` around lines 28 - 33, The
binaries list currently includes pip and python paths but omits common pip3
executables; update the binaries section (the "binaries" list) to also include
entries for pip3 (for example paths like /usr/local/bin/pip3 and /usr/bin/pip3)
so systems that expose pip as pip3 are covered—add those new path entries
alongside the existing { path: /usr/local/bin/pip } and { path: /usr/bin/pip }
entries.
🤖 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/npm.yaml`:
- Around line 27-30: The binaries list currently only includes /usr/local/bin/*
which misses common system installs; update the binaries block (the entries
under "binaries") to also include /usr/bin/node, /usr/bin/npm, and /usr/bin/yarn
so Node/npm/Yarn installed in /usr/bin are recognized; keep the existing
/usr/local/bin entries and append the three /usr/bin/* entries to the list.

In `@nemoclaw-blueprint/policies/presets/pypi.yaml`:
- Around line 28-33: The binaries list currently includes pip and python paths
but omits common pip3 executables; update the binaries section (the "binaries"
list) to also include entries for pip3 (for example paths like
/usr/local/bin/pip3 and /usr/bin/pip3) so systems that expose pip as pip3 are
covered—add those new path entries alongside the existing { path:
/usr/local/bin/pip } and { path: /usr/bin/pip } entries.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: df9c1410-0571-4b30-9a7d-c46c6b7fd4df

📥 Commits

Reviewing files that changed from the base of the PR and between 20a63d0 and 30329d1.

📒 Files selected for processing (3)
  • nemoclaw-blueprint/policies/presets/npm.yaml
  • nemoclaw-blueprint/policies/presets/pypi.yaml
  • test/policies.test.js

@cv

cv commented Mar 24, 2026

Copy link
Copy Markdown
Collaborator

Closing — fully covered by #356 (merged to main) which restructured npm/pypi presets with access: full and added binaries, and #645 which added binaries restrictions to all remaining entries. This PR also uses tls: terminate which #356 specifically replaced with access: full to fix CONNECT tunneling for package managers.

@cv cv closed this Mar 24, 2026
mafueee pushed a commit to mafueee/NemoClaw that referenced this pull request Mar 28, 2026
…NVIDIA#418)

The trigger-wheel-publish job was downloading the wheel artifact onto a
persistent self-hosted runner just to glob the filenames. Since
download-artifact does not clean the destination directory, stale .whl
files from every previous run accumulated in release/ and were all sent
to GitLab as WHEEL_FILENAMES.

Confirmed on the runner: /home/ubuntu/actions-runner/_work/OpenShell/
OpenShell/release/ contained 120+ wheels spanning versions 0.0.5
through 0.0.10.

Fix: capture wheel filenames as a job output in build-python-wheels and
pass them directly to trigger-wheel-publish. The trigger job no longer
downloads the artifact at all — it only needs the filenames to construct
GitHub release asset URLs for GitLab/Kitmaker.
@wscurran wscurran added area: ci CI workflows, checks, release automation, or GitHub Actions area: install Install, setup, prerequisites, or uninstall flow area: onboarding Onboarding FSM, provider setup, sandbox launch, or first-run flow bug-fix PR fixes a bug or regression chore Build, CI, dependency, or tooling maintenance and removed priority: medium chore Build, CI, dependency, or tooling maintenance labels Jun 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: ci CI workflows, checks, release automation, or GitHub Actions area: install Install, setup, prerequisites, or uninstall flow area: onboarding Onboarding FSM, provider setup, sandbox launch, or first-run flow bug-fix PR fixes a bug or regression

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix broken pypi and npm policy presets so package managers work inside the sandbox

5 participants