fix(entrypoint): gracefully skip invalid overrides instead of hard-exit - #2700
Conversation
Validation failures for non-security-critical overrides (malformed integers, invalid boolean, invalid API type, non-http CORS origin) now return 0 instead of return 1. Under set -e, return 1 killed the container before CMD could run, breaking the runtime-overrides E2E test 14 which expects to read unchanged config after a rejected override. Security-critical rejections (symlinks, control characters) remain return 1 since those indicate real tampering attempts. Fixes #2698 Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe pull request modifies validation error handling in the entrypoint script. When environment variable validation fails for model/API parameters and CORS origin, the script now logs a warning and continues startup instead of exiting with failure, preserving build-time defaults and preventing container abort. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 docstrings
🧪 Generate unit tests (beta)
Review rate limit: 8/10 reviews remaining, refill in 10 minutes and 5 seconds. Comment |
…it (NVIDIA#2700) ## Summary Fixes NVIDIA#2698 — `runtime-overrides-e2e` test 14 crashes because invalid override validation uses `return 1`, which under `set -euo pipefail` kills the container before CMD runs. ## Root Cause PR NVIDIA#2659 tightened the trigger guard for `apply_model_override()` but kept `return 1` for validation failures. When `NEMOCLAW_CONTEXT_WINDOW=notanumber` is rejected, `return 1` propagates through `set -e` and exits the entrypoint. The container dies before the test can read the config. ## Fix Change non-security-critical validation failures from `return 1` to `return 0`: - Invalid API type → skip, don't crash - Non-integer context window → skip, don't crash - Non-integer max tokens → skip, don't crash - Invalid reasoning boolean → skip, don't crash - Non-http CORS origin → skip, don't crash Security-critical rejections remain `return 1`: - Symlink on config/hash path (indicates tampering) - Control characters in model override (indicates injection) - Oversized model override (indicates injection) The security warning is still logged to stderr, but the container starts normally with config unchanged. ## Verification - [x] Unit tests pass (`test/nemoclaw-start.test.ts` — assertions use `toContain` on message strings which are preserved) - [ ] `runtime-overrides-e2e` passes (to be validated on sparky) ## AI Disclosure - [x] AI-assisted — tool: Claude Code (pi agent) --- Signed-off-by: Julie Yaunches <jyaunches@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved startup resilience by gracefully handling invalid configuration parameters. The system now logs warnings and continues instead of aborting when misconfigured environment variables are detected. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
…2842) ## Summary Resolves #2762. The Dockerfile build invokes `scripts/generate-openclaw-config.py` once at image build time to bake `openclaw.json`. Two of the build args it consumes (`NEMOCLAW_CONTEXT_WINDOW`, `NEMOCLAW_MAX_TOKENS`) were coerced via an unguarded `int(env.get(...))`, so a non-numeric, zero, or negative `--build-arg` would surface a Python traceback and abort the build instead of skipping the override the way PR #2700 made the runtime entrypoint behave. This change mirrors the runtime-entrypoint pattern: log a `[SECURITY] ... — skipping override, falling back to default` warning to stderr and use the documented default. Empty-string overrides also fall back to the default now, consistent with `NEMOCLAW_PROXY_HOST` / `NEMOCLAW_PROXY_PORT`. ## Related Issue Resolves #2762 ## Changes - `scripts/generate-openclaw-config.py`: new private helper `_coerce_positive_int(env, name, default)` that returns the parsed value when it is a positive integer string, otherwise emits the `[SECURITY]` warning and returns the default. Both `NEMOCLAW_CONTEXT_WINDOW` (default 131072) and `NEMOCLAW_MAX_TOKENS` (default 4096) now route through it. - `test/generate-openclaw-config.test.ts`: 8 new tests — 2 empty-string fallback cases (matching the existing `NEMOCLAW_PROXY_*` empty-string suite) plus 6 invalid-value cases (`notanumber`, `0`, `-1` for each var) that assert both the fallback value lands in the generated config AND the `[SECURITY]` warning text appears on stderr. ## 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) ## Verification - [X] `npx prek run --all-files` passes - [X] `npm test` passes (38/38 in `generate-openclaw-config.test.ts`, 112/112 in `nemoclaw-start.test.ts`) - [X] Tests added or updated for new or changed behavior - [X] No secrets, API keys, or credentials committed - [ ] Docs updated for user-facing behavior changes - [ ] `make 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) ## AI Disclosure - [X] AI-assisted — tools: Claude Code, OpenAI Codex --- Signed-off-by: Tinson Lai <tinsonl@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Validation tightened for context window and max token settings: only strictly positive integers are accepted. Empty, non-numeric, zero, negative, or excessively large values now trigger a security warning to stderr and revert to safe defaults. * **Tests** * Added tests covering empty, non-numeric, zero/negative, and oversized environment inputs to verify warnings and fallback behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Stress-test against 5 historical NemoClaw cases surfaced two real spec
gaps that produced incorrect classifications:
1. Evidence-required filter was too strict for partial-fix detection.
The LLM couldn't cite "PR diff line" when the gap is what the PR
did NOT touch. Extended evidence to three citation shapes:
- DIRECT (original): cite specific PR line + issue symptom
- BY-OMISSION: cite PR scope + issue symptom showing same bug class
in different instances PR did not touch (catches partial-fix)
- FOLLOW-ON: cite the symbol/file PR introduced + issue's request
to harden the same code (catches "PR introduced X, now harden X")
2. ADJACENT_FIX wording assumed the issue was already broken and the
PR resolves it. Reframed to also include "PR opens a clear follow-on
path on the same code the PR just touched" — covers the common
pattern where an issue requests hardening of code a PR just
introduced.
Test cases that drove these patches:
- #2700 → #2762 (partial-fix, was failing — now passes via by-omission)
- #2696 → #2875 (follow-on hardening, was ambiguous — now explicit)
Summary
Fixes #2698 —
runtime-overrides-e2etest 14 crashes because invalid override validation usesreturn 1, which underset -euo pipefailkills the container before CMD runs.Root Cause
PR #2659 tightened the trigger guard for
apply_model_override()but keptreturn 1for validation failures. WhenNEMOCLAW_CONTEXT_WINDOW=notanumberis rejected,return 1propagates throughset -eand exits the entrypoint. The container dies before the test can read the config.Fix
Change non-security-critical validation failures from
return 1toreturn 0:Security-critical rejections remain
return 1:The security warning is still logged to stderr, but the container starts normally with config unchanged.
Verification
test/nemoclaw-start.test.ts— assertions usetoContainon message strings which are preserved)runtime-overrides-e2epasses (to be validated on sparky)AI Disclosure
Signed-off-by: Julie Yaunches jyaunches@nvidia.com
Summary by CodeRabbit