fix(sandbox): honor model override in reconcile and log guard-chain warning to file#6065
Conversation
…arning to file Two regressions from #5874: 1. reconcile_agent_model_with_provider ran after apply_model_override and overwrote agents.defaults.model.primary with an inference/-prefixed variant because qualify() unconditionally adds the prefix. Guard with an early return when NEMOCLAW_MODEL_OVERRIDE is set — the comment at the call site already documented this intent but the implementation didn't enforce it. 2. restore_openclaw_runtime_guard_chain wrote its warning only to stderr. The new host-mediated recovery path (docker exec nemoclaw-gateway-control recover) runs inside PID 1, not inside a fresh sandbox exec, so stderr never reached /tmp/gateway.log. Mirror the warning to the gateway log file using the same pattern as the watchdog (_NEMOCLAW_GATEWAY_LOG seam for tests). Signed-off-by: Preksha Vijayvargiya <prekshiv@nvidia.com>
📝 WalkthroughWalkthroughModifies scripts/nemoclaw-start.sh with two changes: an early-return guard preventing agent/provider-model reconciliation when NEMOCLAW_MODEL_OVERRIDE is set, and duplication of the runtime guard chain warning message into the gateway log file in addition to stderr output. ChangesStartup Script Fixes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in the branch is 96%. Coverage data for the branch is not yet available. Show a code coverage summary of the most covered files.
TypeScript / code-coverage/cliThe overall coverage in the branch is 68%. Coverage data for the branch is not yet available. Show a code coverage summary of the most covered files.
Updated |
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
E2E Target RecommendationRequired E2E targets: Dispatch required E2E targets:
Full E2E target advisor summaryE2E Target AdvisorBase: Required E2E targets
Optional E2E targets
Relevant changed files
|
PR Review Advisor — Changes requestedMerge posture: Do not merge yet Action checklist
Findings index
Review findings by urgency: 0 required fixes, 2 items to resolve/justify, 0 in-scope improvements
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/nemoclaw-start.sh`:
- Around line 1068-1071: Add test coverage for the no-override reconciliation
path in `scripts/nemoclaw-start.sh`, specifically around the
`NEMOCLAW_MODEL_OVERRIDE` guard and the gateway/legacy model reconciliation
logic. Extend the existing tests that verify `agents.defaults.model.primary`
preservation so they also assert that when `NEMOCLAW_MODEL_OVERRIDE` is unset,
reconciliation can still replace the model with the gateway-derived value. Make
sure the new case targets the same flow controlled by the override check so
regressions in `apply_model_override`/reconciliation are caught.
🪄 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: Enterprise
Run ID: 504fff40-39aa-4268-b8b1-00468674de30
📒 Files selected for processing (1)
scripts/nemoclaw-start.sh
| # apply_model_override already won; reconciling against the gateway would | ||
| # overwrite the user's explicit choice with an inference/-prefixed variant. | ||
| [ -z "${NEMOCLAW_MODEL_OVERRIDE:-}" ] || return 0 | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for a test asserting reconciliation still runs when override is unset.
rg -n -B2 -A15 'reconcile_agent_model_with_provider|provider-model reconciliation' test/nemoclaw-start.test.tsRepository: NVIDIA/NemoClaw
Length of output: 2303
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='test/nemoclaw-start.test.ts'
echo '--- search: override + primary ---'
rg -n 'NEMOCLAW_MODEL_OVERRIDE|agents\.defaults\.model\.primary|reconcile_agent_model_with_provider|gateway|legacy-derived|provider-model' "$file" || true
echo
echo '--- around first reconcile stub ---'
sed -n '430,520p' "$file"
echo
echo '--- around second reconcile stub ---'
sed -n '3600,3685p' "$file"Repository: NVIDIA/NemoClaw
Length of output: 19828
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1280,1385p' test/nemoclaw-start.test.tsRepository: NVIDIA/NemoClaw
Length of output: 3748
Add coverage for the unset-override reconciliation path. The tests cover NEMOCLAW_MODEL_OVERRIDE preserving agents.defaults.model.primary, but not the no-override case where gateway/legacy reconciliation should still be able to replace it. Without that, a future change could disable reconciliation entirely without a failing test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/nemoclaw-start.sh` around lines 1068 - 1071, Add test coverage for
the no-override reconciliation path in `scripts/nemoclaw-start.sh`, specifically
around the `NEMOCLAW_MODEL_OVERRIDE` guard and the gateway/legacy model
reconciliation logic. Extend the existing tests that verify
`agents.defaults.model.primary` preservation so they also assert that when
`NEMOCLAW_MODEL_OVERRIDE` is unset, reconciliation can still replace the model
with the gateway-derived value. Make sure the new case targets the same flow
controlled by the override check so regressions in
`apply_model_override`/reconciliation are caught.
Source: Path instructions
PR Review Advisor (Nemotron Ultra) — Changes requestedMerge posture: Do not merge yet Action checklist
Findings index
Review findings by urgency: 0 required fixes, 3 items to resolve/justify, 1 in-scope improvement
|
Vitest E2E Target Results — ✅ All requested jobs passedRun: 28472585714
|
<!-- markdownlint-disable MD041 --> ## Summary Refreshes the public documentation for NemoClaw v0.0.71 after scanning commits since v0.0.70. Adds release notes and fills the remaining doc gaps for Windows bootstrap diagnostics, OpenClaw agent auto-relock warnings, auto-pair cadence tuning, and plugin-install recovery hints. ## Changes - `docs/about/release-notes.mdx`: adds the v0.0.71 release-note section, grouped by gateway recovery, OpenShell auth, policy provenance, day-two maintenance, messaging/inference, and Windows setup. - `docs/get-started/windows-preparation.mdx`: documents sanitized WSL install output and reboot gating in the Windows bootstrap. - `docs/reference/commands.mdx`: documents the host `agent` wrapper's shields auto-relock warning and OpenClaw auto-pair watcher tuning variables. - `docs/reference/troubleshooting.mdx`: adds plugin-install network failure recovery guidance and updates Windows WSL troubleshooting for sanitized install logs and reboot-required handling. Source summary: - #6065 -> `docs/about/release-notes.mdx`: Notes explicit model override preservation and gateway-log guard-chain recovery diagnostics. - #5874 -> `docs/about/release-notes.mdx`: Summarizes host-mediated `recover` and `gateway restart`, linking to lifecycle, command, troubleshooting, and trusted-boundary docs already added by the source PR. - #5596 -> `docs/about/release-notes.mdx`: Summarizes OpenShell 0.0.71 gateway auth, loopback binding, and compatibility-container docs already added by the source PR. - #5797 and #5798 -> `docs/about/release-notes.mdx`: Summarizes `policy-list` provenance, Restricted tier suppression, and Balanced tier weather behavior already reflected in policy docs. - #5784 -> `docs/about/release-notes.mdx`: Summarizes `--destroy-user-data` and the safe `--yes` uninstall behavior already documented in lifecycle and command docs. - #6034 -> `docs/about/release-notes.mdx`: Summarizes custom Dockerfile warm-build cache behavior already documented in the command reference. - #5951 -> `docs/reference/commands.mdx`: Documents the stderr-only host `agent` wrapper warning after recent shields auto-relock. - #5387 -> `docs/reference/commands.mdx`: Documents OpenClaw auto-pair watcher cadence and fast-reentry tuning variables. - #5835 -> `docs/reference/troubleshooting.mdx`: Adds recovery guidance for OpenClaw plugin-install network failures. - #5995 and #5956 -> `docs/about/release-notes.mdx`: Summarizes Microsoft Teams final-message delivery and runtime mention hints already covered by messaging docs. - #5716 -> `docs/about/release-notes.mdx`: Summarizes non-interactive Ollama loopback safety already covered by local inference docs. - #5505, #5527, and #5528 -> `docs/about/release-notes.mdx`: Summarizes compatible local endpoint, model task-fit, and model capability audit docs. - #6009 -> `docs/get-started/windows-preparation.mdx`, `docs/reference/troubleshooting.mdx`: Documents sanitized Windows bootstrap WSL output and reboot-required gating. - #6055 -> no additional source doc page change needed beyond the already-merged quickstart update; release notes did not duplicate routine quickstart cleanup. No matching v0.0.71 GitHub announcement discussion was found in the latest 20 discussions, so this refresh is based on the commit scan and existing source PR docs. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [x] Doc only (includes code sample changes) ## Quality Gates <!-- Check all that apply. For any "covered by existing tests", "not applicable", or waiver entry, add a brief justification on the same line or in the Changes section. --> - [ ] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [x] Tests not applicable — justification: docs-only refresh with no runtime behavior changes. - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: - [ ] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [ ] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification <!-- Check each item you ran and confirmed. Leave unchecked items you skipped. Doc-only changes do not require npm test unless you ran it. --> - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] Git hooks passed during commit and push, or `npx prek run --from-ref main --to-ref HEAD` passes - [ ] Targeted tests pass for changed behavior - [ ] Full `npm test` passes (broad runtime changes only) - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) — ran `npm run docs`; Fern reported 0 errors and 2 existing warnings. - [x] 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) --- <!-- DCO sign-off is required in this PR description, and every commit must appear as Verified in GitHub. Run: git config user.name && git config user.email --> Signed-off-by: Julie Yaunches <jyaunches@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added a new release-notes entry covering gateway recovery, authentication, network policy/provenance output, uninstall safety, Windows bootstrap diagnostics, messaging defaults, and inference setup guidance. * Clarified Windows preparation steps around reboot behavior and redacting troubleshooting transcripts. * Expanded command reference details for OpenClaw wrapper behavior and new auto-pair tuning options. * Improved troubleshooting guidance for plugin installation issues, WSL repair/reboot cases, and install timing problems. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Julie Yaunches <jyaunches@nvidia.com> Co-authored-by: Prekshi Vyas <34834085+prekshivyas@users.noreply.github.com>
…st it An audit of the live E2E suite (which does not run on PR CI) found behavior-critical assertions that were only guarded by live targets, the same gap that let the #6065 regressions ship. Backfill the high-priority, security/recovery-class ones as fast mocked units that run on every PR: - ollama-auth-proxy: Bearer enforcement, no /api/tags bypass (#3338), header stripping, non-ASCII auth no-crash (#4820), backend 502. - config get: credential redaction + gateway-key omission (the nvapi- class). - device approval policy: scope-upgrade allowlist gate, gateway-env stripping, and recover-failed rejection paths (#4462). - shields audit JSONL: credentials redacted before persistence. - hermes env secret boundary: value-shape (not key-name) discriminator accepts openshell:resolve refs and rejects raw secrets without echoing (added to the dedicated hardening suite). - dashboard bind: NEMOCLAW_DASHBOARD_BIND opt-in incl. negative cases (#3259). - whatsapp compact QR: package shape-detection + terminal-only small (#4522). Guard the recurrence: scripts/checks/no-unit-blocks-in-live-e2e.ts bans the vitest it(...) primitive inside test/e2e/live/** (those blocks never run on PR CI). Relocate the two existing offenders — the skill-agent and messaging-compatible-endpoint classifier blocks — into importable test/e2e/support modules with PR-collected unit tests; the live tests import them unchanged. whatsapp-qr-compact.ts: minimal behavior-preserving refactor to export the pure shape-detection/patch helpers (the preload still auto-installs on require); tsconfig.runtime-preloads.json excludes the new co-located test from the shipped preload build. SKIP=test-cli: the full cli+integration vitest hook trips on pre-existing macOS bash 3.2 failures in untouched shell-harness suites (select/set -u); CI runs bash 5.x green. Every new/changed file here was verified green individually, and the checks registry + budget + gitleaks + typecheck pass. Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
…st it (#6086) ## Summary Two regressions from #5874 (fixed in #6065) only surfaced in **live E2E targets that don't run on PR CI**. This PR closes that class of gap: it audits the live suite for behavior-critical assertions that are cheaply mockable, backfills them as fast units that run on **every** PR, and adds a guard so pure-unit blocks can't hide in live files again. ## What's here **1. The two direct #6065 regression fences** (mocked shell-units) - `reconcile`: an explicit `NEMOCLAW_MODEL_OVERRIDE` survives a divergent gateway model and the stale in-file fallback; normal drift-correction still runs when unset. - `guard recovery`: the restore warning is mirrored into `_NEMOCLAW_GATEWAY_LOG` (the marker the crash-loop E2E polls), and stays silent when the chain is healthy. **2. High-priority mockable backfill** (security/recovery class) - ollama-auth-proxy: Bearer enforcement, no `/api/tags` bypass (#3338), header stripping, non-ASCII auth no-crash (#4820), backend 502. - `config get`: credential redaction + `gateway`-key omission (the `nvapi-` regression class). - device approval policy: scope-upgrade allowlist gate, gateway-env stripping, recover-failed rejection paths (#4462). - shields audit JSONL: credentials redacted before persistence. - hermes env secret boundary: value-shape (not key-name) discriminator; raw secrets rejected without echoing. - dashboard bind: `NEMOCLAW_DASHBOARD_BIND` opt-in incl. negative cases (#3259). - whatsapp compact QR: package shape-detection + terminal-only `small` (#4522). **3. Medium/low backfill** - ollama token-file lifecycle (0600 / persisted / divergent-repair); extra-placeholder-keys canonical placeholder + accepted-keys breadcrumb; hermes `remove_stale_gateway_file` symlink-safety; token-rotation selective-rebuild naming; `_validate_port` fail-closed; snapshot `help` branch. **4. Regression guard** - `scripts/checks/no-unit-blocks-in-live-e2e.ts` bans the vitest `it(...)` primitive inside `test/e2e/live/**` (that glob is uncollected on PR CI, so such blocks never run). Wired into the checks registry with its own unit test. - Relocated the existing offenders (skill-agent + messaging-compatible-endpoint classifier blocks, plus the bare-`test(` unit cases in common-egress + openclaw-inference-switch) into importable `test/e2e/support` modules with PR-collected tests; the live tests import them unchanged. ## Notes - Minimal behavior-preserving refactor to `whatsapp-qr-compact.ts` to export its pure helpers (the preload still auto-installs on require); `tsconfig.runtime-preloads.json` excludes the new co-located test from the shipped preload build. - `nemoclaw-start.sh`: made two possibly-empty-array iterations bash-3.2-safe via the existing `"${arr[@]+...}"` idiom so the shell-unit harnesses run on stock macOS bash. - **Deferred (2 low-value items):** the install.sh "Resolved install ref:" log assertion and the OpenClaw anthropic plain-baseUrl assertion both land in legacy-budget-capped test files where the growth guardrail forbids bumping the budget; the anthropic contract is already covered on the Hermes side and enforced live on the OpenClaw side. ## Verification - Every new/changed test verified green individually across the `cli`, `integration`, and `e2e-support` projects. - `npm run checks` (incl. the new live-unit-block guard), test-file-size budget, gitleaks, and CLI typecheck all pass. - The full `test-cli` pre-commit hook was skipped locally only because it trips on **pre-existing** macOS bash 3.2 failures in untouched shell-harness suites (`select`/`set -u`); CI runs bash 5.x, where they are green. --- Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved startup robustness for environment parsing and background launch behavior. * WhatsApp compact-QR rendering more consistently uses the compact “terminal” style. * Dashboard remote bind activates only when explicitly opted in via `NEMOCLAW_DASHBOARD_BIND=0.0.0.0`. * Tightened audit/config redaction to prevent secret leakage and omit gateway details. * **Tests** * Expanded coverage for guard-chain recovery warnings, model override precedence, Hermes env-boundary hardening, and proxy/policy correctness. * **Chores** * Added a CI safeguard to prevent unit-test primitives from being included in live E2E tests. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
## Summary Follow-up to merged PR #6065 to address the unaddressed PR Review Advisor warnings: - Adds safe gateway-log append helper for PID 1/root recovery warnings; refuses symlink/non-regular/replaced log targets instead of appending through them. - Adds guard-chain recovery tests proving regular log append still works and unsafe targets are refused. - Adds focused reconciliation coverage proving `NEMOCLAW_MODEL_OVERRIDE` wins over conflicting gateway state, plus no-override reconciliation still runs. ## Test plan - [x] `npx vitest run --project integration test/nemoclaw-start-reconcile.test.ts test/nemoclaw-start-guard-recovery.test.ts` Refs #6065 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Hardened gateway log writing to refuse unsafe targets (non-regular, swapped, invalid), while sanitizing and newline-terminating log lines in UTF-8. * Recovery now records restoration warnings in the gateway log when safe, without blocking startup if logging fails. * Reconcile behavior improved: explicit model overrides are preserved; otherwise configuration is reconciled from the live gateway model. * **Tests** * Expanded gateway recovery coverage with regular/symlink/directory/missing log scenarios, validating behavior via gateway-log contents. * Added reconcile tests for overridden vs non-overridden behavior. * Updated gateway health watchdog tests to direct critical logging to controlled per-test log files. <!-- end of auto-generated comment: release notes by coderabbit.ai --> ## Advisor disposition / E2E evidence PR Review Advisor `PRA-1` / `PRA-2` runtime-validation warnings are resolved by the combination of in-PR helper hardening coverage plus live E2E evidence: - Source boundary: `append_openclaw_gateway_log_line` is the permanent PID 1 append boundary and hardcodes canonical `/tmp/gateway.log`; it intentionally refuses symlink, non-regular, replaced, or missing log targets rather than honoring inherited path seams. - Source-fix constraint: startup remains responsible for pre-creating canonical `/tmp/gateway.log`; recovery/watchdog append paths must not create a missing log because creation at recovery time would re-open the unsafe target/ownership ambiguity this PR removes. - Regression/unit coverage: `test/nemoclaw-start-guard-recovery.test.ts` covers regular append, symlink refusal, non-regular refusal, missing-log no-create, replaced-target refusal, inherited env ignored, and newline sanitization. `test/nemoclaw-start-gateway-health.test.ts` covers watchdog CRITICAL breadcrumb append through the same safe helper. - Runtime validation: current-head E2E revalidation passed on `9cda35bdaa6d426d160d262ac35c99c7449c1560`: run https://github.com/NVIDIA/NemoClaw/actions/runs/28874860688 passed required jobs `gateway-guard-recovery` and `issue-2478-crash-loop-recovery`, and run https://github.com/NVIDIA/NemoClaw/actions/runs/28874896411 passed required target `ubuntu-repo-cloud-openclaw`. The earlier same-PR artifact for `issue-2478-crash-loop-recovery` showed the real sandbox respawn path emitted `[gateway-recovery] WARNING ... (#2478/#2701)` and mirrored the same line as `[gateway-log:] [gateway-recovery] WARNING ...`, proving the real PID 1 startup/recovery lifecycle pre-created and preserved the canonical gateway log before append. - Removal condition: keep the helper-level contract tests until gateway recovery/watchdog logging is no longer routed through `/tmp/gateway.log` or the startup ownership contract is replaced by a different diagnostics channel with equivalent live E2E coverage. --------- Signed-off-by: cjagwani <cjagwani@nvidia.com> Co-authored-by: cjagwani <cjagwani@nvidia.com>
Summary
Two E2E regressions introduced by #5874:
runtime-overrides:reconcile_agent_model_with_providerran afterapply_model_overrideand overwroteagents.defaults.model.primarywith aninference/-prefixed variant becausequalify()unconditionally prependsinference/. The comment at the call site already documented that the explicit override should win; this adds the guard to enforce it.issue-2478-crash-loop-recovery:restore_openclaw_runtime_guard_chainwrote its warning only to stderr. The new host-mediated recovery path (docker exec … nemoclaw-gateway-control recover) runs inside PID 1, not a fresh sandbox exec, so stderr never reached/tmp/gateway.log. Mirrors the warning to the gateway log file using the same_NEMOCLAW_GATEWAY_LOGseam the watchdog already uses.Test plan
runtime-overridesE2E target — expectprimaryModel(modelOverride)to return"anthropic/claude-sonnet-4-6"(not"inference/anthropic/…")issue-2478-crash-loop-recoveryE2E target — expect[gateway-recovery] WARNING: .*restoring library guards from packaged preloadsto appear in/tmp/gateway.logSigned-off-by: Preksha Vijayvargiya prekshiv@nvidia.com
🤖 Generated with Claude Code
Summary by CodeRabbit