fix(install): skip shim when npm prefix equals shim dir#1718
Merged
cv merged 1 commit intoNVIDIA:mainfrom Apr 10, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
When npm_config_prefix=$HOME/.local (as in the brev E2E environment), npm link places the nemoclaw binary at $HOME/.local/bin/nemoclaw — the same path that ensure_nemoclaw_shim() targets for the wrapper shim. Without this guard the function overwrites the real binary with a shell script that exec's itself, causing an infinite loop. is_real_nemoclaw_cli() (added in NVIDIA#1606) correctly detected the broken binary but misidentified it as the npm placeholder package and ran npm uninstall -g, leaving no binary at all. Add a -ef guard: if cli_path and shim_path refer to the same file, the binary is already in the right place — return early without writing a self-referential wrapper. Fixes NVIDIA#1606 regression caught by E2E brev (run 24216985741). Signed-off-by: Dongni Yang <dongniy@nvidia.com> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Dongni-Yang
force-pushed
the
fix/install-shim-self-reference
branch
from
ce8c666 to
bcc9bc2
Compare
|
❌ Brev E2E (full): FAILED on branch |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
test/validate-config-schemas.test.ts (1)
74-97: Add a positive test forprotocol: rest+access(withoutrules).Current negatives only assert the strict
rulesrequirement. Once the schema condition is fixed, you’ll want a pass-case to lock that intended behavior.Also applies to: 168-179
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/validate-config-schemas.test.ts` around lines 74 - 97, Add a positive test case in test/validate-config-schemas.test.ts that verifies a policy addition with protocol: "rest" can pass when using an "access" property instead of "rules" (mirror the existing negative test "rejects blueprint policyAddition endpoint with protocol rest but no rules" but assert validate(...) === true). Target the same structure used in the bad case (components.policy.additions.my_service.endpoints) but include an "access" object (or array) appropriate to the schema (the same shape you expect to allow) and call validate(better) expecting true; also add a similar positive case corresponding to the other negative at lines 168-179 so both negative and positive behaviors are covered for the protocol "rest" condition.schemas/sandbox-policy.schema.json (1)
14-90: Consider closing nested policy objects to catch config typos earlier.
filesystem_policy,landlock,process,networkPolicyEntry, andendpointcurrently allow unknown keys. In practice, this can let misspelled fields pass schema validation silently.Suggested patch
"filesystem_policy": { "type": "object", + "additionalProperties": false, "properties": { @@ "landlock": { "type": "object", + "additionalProperties": false, "properties": { @@ "process": { "type": "object", + "additionalProperties": false, "properties": { @@ "networkPolicyEntry": { "type": "object", "required": ["name", "endpoints"], + "additionalProperties": false, "properties": { @@ "endpoint": { "type": "object", "required": ["host", "port"], + "additionalProperties": false, "properties": {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@schemas/sandbox-policy.schema.json` around lines 14 - 90, The listed schema objects allow unknown keys; update each object definition (filesystem_policy, landlock, process, networkPolicyEntry, endpoint) to include "additionalProperties": false so typos are rejected, and ensure any nested arrays/refs still validate (e.g., network_policies uses networkPolicyEntry, endpoint is referenced by networkPolicyEntry) so no other constraints are broken; add the additionalProperties flag inside each object's "properties" object block to disallow extraneous fields.scripts/validate-configs.ts (1)
94-114: Reject unexpected CLI args instead of silently running default validation.At Line [113], any unrecognized args (e.g., typoed flags) currently fall through to
discoverTargets(). That can mask invocation mistakes and report misleading success.Suggested patch
function main(): void { const args = process.argv.slice(2); @@ const hasFileFlag = args.indexOf("--file") !== -1; const hasSchemaFlag = args.indexOf("--schema") !== -1; + if (!hasFileFlag && !hasSchemaFlag && args.length > 0) { + console.error("Usage: validate-configs.ts --file <config> --schema <schema>"); + process.exitCode = 1; + return; + } if (hasFileFlag !== hasSchemaFlag) { console.error("Usage: validate-configs.ts --file <config> --schema <schema>"); process.exitCode = 1; return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/validate-configs.ts` around lines 94 - 114, The CLI currently accepts only --file and --schema but silently ignores any other flags and falls back to discoverTargets(); update the argument handling in validate-configs.ts to reject unexpected flags: after parsing args and computing hasFileFlag/hasSchemaFlag (and before falling back to discoverTargets()), scan args for any token starting with "-" that is not "--file" or "--schema" (and not the values immediately following those flags), and if any unrecognized flag is found, print the same usage message and set process.exitCode = 1 then return; ensure this logic references the existing variables and flow using args, fileIdx/schemaIdx, hasFileFlag/hasSchemaFlag, targets, and discoverTargets so valid --file/--schema usage still works but typoed or unknown flags no longer silently run default validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.pre-commit-config.yaml:
- Line 173: The hook's files regex (the value after the files: key — currently
^(nemoclaw-blueprint/.*\.yaml$|nemoclaw/openclaw\.plugin\.json$|schemas/.*\.json$))
only matches .yaml and misses .yml files; update that regex to accept both
extensions (for example replace \.yaml$ with \.(?:yml|yaml)$ or use \.ya?ml$) so
changes to .yml presets/configs will trigger the schema validation pre-commit
hook.
In `@schemas/blueprint.schema.json`:
- Around line 137-141: The JSON Schema currently forces protocol "rest" to
always include "rules"; update the conditional so REST endpoints must include
either "rules" or an access policy instead of mandating "rules" unconditionally.
Concretely, replace the "then": { "required": ["rules"] } with a clause that
requires at least one of ["rules","access"] (e.g., "then": { "anyOf": [ {
"required": ["rules"] }, { "required": ["access"] } ] } ), keeping the existing
"if" that checks "protocol": "rest".
In `@schemas/policy-preset.schema.json`:
- Around line 60-64: The current conditional requires "rules" when protocol is
"rest", which disallows REST presets that use "access"; update the schema
conditional block (the "if" that checks properties.protocol === "rest") so its
"then" allows either "rules" OR "access" — replace the existing then: {
"required": ["rules"] } with a then that uses oneOf: [ { "required": ["rules"]
}, { "required": ["access"] } ] (ensure "access" is validated elsewhere or add
its schema if missing).
---
Nitpick comments:
In `@schemas/sandbox-policy.schema.json`:
- Around line 14-90: The listed schema objects allow unknown keys; update each
object definition (filesystem_policy, landlock, process, networkPolicyEntry,
endpoint) to include "additionalProperties": false so typos are rejected, and
ensure any nested arrays/refs still validate (e.g., network_policies uses
networkPolicyEntry, endpoint is referenced by networkPolicyEntry) so no other
constraints are broken; add the additionalProperties flag inside each object's
"properties" object block to disallow extraneous fields.
In `@scripts/validate-configs.ts`:
- Around line 94-114: The CLI currently accepts only --file and --schema but
silently ignores any other flags and falls back to discoverTargets(); update the
argument handling in validate-configs.ts to reject unexpected flags: after
parsing args and computing hasFileFlag/hasSchemaFlag (and before falling back to
discoverTargets()), scan args for any token starting with "-" that is not
"--file" or "--schema" (and not the values immediately following those flags),
and if any unrecognized flag is found, print the same usage message and set
process.exitCode = 1 then return; ensure this logic references the existing
variables and flow using args, fileIdx/schemaIdx, hasFileFlag/hasSchemaFlag,
targets, and discoverTargets so valid --file/--schema usage still works but
typoed or unknown flags no longer silently run default validation.
In `@test/validate-config-schemas.test.ts`:
- Around line 74-97: Add a positive test case in
test/validate-config-schemas.test.ts that verifies a policy addition with
protocol: "rest" can pass when using an "access" property instead of "rules"
(mirror the existing negative test "rejects blueprint policyAddition endpoint
with protocol rest but no rules" but assert validate(...) === true). Target the
same structure used in the bad case
(components.policy.additions.my_service.endpoints) but include an "access"
object (or array) appropriate to the schema (the same shape you expect to allow)
and call validate(better) expecting true; also add a similar positive case
corresponding to the other negative at lines 168-179 so both negative and
positive behaviors are covered for the protocol "rest" condition.
🪄 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: Pro
Run ID: 1cb3410a-358f-41c8-80ed-d0daf640ad06
📒 Files selected for processing (12)
.github/actions/basic-checks/action.yaml.pre-commit-config.yamlnemoclaw-blueprint/blueprint.yamlpackage.jsonschemas/blueprint.schema.jsonschemas/onboard-config.schema.jsonschemas/openclaw-plugin.schema.jsonschemas/policy-preset.schema.jsonschemas/sandbox-policy.schema.jsonscripts/install.shscripts/validate-configs.tstest/validate-config-schemas.test.ts
cv
approved these changes
Apr 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fix: Add a `-ef` guard before writing the shim. If `cli_path` and `shim_path` refer to the same file, the binary is already in place — return early.
Why #1606 didn't catch this: All local/unit tests use the default npm prefix, which differs from `NEMOCLAW_SHIM_DIR`. The self-reference only occurs when `npm_config_prefix=$HOME/.local` is explicitly exported — exclusive to the brev E2E runner. Before #1606 the broken shim was silently accepted (no verification step existed).
Fixes regression from #1606, caught by E2E brev run 24216985741.
Test plan
Signed-off-by: Dongni Yang dongniy@nvidia.com
🤖 Generated with Claude Code