fix(scripts): enforce config integrity check in non-root mode#1071
fix(scripts): enforce config integrity check in non-root mode#1071cv merged 11 commits intoNVIDIA:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe non-root execution path in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
🧹 Nitpick comments (1)
scripts/nemoclaw-start.sh (1)
326-329: Root path behavior remains correct and consistent.The root path relies on
set -euo pipefailto makeverify_config_integrityfatal, which works correctly. Both execution paths now consistently refuse to start on integrity failures.Operational note: Per the relevant code snippets, some callers (
bin/lib/onboard.js,scripts/telegram-bridge.js,scripts/walkthrough.sh) may not properly propagate the exit code when invokingnemoclaw-startoutside the ENTRYPOINT context (e.g., via SSH, tmux, oropenshell sandbox create). While this script now correctly exits with status 1 on integrity failure, the callers may silently ignore it. Consider auditing those call sites to ensure security failures are surfaced to users.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/nemoclaw-start.sh` around lines 326 - 329, The script now correctly exits with non-zero on verify_config_integrity failures, but some callers (bin/lib/onboard.js, scripts/telegram-bridge.js, scripts/walkthrough.sh) may not propagate that exit code; audit those call sites and ensure they detect and forward the child process exit status (e.g., check spawn/exec return codes or use synchronous/awaited execution and process.exit(retcode) when invoking nemoclaw-start) so integrity failures are not silently ignored.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/nemoclaw-start.sh`:
- Around line 326-329: The script now correctly exits with non-zero on
verify_config_integrity failures, but some callers (bin/lib/onboard.js,
scripts/telegram-bridge.js, scripts/walkthrough.sh) may not propagate that exit
code; audit those call sites and ensure they detect and forward the child
process exit status (e.g., check spawn/exec return codes or use
synchronous/awaited execution and process.exit(retcode) when invoking
nemoclaw-start) so integrity failures are not silently ignored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8b745840-0871-4fb6-a7ee-a98c7aaed0f3
📒 Files selected for processing (1)
scripts/nemoclaw-start.sh
Caller audit (addressing CodeRabbit review suggestion)Audited the three call sites that invoke
Conclusion: All callers already propagate or surface the exit code. No code changes needed. |
In non-root mode, verify_config_integrity() failure was logged as a warning but execution continued, allowing a tampered openclaw.json to be loaded. The root path correctly treats this as fatal. Make the non-root path consistent: exit 1 on integrity failure so a modified config is never silently accepted regardless of the execution mode. Closes NVIDIA#1013
0285fb0 to
8921233
Compare
Verify that the non-root code path exits on verify_config_integrity failure and does not contain the old "proceeding anyway" fallback. Also verify both root and non-root paths call verify_config_integrity. Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com>
|
Added two regression tests in the latest commit verifying the non-root integrity exit and ensuring both code paths call verify_config_integrity. Related: @Junior00619 also tackled this in #1032 — appreciate the parallel effort on this one. Their PR independently identified the same fix and included test coverage as well. |
Appreciate the tag! Lets continue building :))! |
|
Review note: I compared #1071 and #1032 directly against current main. Both addressed issue #1013 correctly by making the non-root config-integrity check fatal in |
Summary
In non-root mode, verify_config_integrity() failure was logged as a warning but execution continued, allowing a tampered openclaw.json to be loaded. The root path already treats this as fatal. Now consistent: exit 1 on failure in both modes.
Related Issue
Closes #1013
Changes
Type of Change
Testing
npx prek run --all-filespasses (or equivalentlymake check).npm testpasses.make docsbuilds without warnings. (for doc-only changes)Checklist
General
Code Changes
Summary by CodeRabbit