fix(onboard): fall back on a negative timeout or poll override - #7891
Conversation
`envInt` backs the poll counts, poll intervals and readiness budgets used across onboarding and gateway recovery. It clamped a negative override to 0 while falling back to the caller's default for every other unusable value, so the same function answered two kinds of invalid input in opposite ways -- and picked the most damaging reading for one of them: 0 empties a poll loop, zeroes a readiness budget, and turns NEMOCLAW_GATEWAY_START_TIMEOUT into a timeout that expires before its first attempt. Treat a negative the way the sibling readNonNegativeNumberEnv already does and fall back. An explicit 0 is unchanged, so callers that read it as "disabled" or clamp it upward themselves keep their meaning; only input that was never valid changes. One caller already compensated with a local Math.max(1, ...); the rest inherited the hazard. Refs #7881 Signed-off-by: Yanyun Liao <yanyunl@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)
📝 WalkthroughWalkthrough
ChangesTimeout environment validation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit e50accf in the TypeScript / code-coverage/cliThe overall coverage in commit e50accf in the Show a code coverage summary of the most impacted files.
Updated |
|
🌿 Preview your docs: https://nvidia-preview-pr-7891.docs.buildwithfern.com/nemoclaw |
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 `@src/lib/onboard/env-int.test.ts`:
- Around line 60-62: Strengthen the test around envInt by supplying a
non-default value for NEMOCLAW_TEST_KNOB in the custom environment map and
asserting that value is returned. Ensure the assertion distinguishes the
supplied map from process.env, using a conflicting process value only if it can
be set and restored deterministically.
🪄 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: f90a0855-09f8-4de0-89e7-3b7e8c5c34b2
📒 Files selected for processing (4)
docs/inference/configure-inference-timeouts.mdxsrc/lib/onboard/env-int.test.tssrc/lib/onboard/env.tssrc/lib/onboard/sandbox-readiness-tracing.test.ts
PR Review Advisor — InformationalAdvisor assessment: Informational / low confidence Model lanes
1 additional E2E selection from the second opinionAdvisory only. The primary lane did not select these E2E jobs or targets.
Second-opinion E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
…onment The custom-environment case asserted the fallback while passing an empty map. That passes even if `envInt` ignores the supplied map and reads `process.env`, because the key is absent there too, so the assertion proved nothing about the precedence its title claims. Stub a conflicting process value, then assert both directions: a supplied value is read, and an absent key falls back rather than picking up the process value. Teardown runs in a `finally` so a failing expectation cannot leak the stub. Verified by mutation: rewriting `envInt` to read `process.env[name]` now fails this test, which it did not before. Signed-off-by: Yanyun Liao <yanyunl@nvidia.com>
cjagwani
left a comment
There was a problem hiding this comment.
Approved exact head b7055880323b0a352537bd0f567d0a74ed4e3ee2.
envInt now treats negative finite values consistently with other invalid inputs while preserving explicit zero and existing rounding semantics. The focused table tests cover negative, non-finite, empty, zero, fractional, and supplied-environment precedence cases; the debounce test verifies the affected caller contract. All 53 exact-head checks are green, DCO and commit verification pass, and no unresolved major/critical findings remain. Stale-base-only failure is waived because GitHub reports MERGEABLE/conflict-free.
<!-- markdownlint-disable MD041 --> ## Summary Adds the canonical dated `v0.0.101` changelog entry that was missing when the release tag was cut. This post-release recovery records the shipped behavior on current `main` without changing or replacing the existing tag. ## Changes - Add `docs/changelog/2026-08-03.mdx` with the exact `## v0.0.101` heading, release summary, detailed behavior changes, support boundaries, and links to durable documentation. - [#7317](#7317) -> `docs/changelog/2026-08-03.mdx`: Records experimental OpenClaw Google Chat support and its restricted credential and webhook boundary. - [#7715](#7715) -> `docs/changelog/2026-08-03.mdx`: Records strict onboarding recovery state and authoritative resume identity. - [#7749](#7749) -> `docs/changelog/2026-08-03.mdx`: Records the provider-neutral policy seam and unchanged runtime support boundary. - [#7817](#7817) -> `docs/changelog/2026-08-03.mdx`: Records preserved Hermes home-channel assignments across rebuilds. - [#7820](#7820) -> `docs/changelog/2026-08-03.mdx`: Records the SSH-session status field correction. - [#7847](#7847) -> `docs/changelog/2026-08-03.mdx`: Records fail-closed credential filtering for migration and rebuild backups. - [#7870](#7870) -> `docs/changelog/2026-08-03.mdx`: Records sandbox-qualified in-sandbox host command hints. - [#7875](#7875) -> `docs/changelog/2026-08-03.mdx`: Records Microsoft Teams stop and start E2E coverage. - [#7885](#7885) -> `docs/changelog/2026-08-03.mdx`: Records Hermes managed gateway detection in status. - [#7889](#7889) -> `docs/changelog/2026-08-03.mdx`: Records policy-authenticated HTTPS Pin Runtime route revocation. - [#7891](#7891) -> `docs/changelog/2026-08-03.mdx`: Records default fallback for negative timeout and polling overrides. - [#7993](#7993) -> `docs/changelog/2026-08-03.mdx`: Records correct sibling detection during uninstall. - [#7995](#7995) -> `docs/changelog/2026-08-03.mdx`: Records absent configuration-hash handling before shields lock. - [#8001](#8001) -> `docs/changelog/2026-08-03.mdx`: Records the dormant atomic managed workload replacement foundation. - [#8029](#8029) -> `docs/changelog/2026-08-03.mdx`: Records repository terminology review in PR Review Advisor. - [#8031](#8031) -> `docs/changelog/2026-08-03.mdx`: Records provider-neutral managed snapshot authority. - [#8032](#8032) -> `docs/changelog/2026-08-03.mdx`: Records immutable managed clone handoff contracts. - [#8034](#8034) -> `docs/changelog/2026-08-03.mdx`: Records the dormant provider-owned clone transaction surface. - [#8035](#8035) -> `docs/changelog/2026-08-03.mdx`: Records the dormant Hermes managed clone broker boundary. - [#8036](#8036) -> `docs/changelog/2026-08-03.mdx`: Records the dormant transactional managed bootstrap boundary. - [#8037](#8037) -> `docs/changelog/2026-08-03.mdx`: Records dormant Docker bootstrap primitives and the unchanged provider support boundary. - [#8070](#8070) -> `docs/changelog/2026-08-03.mdx`: Records consolidated sandbox resource-limit E2E coverage. - [#8071](#8071) -> `docs/changelog/2026-08-03.mdx`: Records escaped and bounded CLI validation diagnostics. - [#8081](#8081) -> `docs/changelog/2026-08-03.mdx`: Records bounded linear snapshot Base64 validation. - [#8085](#8085) -> `docs/changelog/2026-08-03.mdx`: Records commit-bound workflow approval for eligible same-repository maintainers. - [#8088](#8088) -> `docs/changelog/2026-08-03.mdx`: Records Hermes managed-policy E2E selection. - [#8090](#8090) -> `docs/changelog/2026-08-03.mdx`: Records pinned CI search-tool provisioning. - [#8106](#8106) -> `docs/changelog/2026-08-03.mdx`: Records fallback from failed managed OpenShell gateway startup. - [#8107](#8107) -> `docs/changelog/2026-08-03.mdx`: Records Hermes adapter lifecycle E2E selection. - [#8128](#8128) -> `docs/changelog/2026-08-03.mdx`: Records the dormant transactional Docker bootstrap adapter and rollback authority. - [#8140](#8140) -> `docs/changelog/2026-08-03.mdx`: Records Slack conflict scope across independent OpenShell gateways. - [#8147](#8147) -> `docs/changelog/2026-08-03.mdx`: Records completion of durable v0.0.100 documentation audit follow-ups. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [x] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [ ] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [x] Tests not applicable — justification: This documentation-only recovery does not change executable behavior. - [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: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `docs-updated` - Evidence: Independently reviewed `docs/changelog/2026-08-03.mdx` at commit `0bebe1f568e3dc85cf410aac1dfb8f8830070b85`. Its blob is `82887920f9720eafd75db6b2271c35f7477edb9b`. The entry follows the writing guide, controlled terminology, changelog structure, MDX SPDX format, literal CLI-name rule, and root-absolute route requirements. It accurately records the `v0.0.100...v0.0.101` release range, Announcement #8162, accepted scope boundaries, and shipped security behavior. There are no code samples. Focused changelog tests and the documentation build pass for this commit. - Agent: Codex Desktop independent documentation writer <!-- docs-review-head-sha: 0bebe1f --> <!-- docs-review-agents-blob-sha: 3dd7c24 --> ## Security Review - Result: `PASS` - Reviewed commit: `0bebe1f568e3dc85cf410aac1dfb8f8830070b85` - Base commit: `643a4ab8b5f583d8555192a37927268b26022c51` - Findings: None. - Secrets and credentials: `PASS`. No credential values or secret files are present. - Input validation and data sanitization: `PASS`. No executable input path changes. - Authentication and authorization: `PASS`. No identity or permission logic changes. - Dependencies and third-party libraries: `PASS`. No dependency changes. - Error handling and logging: `PASS`. No runtime path changes; diagnostic-security claims are precise. - Cryptography and data protection: `PASS`. No implementation changes. - Configuration and security controls: `PASS`. No configuration, container, port, or HTTP changes. - Security testing: `PASS`. No coverage is removed; the entry records shipped test and security behavior. - System security: `PASS`. No runtime control changes; dormant and non-activation boundaries are explicit. - Agent: Codex Desktop independent security reviewer ## Verification - [ ] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub — verification is pending after commit `0bebe1f568e3dc85cf410aac1dfb8f8830070b85` is pushed. - [ ] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run validate:pr` passed after refreshing `origin/main` when hooks were skipped or unavailable — commit hooks passed; pre-push is pending. - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — tests are not applicable to this documentation-only recovery. - [x] Applicable broad gate passed — not applicable to this documentation-only recovery. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, credentials, or private keys are added by this diff. - [ ] `npm run docs` builds without warnings (doc changes only) — GitHub documentation checks are pending. - [x] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) — independent documentation review passed. - [x] New doc pages include SPDX header and frontmatter (new pages only) — the native changelog entry uses the required parser-safe MDX SPDX comment and intentionally has no frontmatter. GitHub CI is authoritative. Focused changelog tests and `npm run docs` passed after the merge refresh. --- Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added experimental Google Chat support. * Improved runtime and session status visibility. * Added onboarding recovery and persistence safeguards. * Added snapshot validation and dormant managed-workload support. * **Bug Fixes** * Improved backup sanitization, route handling, and gateway reliability. * **Documentation** * Added the v0.0.101 changelog and related updates. * **Tests** * Expanded end-to-end coverage and strengthened trusted CI validation. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> Signed-off-by: Carlos Villela <cvillela@nvidia.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com> Co-authored-by: Senthil Ravichandran <senthilr@nvidia.com>
Summary
envIntclamped a negative override to0while falling back to the caller's default forevery other unusable value. Zero is the most damaging reading available for the knobs it
backs — an empty poll loop, a zero-second readiness budget, a timeout that expires before
its first attempt. This makes a negative fall back like any other invalid input.
Closes #7881.
Scope
#7881 has been narrowed to this single defect. Two neighbouring observations that surfaced while
resolving the call sites are behaviour changes to sandbox recovery timing rather than input
validation, and are tracked separately in #7893 so they can be verified against a real cold start
without holding up a zero-risk validation fix:
GATEWAY_RECOVERY_WAIT_DEFAULT_SECONDSis bypassed by anexplicit
timeoutSecondsat four of its five production call sites;NEMOCLAW_GATEWAY_RECOVERY_WAIT_SECONDShas opposite precedence in the two functions that readit.
Neither is touched here. This PR changes one function in
src/lib/onboard/env.ts.Analysis
src/lib/onboard/env.ts:Observed behaviour, next to the sibling helper
readNonNegativeNumberEnv(
src/lib/actions/sandbox/process-recovery.ts):envIntreadNonNegativeNumberEnv-130(fallback)abc30(fallback)30(fallback)000abcand-1are both unusable, but only one reaches the default. The other becomes0,and
0is exactly the value these knobs cannot survive:NEMOCLAW_HEALTH_POLL_COUNT=0→ zero polls, so readiness gives up before it looks once.NEMOCLAW_HEALTH_POLL_INTERVAL=0→ no sleep between polls.NEMOCLAW_GATEWAY_START_TIMEOUT=0→0 * 1000, a timeout already expired at its first check.NEMOCLAW_SANDBOX_READY_TIMEOUT=0→ a zero-second readiness budget.The existing test suite already contained the inconsistency side by side:
sandbox-readiness-tracing.test.tsasserted"abc"→30and, twelve lines later,"-5"→1— the latter only because that one caller wraps the result in a local clamp. That localclamp is the second symptom:
src/lib/onboard/docker-gpu-supervisor-reconnect.tsguards itsown call with
Math.max(1, envInt(...)), and the other ~20 call sites do not.Fix
Reject a negative the way every other unusable value is already rejected, matching what the
sibling helper has always done:
An explicit
0is deliberately unchanged. Callers that read0as "disabled", or clampit upward themselves, keep their exact current meaning — whether
0should stay meaningfulis part of Q3 and is not decided here. Only input that was never valid behaves differently.
Whole-class review. Three parsers read these overrides.
readNonNegativeNumberEnvalready falls back on a negative. The inline parser in
connect.tsalready rejects<= 0and warns.
envIntwas the only one that did not, so the class is this single function andevery call site inherits the fix; no call site needed its own edit. The local
Math.max(1, ...)indocker-gpu-supervisor-reconnect.tsis left in place — it is thatcaller's own floor, not a workaround this change makes redundant.
Tests. A new focused suite covers negatives (
-1,-30,-0.4,-1e3), the non-finiteinputs that already fell back (
abc,NaN,Infinity,-Infinity), unset and empty, therounding contract (
0.4→0,2.6→3), the supplied-env-map path, and a regressionlock that an explicit
0still returns0. The debounce test that encoded the old negativebehaviour is updated to assert the fallback, next to a comment explaining why it now matches
the
"abc"case directly above it.Verification
npx vitest run src/lib/onboard/— 3654 passed. Two pre-existing failures inpreflight.test.ts(checkPortAvailable) are unrelated to this change and reproduce on aclean
maincheckout: they need a free local port that this machine does not have.Changes
src/lib/onboard/env.ts: fall back on a negative override instead of clamping it to zerosrc/lib/onboard/env-int.test.ts: new focused suite for the override contractsrc/lib/onboard/sandbox-readiness-tracing.test.ts: assert the fallback for a negative debounce overridedocs/inference/configure-inference-timeouts.mdx: state the rounding and invalid-value contract for these settingsType of Change
Verification
npx prek run --all-filespassesnpm testpasses (touched files at minimum)make docsbuilds without warnings (doc changes only)AI Disclosure
Signed-off-by: Yanyun Liao yanyunl@nvidia.com
Summary by CodeRabbit