This repository was archived by the owner on Apr 7, 2026. It is now read-only.
sync: rebase on upstream (2026-03-27) - #8
Merged
Merged
Conversation
* fix: improve gateway lifecycle recovery * docs: fix readme markdown list spacing * fix: tighten gateway lifecycle review follow-ups * fix: simplify tokenized control ui output * fix: restore chat route in control ui urls * refactor: simplify ansi stripping in onboard * fix: shorten control ui url output * fix: move control ui below cli next steps
Fixes NVIDIA#949 Co-authored-by: KJ <kejones@nvidia.com>
* chore: add cyclomatic complexity rule (ratchet from 95) Add ESLint complexity rule to bin/ and scripts/ to prevent new functions from accumulating excessive branching. Starting threshold is 95 (current worst offender: setupNim in onboard.js). Ratchet plan: 95 → 40 → 25 → 15. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * chore: ratchet complexity to 20, suppress existing violations Suppress 6 functions that exceed the threshold with eslint-disable comments so we can start enforcing at 20 instead of 95: - setupNim (95), setupPolicies (41), setupInference (22) in onboard.js - deploy (22), main IIFE (27) in nemoclaw.js - applyPreset (24) in policies.js Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * chore: suppress complexity for 3 missed functions preflight (23), getReconciledSandboxGatewayState (25), sandboxStatus (27) --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…#903) Signed-off-by: peteryuqin <peter.yuqin@gmail.com>
…#913) * chore: add tsconfig.cli.json, root execa, TS coverage ratchet Foundation for the CLI TypeScript migration (PR 0 of the shell consolidation plan). No runtime changes — config, tooling, and dependency only. - tsconfig.cli.json: strict TS type-checking for bin/ and scripts/ (noEmit, module: preserve — tsx handles the runtime) - scripts/check-coverage-ratchet.ts: pure TS replacement for the bash+python coverage ratchet script (same logic, same tolerance) - execa ^9.6.1 added to root devDependencies (used by PR 1+) - pr.yaml: coverage ratchet step now runs the TS version via tsx - .pre-commit-config.yaml: SPDX headers cover scripts/*.ts, new tsc-check-cli pre-push hook - CONTRIBUTING.md: document typecheck:cli task and CLI pre-push hook - Delete scripts/check-coverage-ratchet.sh Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Apply suggestion from @brandonpelfrey * chore: address PR feedback — use types_or, add tsx devDep - Use `types_or: [ts, tsx]` instead of file glob for tsc-check-cli hook per @brandonpelfrey's suggestion. - Add `tsx` to devDependencies so CI doesn't re-fetch it on every run per CodeRabbit's suggestion. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(ci): ignore GitHub "Apply suggestion" commits in commitlint * fix(ci): lint only PR title since repo is squash-merge only Reverts the commitlint ignores rule from the previous commit and instead removes the per-commit lint step entirely. Individual commit messages are discarded at merge time — only the squash-merged PR title lands in main and drives changelog generation. Drop the per-commit lint, keep the PR title check, and remove the now-unnecessary fetch-depth: 0. * Revert "fix(ci): lint only PR title since repo is squash-merge only" This reverts commit 1257a47. * Revert "fix(ci): ignore GitHub "Apply suggestion" commits in commitlint" This reverts commit c395657. * docs: fix markdownlint MD032 in README (blank line before list) * refactor: make coverage ratchet script idiomatic TypeScript - Wrap in main() with process.exitCode instead of scattered process.exit() - Replace mutable flags with .map()/.some() over typed MetricResult[] - Separate pure logic (checkMetrics) from formatting (formatReport) - Throw with { cause } chaining instead of exit-in-helpers - Derive CoverageThresholds from METRICS tuple (single source of truth) - Exhaustive switch on CheckStatus discriminated union * refactor: remove duplication in coverage ratchet script - Drop STATUS_LABELS map; inline labels in exhaustive switch - Extract common 'metric coverage is N%' preamble in formatResult - Simplify ratchetedThresholds: use results directly (already in METRICS order) instead of re-scanning with .find() per metric - Compute 'failed' once in main, pass into formatReport to avoid duplicate .some() scan * refactor: simplify coverage ratchet with FP patterns - Extract classify() as a named pure function (replaces nested ternary) - loadJSON takes repo-relative paths, eliminating THRESHOLD_PATH and SUMMARY_PATH constants (DRY the join-with-REPO_ROOT pattern) - Drop CoverageMetric/CoverageSummary interfaces (only pct is read); use structural type at the call site instead - Inline ratchetedThresholds (one-liner, used once) - formatReport derives fail/improved from results instead of taking a pre-computed boolean (let functions derive from data, don't thread derived state) - sections.join("\n\n") replaces manual empty-string pushing - Shorter type names (Thresholds, Status, Result) — no ambiguity in a single-purpose script * refactor: strip coverage ratchet to failure-only output prek hides output from commands that exit 0, so ok/improved reporting was dead code. Remove Status, Result, classify, formatResult, formatReport, and the ratcheted-thresholds suggestion block. The script now just filters for regressions and prints actionable errors on failure. --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: Brandon Pelfrey <bpelfrey@nvidia.com>
…ets (NVIDIA#438) * fix: use CONNECT tunnel for WebSocket endpoints in Discord/Slack presets The egress proxy's HTTP idle timeout (~2 min) kills long-lived WebSocket connections when endpoints are configured with protocol:rest + tls:terminate. Switch WebSocket endpoints to access:full (CONNECT tunnel) which bypasses HTTP-level timeouts entirely. Discord: - gateway.discord.gg → access:full (WebSocket gateway) - Add PUT/PATCH/DELETE methods for discord.com (message editing, reactions) - Add media.discordapp.net for attachment access Slack: - Add wss-primary.slack.com and wss-backup.slack.com → access:full (Socket Mode WebSocket endpoints) Partially addresses NVIDIA#409 — the policy-level fix enables WebSocket connections to survive. The hardcoded 2-min timeout in openshell-sandbox still affects any protocol:rest endpoints with long-lived connections. Related: NVIDIA#361 (WhatsApp Web, same root cause) * fix: correct comment wording for media endpoint and YAML formatting
* fix: remove unused RECOMMENDED_NODE_MAJOR from scripts/install.sh Shellcheck flagged it as unused after the min/recommended merge. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: enforce full semver >=22.16.0 in installer scripts The runtime checks only compared the major Node.js version, allowing 22.0–22.15 to pass despite package.json requiring >=22.16.0. Use the version_gte() helper for full semver comparison in both installers. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: harden version_gte and align fallback message Guard version_gte() against prerelease suffixes (e.g. "22.16.0-rc.1") that would crash bash arithmetic. Also update the manual-install fallback message to reference MIN_NODE_VERSION instead of hardcoded "22". Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: update test stubs for Node.js 22.16 minimum and add Node 20 rejection test - Bump node stub in 'succeeds with acceptable Node.js' from v20.0.0 to v22.16.0 - Bump node stub in buildCurlPipeEnv from v22.14.0 to v22.16.0 - Add new test asserting Node.js 20 is rejected by ensure_supported_runtime --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
# Conflicts: # bin/nemoclaw.js
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @AgentWOPR, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
📝 WalkthroughWalkthroughThis pull request raises the minimum Node.js version requirement from 20 to 22.16 across documentation, configuration, and scripts, migrates the coverage ratchet checker from Bash to TypeScript, adds ESLint complexity rules and suppressions for CLI code, updates network policy presets for Discord and Slack, and introduces TypeScript type-checking for CLI-specific code via new configuration. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant InstallScript as install.sh / scripts/install.sh
participant VersionCheck as version_gte()
participant NodeEnv as Detected Node.js
User->>InstallScript: Execute installer
InstallScript->>NodeEnv: Retrieve node version (node -v)
NodeEnv-->>InstallScript: v22.16.0 (or other)
InstallScript->>VersionCheck: Compare against 22.16.0
alt Version >= 22.16.0
VersionCheck-->>InstallScript: Return true
InstallScript->>User: Proceed with setup
else Version < 22.16.0
VersionCheck-->>InstallScript: Return false
InstallScript->>User: Fail with requirement message
end
sequenceDiagram
participant GHAction as GitHub Actions
participant CoverageCheck as Coverage Ratchet Script
participant CoverageFile as coverage-summary.json
participant ThresholdFile as coverage-threshold.json
GHAction->>CoverageCheck: Invoke (npx tsx check-coverage-ratchet.ts)
CoverageCheck->>CoverageFile: Load actual metrics
CoverageFile-->>CoverageCheck: Return {lines, functions, branches, statements}
CoverageCheck->>ThresholdFile: Load thresholds
ThresholdFile-->>CoverageCheck: Return threshold values
CoverageCheck->>CoverageCheck: Compare each metric ±1% tolerance
alt Any metric fails (actual < threshold - 1)
CoverageCheck-->>GHAction: Exit code 1, print failures
else All metrics pass
CoverageCheck-->>GHAction: Exit code 0, silent success
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Review Summary by QodoUpgrade Node.js to 22.16, add complexity linting, and migrate coverage tooling to TypeScript
WalkthroughsDescription• Upgrade Node.js minimum requirement from 20 to 22.16.0 - Update version checks in install scripts and tests - Improve semver comparison to handle full version strings • Add ESLint cyclomatic complexity rule with suppressions - Ratchet threshold to 20, suppress 9 existing violations - Prevent new functions from accumulating excessive branching • Migrate coverage ratchet script from Bash/Python to TypeScript - Add new check-coverage-ratchet.ts with proper type safety - Remove legacy shell script implementation • Enhance Discord and Slack policy presets - Add WebSocket gateway support with CONNECT tunnels - Expand HTTP methods and media access rules • Add CLI TypeScript type-checking configuration - Create tsconfig.cli.json for bin/ and scripts/ directories - Add typecheck:cli npm script and pre-push hook • Update documentation and dependencies - Bump @types/node to ^22.0.0 in plugin package - Update README with host-side config file locations - Add pre-commit hook for new TypeScript scripts Diagramflowchart LR
A["Node.js 20 → 22.16"] -->|version_gte function| B["Semver comparison"]
B -->|install.sh & tests| C["Runtime validation"]
D["Bash coverage script"] -->|migrate| E["TypeScript coverage checker"]
E -->|check-coverage-ratchet.ts| F["Type-safe metrics"]
G["ESLint config"] -->|add complexity rule| H["9 suppressions"]
H -->|ratchet to 20| I["Prevent branching bloat"]
J["Discord/Slack presets"] -->|add WebSocket| K["CONNECT tunnels"]
K -->|avoid HTTP timeouts| L["Persistent connections"]
M["tsconfig.cli.json"] -->|type-check| N["bin/ & scripts/"]
N -->|pre-push hook| O["CLI validation"]
File Changes1. bin/lib/onboard.js
|
Code Review by Qodo
1. Unprovisioned /data HOME
|
Greptile SummaryThis upstream sync rebases WOPR sidecar commits onto Confidence Score: 2/5Not safe to merge until the GitHub-hosted runner violation is resolved — it's a declared org-wide blocker. The code changes themselves (Node version bump, coverage ratchet rewrite, tsconfig, policy presets, Dockerfile conflict resolution) are all clean and correct. However, every single job in the modified .github/workflows/pr.yaml — all six jobs need their
|
| Filename | Overview |
|---|---|
| .github/workflows/pr.yaml | Coverage ratchet step migrated from bash to tsx; all 6 jobs still use GitHub-hosted runners in violation of the org self-hosted runner requirement |
| Dockerfile | Merge conflict markers cleanly resolved: upstream config-hash pinning block is kept, WOPR sidecar ENV HOME=/data and EXPOSE 3100 are preserved; removed cp-a lines from HEAD are not referenced elsewhere |
| scripts/check-coverage-ratchet.ts | Replaces the bash/Python coverage ratchet with a typed TypeScript implementation; logic is correct but silently drops the coverage improved ratchet-up prompt present in the old script |
| install.sh | Minimum Node.js version bumped from major-20 to semver 22.16.0; adds proper version_gte semver comparator with prerelease-suffix guard; install_node now skips install when any version >= 22.16.0 is present |
| nemoclaw-blueprint/policies/presets/discord.yaml | gateway.discord.gg switched to access: full (CONNECT tunnel) for WebSocket support; PUT/PATCH/DELETE added to discord.com; media.discordapp.net added as read-only REST entry; changes are well-commented |
| tsconfig.cli.json | New TypeScript config for CLI/scripts type-checking (bin/, scripts/); noEmit, strict, allowJs/checkJs false, module:preserve for ESM compatibility |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[PR opened] --> B[lint job\nruns-on: ubuntu-latest ⚠️]
A --> C[test-unit job\nruns-on: ubuntu-latest ⚠️]
A --> D[build-sandbox-images\nruns-on: ubuntu-latest ⚠️]
A --> E[build-sandbox-images-arm64\nruns-on: ubuntu-24.04-arm ⚠️]
C --> C1[vitest --coverage]
C1 --> C2[npx tsx scripts/check-coverage-ratchet.ts\nREPLACES bash script]
D --> F[test-e2e-sandbox\nruns-on: ubuntu-latest ⚠️]
D --> G[test-e2e-gateway-isolation\nruns-on: ubuntu-latest ⚠️]
style B fill:#f99,stroke:#c00
style C fill:#f99,stroke:#c00
style D fill:#f99,stroke:#c00
style E fill:#f99,stroke:#c00
style F fill:#f99,stroke:#c00
style G fill:#f99,stroke:#c00
style C2 fill:#9cf,stroke:#069
Comments Outside Diff (1)
-
.github/workflows/pr.yaml, line 19 (link)GitHub-hosted runners violate org CI policy
All six jobs in this workflow use GitHub-hosted runners (
ubuntu-latestorubuntu-24.04-arm), which violates the WOPR org requirement that all GitHub Actions workflows useruns-on: [self-hosted, Linux, X64]exclusively. This is flagged as a blocker.Affected jobs and their
runs-onvalues:lint(line 19):ubuntu-latesttest-unit(line 52):ubuntu-latestbuild-sandbox-images(line 79):ubuntu-latestbuild-sandbox-images-arm64(line 121):ubuntu-24.04-armtest-e2e-sandbox(line 144):ubuntu-latesttest-e2e-gateway-isolation(line 164):ubuntu-latest
Each affected job should be changed, for example:
(The arm64 job would need a corresponding self-hosted arm64 label.)
Rule Used: WOPR Universal Conventions (apply to all wopr-netw... (source)
Prompt To Fix With AI
This is a comment left during a code review. Path: .github/workflows/pr.yaml Line: 19 Comment: **GitHub-hosted runners violate org CI policy** All six jobs in this workflow use GitHub-hosted runners (`ubuntu-latest` or `ubuntu-24.04-arm`), which violates the WOPR org requirement that all GitHub Actions workflows use `runs-on: [self-hosted, Linux, X64]` exclusively. This is flagged as a blocker. Affected jobs and their `runs-on` values: - `lint` (line 19): `ubuntu-latest` - `test-unit` (line 52): `ubuntu-latest` - `build-sandbox-images` (line 79): `ubuntu-latest` - `build-sandbox-images-arm64` (line 121): `ubuntu-24.04-arm` - `test-e2e-sandbox` (line 144): `ubuntu-latest` - `test-e2e-gateway-isolation` (line 164): `ubuntu-latest` Each affected job should be changed, for example: (The arm64 job would need a corresponding self-hosted arm64 label.) **Rule Used:** WOPR Universal Conventions (apply to all wopr-netw... ([source](https://app.greptile.com/review/custom-context?memory=c0d7e48b-8a3b-4e69-8256-317505a38329)) How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: .github/workflows/pr.yaml
Line: 19
Comment:
**GitHub-hosted runners violate org CI policy**
All six jobs in this workflow use GitHub-hosted runners (`ubuntu-latest` or `ubuntu-24.04-arm`), which violates the WOPR org requirement that all GitHub Actions workflows use `runs-on: [self-hosted, Linux, X64]` exclusively. This is flagged as a blocker.
Affected jobs and their `runs-on` values:
- `lint` (line 19): `ubuntu-latest`
- `test-unit` (line 52): `ubuntu-latest`
- `build-sandbox-images` (line 79): `ubuntu-latest`
- `build-sandbox-images-arm64` (line 121): `ubuntu-24.04-arm`
- `test-e2e-sandbox` (line 144): `ubuntu-latest`
- `test-e2e-gateway-isolation` (line 164): `ubuntu-latest`
Each affected job should be changed, for example:
```suggestion
runs-on: [self-hosted, Linux, X64]
```
(The arm64 job would need a corresponding self-hosted arm64 label.)
**Rule Used:** WOPR Universal Conventions (apply to all wopr-netw... ([source](https://app.greptile.com/review/custom-context?memory=c0d7e48b-8a3b-4e69-8256-317505a38329))
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: scripts/check-coverage-ratchet.ts
Line: 43
Comment:
**Silent success loses "ratchet-up" signal for contributors**
The old `check-coverage-ratchet.sh` explicitly printed per-metric `IMPROVED` lines and a full diff-ready JSON snippet telling contributors to update `ci/coverage-threshold.json` whenever coverage rose above the current floor. The new TypeScript version exits silently when all metrics pass — including when coverage has improved substantially above the threshold — so the ratchet floor will drift lower over time as contributors never see a prompt to update it.
Consider emitting at least a brief "coverage improved" notice when any metric exceeds its threshold by more than the tolerance:
```ts
const improved = METRICS.filter(
(m) => summary.total[m].pct > thresholds[m] + TOLERANCE,
);
if (improved.length > 0) {
console.log("Coverage improved on:", improved.join(", "));
console.log("Consider updating ci/coverage-threshold.json to ratchet the floor.");
}
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Merge remote-tracking branch 'upstream/m..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| threshold: thresholds[metric], | ||
| })).filter((r) => r.actual < r.threshold - TOLERANCE); | ||
|
|
||
| if (failures.length === 0) return; |
There was a problem hiding this comment.
The old check-coverage-ratchet.sh explicitly printed per-metric IMPROVED lines and a full diff-ready JSON snippet telling contributors to update ci/coverage-threshold.json whenever coverage rose above the current floor. The new TypeScript version exits silently when all metrics pass — including when coverage has improved substantially above the threshold — so the ratchet floor will drift lower over time as contributors never see a prompt to update it.
Consider emitting at least a brief "coverage improved" notice when any metric exceeds its threshold by more than the tolerance:
const improved = METRICS.filter(
(m) => summary.total[m].pct > thresholds[m] + TOLERANCE,
);
if (improved.length > 0) {
console.log("Coverage improved on:", improved.join(", "));
console.log("Consider updating ci/coverage-threshold.json to ratchet the floor.");
}Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/check-coverage-ratchet.ts
Line: 43
Comment:
**Silent success loses "ratchet-up" signal for contributors**
The old `check-coverage-ratchet.sh` explicitly printed per-metric `IMPROVED` lines and a full diff-ready JSON snippet telling contributors to update `ci/coverage-threshold.json` whenever coverage rose above the current floor. The new TypeScript version exits silently when all metrics pass — including when coverage has improved substantially above the threshold — so the ratchet floor will drift lower over time as contributors never see a prompt to update it.
Consider emitting at least a brief "coverage improved" notice when any metric exceeds its threshold by more than the tolerance:
```ts
const improved = METRICS.filter(
(m) => summary.total[m].pct > thresholds[m] + TOLERANCE,
);
if (improved.length > 0) {
console.log("Coverage improved on:", improved.join(", "));
console.log("Consider updating ci/coverage-threshold.json to ratchet the floor.");
}
```
How can I resolve this? If you propose a fix, please make it concise.![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+151
to
+152
| # WOPR sidecar — ensure writable HOME for runtime state | ||
| ENV HOME=/data |
There was a problem hiding this comment.
1. Unprovisioned /data home 🐞 Bug ⛯ Reliability
Dockerfile sets HOME=/data, but the image never creates /data or makes it writable for the sandbox/gateway users; the entrypoint writes under ~/.openclaw as the sandbox user, which will fail if /data isn’t mounted and writable. This can prevent the container from starting (set -e) in environments that don’t pre-create a writable /data volume.
Agent Prompt
### Issue description
The container sets `HOME=/data` but does not ensure `/data` exists and is writable for the `sandbox`/`gateway` users. Since the entrypoint writes under `~/.openclaw/...` as `sandbox`, startup can fail when `/data` isn’t mounted or is root-owned.
### Issue Context
- `Dockerfile` sets `ENV HOME=/data`.
- `scripts/nemoclaw-start.sh` writes auth profiles to `~/.openclaw/...` via `os.path.expanduser('~')` as the `sandbox` user.
- The base image provisions `/sandbox` but not `/data`.
### Fix Focus Areas
- Dockerfile[151-155]
- scripts/nemoclaw-start.sh[87-107]
- Dockerfile.base[82-89]
### Suggested fix
In `Dockerfile`, before setting `ENV HOME=/data`, add a step to create `/data` and make it writable for both `gateway` and `sandbox` (e.g., `mkdir -p /data && chmod 1777 /data`, or a tighter ownership/ACL strategy that still allows both users to write).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
scripts/check-coverage-ratchet.ts (1)
37-41: Consider defensive access for coverage metrics.If Vitest's coverage output is missing a metric (e.g., due to configuration changes or version differences), accessing
summary.total[metric].pctwould throw. A defensive check would make failures more diagnosable.🛡️ Optional: Add defensive metric access
- const failures = METRICS.map((metric) => ({ - metric, - actual: summary.total[metric].pct, - threshold: thresholds[metric], - })).filter((r) => r.actual < r.threshold - TOLERANCE); + const failures = METRICS.map((metric) => { + const entry = summary.total[metric]; + if (!entry || typeof entry.pct !== "number") { + throw new Error(`Missing coverage metric: ${metric}`); + } + return { + metric, + actual: entry.pct, + threshold: thresholds[metric], + }; + }).filter((r) => r.actual < r.threshold - TOLERANCE);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/check-coverage-ratchet.ts` around lines 37 - 41, The failures building logic assumes summary.total[metric].pct exists and will throw if a metric is missing; update the code that constructs failures (the METRICS map and subsequent .filter) to defensively access values from summary.total for each metric (e.g., check summary.total?.[metric] and summary.total[metric]?.pct) and either default missing pct to 0 or skip that metric and log a diagnostic; ensure thresholds[metric] and TOLERANCE are used with the defensive value so absent metrics don't raise exceptions when computing r.actual < r.threshold - TOLERANCE.docs/reference/troubleshooting.md (1)
50-63: Node.js version update looks correct; consider varying sentence openers.The version update from 20 to 22.16 aligns with the project-wide minimum version change. The
nvmcommands correctly reference Node 22.Static analysis flagged three consecutive sentences starting with "If" (lines 51, 57-58). Consider rephrasing one to improve flow:
Suggested rewording
-If the version is below 22.16, install a supported release. -If you use nvm, run: +When the version is below 22.16, install a supported release. +If you use nvm, run:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/troubleshooting.md` around lines 50 - 63, Rephrase one of the three consecutive sentences that start with "If" to improve sentence variety: change either "If the installer exits with a Node.js version error," to "When the installer exits with a Node.js version error," or change "If the version is below 22.16, install a supported release." to "Install a supported release if your version is below 22.16." and change "If you use nvm, run:" to "For nvm users, run:" so the three lines ("If the installer exits with a Node.js version error,", "If the version is below 22.16, install a supported release.", "If you use nvm, run:") no longer all start with "If" while preserving the same meaning and commands.test/install-preflight.test.js (1)
1304-1328: Please make the negative case hit the new 22.16 patch boundary.Using
v20.18.0proves a much older runtime is rejected, but the new contract here is22.15.x -> 22.16.0. A regression back to a major-only>=22check would still keep this test green, so I’d add av22.15.xcase alongside this one.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/install-preflight.test.js` around lines 1304 - 1328, The test "rejects Node.js 20 which is below the 22.16 minimum" is too loose for the new 22.16 patch boundary; add a negative case that specifically uses a v22.15.x runtime to ensure the code rejects 22.15.* (e.g., create another it block or extend this one to call writeExecutable for "node" returning "v22.15.3" and invoke callEnsureSupportedRuntime with that fakeBin), reuse writeExecutable and callEnsureSupportedRuntime so the assertion checks result.status !== 0 and output includes "v22.15.3" and "Unsupported runtime detected".nemoclaw-blueprint/policies/presets/slack.yaml (1)
36-43: Add a regression test for the WebSocket tunnel hosts.For Slack/Discord, the current policy tests mostly prove that hosts can be extracted; they do not pin
access: fullon the long-lived WebSocket entries. A later flip back to REST would still pass today, so a targetedpolicies.test.jsassertion for these Socket Mode hosts (andgateway.discord.gg) would make this fix much harder to regress.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw-blueprint/policies/presets/slack.yaml` around lines 36 - 43, Add a regression test in policies.test.js that loads/parses the presets (the same codepath used by existing policy tests) and asserts that the long-lived WebSocket entries "wss-primary.slack.com" and "wss-backup.slack.com" (and similarly "gateway.discord.gg") are present with access: full; locate the test file policies.test.js, call the same policy builder/loader used by other tests, find the host entries by hostname, and add strict assertions that their access property equals "full" so future flips to REST will fail the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@install.sh`:
- Around line 216-218: The script sets MIN_NODE_VERSION="22.16.0" but
install_nodejs() only checks for the existence of a node binary and returns
early, causing machines with older Node (e.g., v20.x or v22.14.x) to skip
upgrade; update install_nodejs() to detect the installed node version (e.g., run
node --version), semver-compare it with MIN_NODE_VERSION, and only return early
if the installed version >= MIN_NODE_VERSION; otherwise invoke the
upgrade/installer path (reuse the existing logic from scripts/install.sh or the
curl-pipe installer) and ensure RUNTIME_REQUIREMENT_MSG and MIN_NPM_MAJOR checks
are enforced after upgrading; reference install_nodejs(), main(),
MIN_NODE_VERSION, MIN_NPM_MAJOR when making the change.
In `@package.json`:
- Around line 31-32: Update CI workflow node versions to match the package.json
engines constraint: change any workflow job using node-version "20" or an
unpinned "22" to use Node 22 pinned at or above the minimum required patch
(e.g., "22.16"); specifically, align the package.json "engines" -> "node" value
(">=22.16.0") with workflow node-version keys so all CI runs use at least 22.16
(replace node-version: "20" with "22.16" and change node-version: "22" to
"22.16" or higher).
---
Nitpick comments:
In `@docs/reference/troubleshooting.md`:
- Around line 50-63: Rephrase one of the three consecutive sentences that start
with "If" to improve sentence variety: change either "If the installer exits
with a Node.js version error," to "When the installer exits with a Node.js
version error," or change "If the version is below 22.16, install a supported
release." to "Install a supported release if your version is below 22.16." and
change "If you use nvm, run:" to "For nvm users, run:" so the three lines ("If
the installer exits with a Node.js version error,", "If the version is below
22.16, install a supported release.", "If you use nvm, run:") no longer all
start with "If" while preserving the same meaning and commands.
In `@nemoclaw-blueprint/policies/presets/slack.yaml`:
- Around line 36-43: Add a regression test in policies.test.js that loads/parses
the presets (the same codepath used by existing policy tests) and asserts that
the long-lived WebSocket entries "wss-primary.slack.com" and
"wss-backup.slack.com" (and similarly "gateway.discord.gg") are present with
access: full; locate the test file policies.test.js, call the same policy
builder/loader used by other tests, find the host entries by hostname, and add
strict assertions that their access property equals "full" so future flips to
REST will fail the test.
In `@scripts/check-coverage-ratchet.ts`:
- Around line 37-41: The failures building logic assumes
summary.total[metric].pct exists and will throw if a metric is missing; update
the code that constructs failures (the METRICS map and subsequent .filter) to
defensively access values from summary.total for each metric (e.g., check
summary.total?.[metric] and summary.total[metric]?.pct) and either default
missing pct to 0 or skip that metric and log a diagnostic; ensure
thresholds[metric] and TOLERANCE are used with the defensive value so absent
metrics don't raise exceptions when computing r.actual < r.threshold -
TOLERANCE.
In `@test/install-preflight.test.js`:
- Around line 1304-1328: The test "rejects Node.js 20 which is below the 22.16
minimum" is too loose for the new 22.16 patch boundary; add a negative case that
specifically uses a v22.15.x runtime to ensure the code rejects 22.15.* (e.g.,
create another it block or extend this one to call writeExecutable for "node"
returning "v22.15.3" and invoke callEnsureSupportedRuntime with that fakeBin),
reuse writeExecutable and callEnsureSupportedRuntime so the assertion checks
result.status !== 0 and output includes "v22.15.3" and "Unsupported runtime
detected".
🪄 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: 0898159d-eae0-494e-b884-e3cb1753bcba
⛔ Files ignored due to path filters (2)
nemoclaw/package-lock.jsonis excluded by!**/package-lock.jsonpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (22)
.agents/skills/nemoclaw-get-started/SKILL.md.agents/skills/nemoclaw-reference/references/troubleshooting.md.github/workflows/pr.yaml.pre-commit-config.yamlCONTRIBUTING.mdDockerfileREADME.mdbin/lib/onboard.jsbin/lib/policies.jsbin/nemoclaw.jsdocs/reference/troubleshooting.mdeslint.config.mjsinstall.shnemoclaw-blueprint/policies/presets/discord.yamlnemoclaw-blueprint/policies/presets/slack.yamlnemoclaw/package.jsonpackage.jsonscripts/check-coverage-ratchet.shscripts/check-coverage-ratchet.tsscripts/install.shtest/install-preflight.test.jstsconfig.cli.json
💤 Files with no reviewable changes (1)
- scripts/check-coverage-ratchet.sh
Comment on lines
+216
to
+218
| MIN_NODE_VERSION="22.16.0" | ||
| MIN_NPM_MAJOR=10 | ||
| RECOMMENDED_NODE_MAJOR=22 | ||
| RUNTIME_REQUIREMENT_MSG="NemoClaw requires Node.js >=${MIN_NODE_MAJOR} and npm >=${MIN_NPM_MAJOR} (recommended Node.js ${RECOMMENDED_NODE_MAJOR})." | ||
| RUNTIME_REQUIREMENT_MSG="NemoClaw requires Node.js >=${MIN_NODE_VERSION} and npm >=${MIN_NPM_MAJOR}." |
There was a problem hiding this comment.
This now breaks repo-checkout installs on machines that already have an older Node.js.
main() still calls install_nodejs() first, but that function returns as soon as any node binary exists. With the floor raised to 22.16.0, a workstation on v20.x or v22.14.x now hard-fails here instead of being upgraded, while scripts/install.sh already handles the upgrade path.
Suggested alignment with the curl-pipe installer
install_nodejs() {
- if command_exists node; then
- info "Node.js found: $(node --version)"
- return
- fi
+ local node_version="" npm_version="" npm_major=""
+ if command_exists node; then
+ node_version="$(node --version 2>/dev/null || true)"
+ npm_version="$(npm --version 2>/dev/null || true)"
+ npm_major="$(version_major "$npm_version")"
+ info "Node.js found: ${node_version:-unknown}"
+ if version_gte "${node_version#v}" "$MIN_NODE_VERSION" \
+ && [[ "$npm_major" =~ ^[0-9]+$ ]] \
+ && (( npm_major >= MIN_NPM_MAJOR )); then
+ return
+ fi
+ info "Upgrading runtime to satisfy ${RUNTIME_REQUIREMENT_MSG}"
+ fi
- info "Node.js not found — installing via nvm…"
+ info "Installing Node.js via nvm…"Also applies to: 305-306
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@install.sh` around lines 216 - 218, The script sets
MIN_NODE_VERSION="22.16.0" but install_nodejs() only checks for the existence of
a node binary and returns early, causing machines with older Node (e.g., v20.x
or v22.14.x) to skip upgrade; update install_nodejs() to detect the installed
node version (e.g., run node --version), semver-compare it with
MIN_NODE_VERSION, and only return early if the installed version >=
MIN_NODE_VERSION; otherwise invoke the upgrade/installer path (reuse the
existing logic from scripts/install.sh or the curl-pipe installer) and ensure
RUNTIME_REQUIREMENT_MSG and MIN_NPM_MAJOR checks are enforced after upgrading;
reference install_nodejs(), main(), MIN_NODE_VERSION, MIN_NPM_MAJOR when making
the change.
Comment on lines
31
to
+32
| "engines": { | ||
| "node": ">=22.0.0" | ||
| "node": ">=22.16.0" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check what node-version is used in CI workflows
rg -n 'node-version' .github/workflows/Repository: wopr-network/nemoclaw
Length of output: 379
🏁 Script executed:
cat -n package.json | sed -n '28,35p'Repository: wopr-network/nemoclaw
Length of output: 244
🏁 Script executed:
cat -n .github/workflows/pr.yaml | sed -n '24,32p'Repository: wopr-network/nemoclaw
Length of output: 303
🏁 Script executed:
cat -n .github/workflows/upstream-sync.yml | sed -n '30,40p'Repository: wopr-network/nemoclaw
Length of output: 489
Fix CI node-version misalignment with engine constraint.
The engine constraint is >=22.16.0, but .github/workflows/upstream-sync.yml (line 34) uses node-version: "20", which violates this requirement. Additionally, .github/workflows/pr.yaml line 28 uses node-version: "22" without pinning to the minimum version. Update upstream-sync.yml to node 22 and consider pinning CI workflows to a specific minimum version (e.g., "22.16") to enforce alignment with the engine constraint across all workflows.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` around lines 31 - 32, Update CI workflow node versions to match
the package.json engines constraint: change any workflow job using node-version
"20" or an unpinned "22" to use Node 22 pinned at or above the minimum required
patch (e.g., "22.16"); specifically, align the package.json "engines" -> "node"
value (">=22.16.0") with workflow node-version keys so all CI runs use at least
22.16 (replace node-version: "20" with "22.16" and change node-version: "22" to
"22.16" or higher).
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
7 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.
Automated upstream sync
Rebased our WOPR sidecar commits onto upstream/main (NVIDIA/NemoClaw).
What this does
Verify
Note
Bump minimum Node.js requirement to 22.16.0 and add WebSocket gateway support to Discord and Slack policies
tsx; adds atypecheck:clinpm script and tsconfig.cli.json for strict type-checking ofbin/andscripts/.CMD ["/bin/bash"]is added to the Dockerfile alongside the existing entrypoint, causing/bin/bashto be passed as an argument to the entrypoint script.Macroscope summarized a87c151.
Summary by CodeRabbit
Release Notes
New Features
Documentation
Chores