fix(onboard): report an ignored NEMOCLAW_REASONING instead of silently keeping the recorded flag - #7466
Conversation
…y keeping the recorded flag A resumed onboard replays the sandbox's recorded selection, so the stored compatible-endpoint reasoning flag wins over ambient NEMOCLAW_REASONING by design (NVIDIA#5735). That precedence was silent: issue NVIDIA#7462 reports exporting NEMOCLAW_REASONING=true and re-running onboarding three ways, with every run keeping reasoning=false and printing nothing about the recorded value or the command that re-reads the variable. Report the mismatch on the resume path and name the recreate command, and say in the capability docs that resume and rebuild replay the recorded flag. Signed-off-by: Souvik Ghosh <138186578+souvikDevloper@users.noreply.github.com>
📝 WalkthroughWalkthroughThe onboarding flow detects conflicting ChangesReasoning replay handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Onboarding
participant ReasoningHelper
participant CompatibleEndpoint
Onboarding->>ReasoningHelper: Compare recorded and ambient NEMOCLAW_REASONING
ReasoningHelper-->>Onboarding: Return mismatch message or null
Onboarding->>CompatibleEndpoint: Apply recorded reasoning value
CompatibleEndpoint-->>Onboarding: Return configured inference state
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Review Advisor — InformationalAdvisor assessment: Informational / medium confidence Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: 1 optional E2E recommendation
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/onboard/reasoning-mode.ts (1)
27-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep this helper independent of
process.env.Line 30 makes a domain helper read global state through a default parameter. Require
envand haveprovider-inference.tspassprocess.env; the unit tests already provide a controlled environment.Proposed refactor
export function describeIgnoredReasoningEnv( storedValue: string | null | undefined, cliName: string, - env: NodeJS.ProcessEnv = process.env, + env: NodeJS.ProcessEnv, ): string | null {const ignoredReasoning = describeIgnoredReasoningEnv( compatibleEndpointReasoning, deps.cliName(), + process.env, );As per path instructions, “keep domain/policy helpers pure (no process/env reads inside pure logic; use injected inputs for testability).”
🤖 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 `@src/lib/onboard/reasoning-mode.ts` around lines 27 - 30, Make the env parameter of describeIgnoredReasoningEnv required, removing its process.env default so the helper remains independent of global state. Update provider-inference.ts to explicitly pass process.env at each call site, while preserving the existing controlled env arguments in unit tests.Source: Path instructions
🤖 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 `@docs/inference/configure-model-capabilities.mdx`:
- Around line 51-53: Update the documentation paragraph covering recorded
reasoning values and mismatch handling so it states that both resumed onboarding
and `nemoclaw <sandbox-name> rebuild` report an explicit NEMOCLAW_REASONING
conflict using the recorded value and recreate command, rather than limiting
this behavior to resumed onboarding.
---
Nitpick comments:
In `@src/lib/onboard/reasoning-mode.ts`:
- Around line 27-30: Make the env parameter of describeIgnoredReasoningEnv
required, removing its process.env default so the helper remains independent of
global state. Update provider-inference.ts to explicitly pass process.env at
each call site, while preserving the existing controlled env arguments in unit
tests.
🪄 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: fd238032-c070-484a-b671-a5937cf9e043
📒 Files selected for processing (5)
docs/inference/configure-model-capabilities.mdxsrc/lib/onboard/machine/handlers/provider-inference.test.tssrc/lib/onboard/machine/handlers/provider-inference.tssrc/lib/onboard/reasoning-mode.test.tssrc/lib/onboard/reasoning-mode.ts
provider-inference.test.ts sits at 1493 of the 1500-line default test-size budget, so the two new cases pushed it to 1553 and failed the codebase-growth-guardrails check. Move them into provider-inference-reasoning-resume.test.ts, matching the sibling split already used for route containment, recovery gating, and Station Express. No test behavior changes and provider-inference.test.ts returns to its base content. Signed-off-by: Souvik Ghosh <138186578+souvikDevloper@users.noreply.github.com>
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/machine/handlers/provider-inference-reasoning-resume.test.ts`:
- Around line 37-43: Update the diagnostic-log assertion for the
NEMOCLAW_REASONING case in the test to verify that the emitted message includes
the recorded value “false,” not only the command guidance. Keep the existing
compatibleEndpointReasoning precedence assertion unchanged.
🪄 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: 4caaf10c-9ad6-4bc8-9b48-4150b93045bf
📒 Files selected for processing (1)
src/lib/onboard/machine/handlers/provider-inference-reasoning-resume.test.ts
…value The handler test verified internal precedence through the returned flag but would still pass if the emitted message omitted the recorded value. Assert the message contains it. Raised by CodeRabbit on PR NVIDIA#7466. Signed-off-by: Souvik Ghosh <138186578+souvikDevloper@users.noreply.github.com>
|
Pushed
|
| Workflow | Status |
|---|---|
| CI / Pull Request | action_required |
| CI / Commit Lint | action_required |
| CI / DCO Check | action_required |
| CI / Documentation Writer Review | action_required |
| Docs / CLI Parity, Docs / Fern Preview | action_required |
| Security / Code Scanning, Security / Installer Hash Check | action_required |
| E2E / macOS | action_required |
The runs that did execute are green: Codebase Growth Guardrails, Require Maintainer Edits, Assign Linked Issue Author, both PR review advisors, and CodeRabbit.
E2E / PR Gate Coordination is consequently sitting at pending waiting for PR CI, which is the shape that ends in "Timed out waiting for the trusted E2E verdict" if nothing approves the runs first. That timeout would be a symptom, not a defect in this change.
This needs someone with write access to approve the workflow runs on this head; I cannot clear it from the fork side. @cv could you kick these off, or point me at what is holding them.
Signed-off-by: Souvik Ghosh 138186578+souvikDevloper@users.noreply.github.com
|
Tested the documented procedure from docs/inference/configure-model-capabilities.mdx export NEMOCLAW_REASONING=true The recreate performed a full sandbox image build and passed all three GPU proofs, openclaw.json → "reasoning": false Also tried: installer with NEMOCLAW_REASONING=true, So on this platform there appears to be no working path to set reasoning:true |
prekshivyas
left a comment
There was a problem hiding this comment.
Reviewed at 84ad567. The change is output-only: recorded reasoning precedence, credentials, routing, and persisted state remain unchanged. Focused tests and docs validation pass; no unresolved review findings. Required E2E remains independently enforced.
<!-- markdownlint-disable MD041 --> ## Summary Add the canonical `docs/changelog/2026-07-25.mdx` release entry with the exact `## v0.0.96` heading. The entry reconciles all 90 first-parent commits since v0.0.95 with all 92 merged PRs in the live `v0.0.96` label ledger and groups the user-visible changes by operator journey. ## Changes - Add the parser-safe dated MDX changelog entry for v0.0.96 with root-absolute links to the focused user guides. - Source summary: - [#7194](#7194) -> `docs/changelog/2026-07-25.mdx`: Document persistent baseline network policy exclusions and their inspection, rebuild, and snapshot behavior. - [#7188](#7188), [#7427](#7427), and [#7546](#7546) -> `docs/changelog/2026-07-25.mdx`: Document DNS-backed HTTPS inference routing, keyless loopback endpoints, and provider-marker isolation. - [#7238](#7238) -> `docs/changelog/2026-07-25.mdx`: Document blueprint sandbox and provider identifier validation before state writes or OpenShell calls, with bounded terminal-safe rejection previews. - [#7319](#7319), [#7274](#7274), [#7528](#7528), [#7353](#7353), and [#7560](#7560) -> `docs/changelog/2026-07-25.mdx`: Document the managed default gateway service, onboarding readiness, and container-runtime identity safeguards. - [#7349](#7349), [#7498](#7498), [#7406](#7406), [#7196](#7196), [#7559](#7559), [#7421](#7421), [#7510](#7510), [#7295](#7295), and [#7565](#7565) -> `docs/changelog/2026-07-25.mdx`: Document gateway-scoped status, lifecycle diagnostics, managed MCP recovery, delete-edge safeguards, and fail-closed CLI prompt and command output. - [#7591](#7591) -> `docs/changelog/2026-07-25.mdx`: Document opt-in authenticated MCP tool-name discovery, its bounded and names-only contract, probe interaction, and rebuild requirement. - [#7305](#7305), [#7480](#7480), [#7471](#7471), [#7365](#7365), and [#7541](#7541) -> `docs/changelog/2026-07-25.mdx`: Document installer version checks, version-tag reporting, license guidance, WSL Ollama selection, and DGX Station vLLM detection. - [#7482](#7482), [#7466](#7466), [#7208](#7208), [#7434](#7434), and [#7586](#7586) -> `docs/changelog/2026-07-25.mdx`: Document Ollama resource details, reasoning precedence, Hermes onboarding behavior, and preserved managed Hermes BuildKit failures. - [#6830](#6830), [#7492](#7492), [#7563](#7563), and [#7582](#7582) -> `docs/changelog/2026-07-25.mdx`: Document the authoritative OpenClaw production lock, fixed managed-image dependencies, immutable Hermes base adoption, and Hermes image-size reduction. - [#7505](#7505), [#7530](#7530), [#7547](#7547), [#7508](#7508), [#7548](#7548), [#7549](#7549), [#7537](#7537), [#7534](#7534), [#7515](#7515), [#7511](#7511), [#7551](#7551), [#7562](#7562), [#7575](#7575), [#7496](#7496), [#7594](#7594), [#7595](#7595), and [#7599](#7599) -> `docs/changelog/2026-07-25.mdx`: Summarize release validation, transient and bounded dispatch reconciliation, exact pre-tag qualification, identity revalidation, npm-audit retry, sharding, image reuse, timeout, telemetry, and workflow-hardening changes. - Reconciled without separate changelog prose: - [#7539](#7539), [#7526](#7526), [#7507](#7507), [#7506](#7506), [#7519](#7519), [#7516](#7516), [#7396](#7396), [#7254](#7254), [#7583](#7583), [#7596](#7596), and [#7598](#7598): Test-harness or fixture-only changes. - [#7403](#7403), [#7161](#7161), [#6877](#6877), [#7531](#7531), [#7525](#7525), [#7522](#7522), [#7536](#7536), [#7552](#7552), [#7566](#7566), [#7553](#7553), [#7561](#7561), [#7577](#7577), [#7569](#7569), [#7585](#7585), [#7584](#7584), [#7592](#7592), [#7580](#7580), [#7571](#7571), [#7517](#7517), [#7589](#7589), [#7402](#7402), [#7558](#7558), [#7544](#7544), and [#7601](#7601): Dependency, internal recovery, validation, contributor-workflow, E2E optimization, telemetry, or CI trust changes with no separate user-facing release claim. - [#7556](#7556), [#7573](#7573), [#7576](#7576), and [#7578](#7578): Experimental repository-maintainer conflict automation with no canonical user documentation surface. ## 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 - [x] Existing tests cover changed behavior — justification: `test/changelog-docs.test.ts` validates dated changelog structure, version headings, and published links. - [ ] Tests not applicable — justification: - [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: Reviewed `docs/changelog/2026-07-25.mdx` at exact head `0f5dedb47` against 90 first-parent release commits and 92 merged PRs labeled `v0.0.96`. Verified parser-safe MDX SPDX, the exact version heading, literal CLI names, writing style, skip terms, all 20 root-absolute published links, and the accepted #7591 opt-in authenticated discovery bounds. #7544, #7599, and #7601 remain internal or CI-only release-ledger entries. Changelog tests passed 6/6, the docs build passed with 0 errors and two pre-existing Fern warnings, and `npm run check:diff` plus the final diff check passed. - Agent: Codex Desktop documentation-writer subagent <!-- docs-review-head-sha: 0f5dedb --> <!-- docs-review-agents-blob-sha: be20a09 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: - Station profile/scenario: - Result: - Supporting evidence: ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — `npx vitest run test/changelog-docs.test.ts`: 6/6 passed. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: Not applicable to this prose-only changelog entry. - [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) — the build passed with 0 errors and 2 existing Fern warnings; the published-route check passed. - [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) — native changelog files use the required parser-safe MDX SPDX comment and no frontmatter. --- Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Persistent network policy exclusions with consistent restore/exclusion reporting across rebuilds/snapshots. * Opt-in MCP tool discovery via `mcp status --tools` with bounded, redacted authenticated traffic. * Improved HTTPS inference switching for custom endpoints and refreshed onboarding/model menu details. * Refined OpenShell gateway defaults for port `8080`, including more reliable readiness checks. * **Bug Fixes** * Prevent incorrect provider/model restoration after compatible-provider update failures. * Preserve managed MCP state after exec loss and tighten gateway/doctor status scoping. * **Tests** * Stronger, fail-closed release validation with hardened evidence/artifact handoff and bounded timeouts/retries. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: Prekshi Vyas <prekshiv@nvidia.com>
Summary
A resumed onboard replays the sandbox's recorded selection, so the recorded compatible-endpoint reasoning flag wins over an ambient
NEMOCLAW_REASONINGby design (#5735). That precedence was silent. Now a resumed onboard that sees an explicitNEMOCLAW_REASONINGdisagreeing with the recorded value prints the recorded value and the command that re-reads the variable, instead of ignoring the variable without output.Related Issue
Refs #7462.
This addresses the reporter's first question — "whether the intended path to set this on an existing sandbox is a different command/flag" — and the silent no-op behind it. It does not change the idle-timeout behavior described in the second half of that issue; see Scope below.
Changes
src/lib/onboard/reasoning-mode.ts: adddescribeIgnoredReasoningEnv(storedValue, cliName, env). Pure — it returns the message ornulland touches no environment. It returnsnullunless the recorded value andNEMOCLAW_REASONINGboth normalize to a flag and disagree, so an unset, invalid, or matching variable stays quiet.cliNameis a required parameter rather than a defaulted one so the Deep Agents CLI names its own binary in the message.src/lib/onboard/machine/handlers/provider-inference.ts: on the resume path, emit that message before callingconfigureCompatibleEndpointReasoning. Order matters —configureCompatibleEndpointReasoningoverwritesprocess.env.NEMOCLAW_REASONINGwith the recorded value, so reading it afterwards can never see what the user asked for. The ternary became anif/elseto hold the two statements.docs/inference/configure-model-capabilities.mdx: state in Apply Capability Changes that onboarding records the flag, that a resumed onboard andrebuildreplay the recorded value, and what a resumed onboard reports on a mismatch.No behavior other than output changes: the recorded value still wins, and
nemoclaw onboard --fresh --name <sandbox> --recreate-sandboxis still the path that re-reads the variable.Scope
The issue also asks whether reasoning-token stream progress should reset the
LLM idle timeout (120s)detector. That timer is OpenClaw's — no NemoClaw source, script, or doc emits or configures that message — so it is not addressed here and needs a separate decision.The rebuild path already reports its own isolation of ambient selection env (
assessRebuildAmbientEnvinsrc/lib/actions/sandbox/rebuild-resume-preflight.ts), so it is left unchanged.Type of Change
Quality Gates
Documentation Writer Review
docs-updateddocs/inference/configure-model-capabilities.mdx; the exact-head review confirmed that resumed onboarding reports the mismatch while rebuild retains its existing generic ambient-environment notice. Focused tests, CLI/plugin builds, CLI type-checking, andnpm run docspassed.Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablereasoning-mode.test.tsandprovider-inference-reasoning-resume.test.ts: 9 passed.New tests:
src/lib/onboard/reasoning-mode.test.ts— four cases overdescribeIgnoredReasoningEnv: the mismatch message names the recorded value and the recreate command; the CLI name comes from the caller; both sides normalize before comparing (novs" YES "reports,yesvs1stays quiet); an unset, invalid, or unrecorded flag returnsnull.src/lib/onboard/machine/handlers/provider-inference.test.ts— a resumed compatible-endpoint sandbox recorded asreasoning=falseunderNEMOCLAW_REASONING=truelogs both the ignored value and the recreate command and still resolves tofalse; the same sandbox recorded astruelogs nothing.Environment caveats — this checkout is Windows, and the following failures reproduce on a clean
upstream/mainwith my commit stashed, so they are pre-existing and environment-specific, not caused by this change:provider-inference.test.ts > clears stale resumed state before writing a non-compatible artifact—O_NOFOLLOW is unavailable on this platform. This is the single failure in the targeted run above.npm run check:diff— theplatform-matrix-synchook andhadolintexit 127 (python3: command not found, hadolint not installed). Every other hook passed, including Biome format, Biome lint, SPDX headers, and shfmt.npm run docs— fails inscripts/generate-starter-prompt.mtswithdocs/resources/starter-prompt.md: use LF line endings, a CRLF artifact of the Windows checkout in a file this PR does not touch.I have not run
npm testornpm run checkin full; this change is not repo-wide and does not touch the test harness. CI is the authority on all of the above.Signed-off-by: Souvik Ghosh 138186578+souvikDevloper@users.noreply.github.com
Summary by CodeRabbit
NEMOCLAW_REASONINGin the environment conflicts with the recorded value, the recorded setting is used instead.nemoclaw ... --recreate-sandboxcommand for remediation.