Skip to content

fix(onboard): support reasoning-compatible endpoints - #5948

Merged
cv merged 9 commits into
mainfrom
codex/replace-3286-reasoning-endpoints
Jun 29, 2026
Merged

fix(onboard): support reasoning-compatible endpoints#5948
cv merged 9 commits into
mainfrom
codex/replace-3286-reasoning-endpoints

Conversation

@cv

@cv cv commented Jun 28, 2026

Copy link
Copy Markdown
Collaborator

Summary

Custom OpenAI-compatible providers can opt into reasoning-mode validation through NEMOCLAW_REASONING, preserving that choice through provider handoff and resume. This clean-history replacement supersedes #3286 while retaining Deepak Jain's attribution and all fixes from its review.

Related Issue

Fixes #3279.
Supersedes #3286.

Changes

  • Normalize common reasoning aliases and persist explicit set and clear operations in onboarding session state.
  • Skip Responses API, tool-call, and streaming probes that reasoning-only compatible endpoints reject.
  • Retry length-limited reasoning_content and reasoning smoke responses with a larger output budget, then fail clearly if final content remains empty.
  • Carry reasoning state through provider selection, resume, flow context, debug summaries, and sandbox creation.
  • Add focused source and package-contract coverage for aliases, probe selection, persistence, null clearing, resume, and response fallback behavior.
  • Keep src/lib/onboard.ts net-neutral and synchronize generated platform documentation citations.
  • Validation: CLI build and typecheck, focused source tests, the package-contract test, 18 platform-doc generator tests, full commit hooks, and push hooks pass. npm run docs reports zero errors and two pre-existing Fern warnings.

Type of Change

  • 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)

Quality Gates

  • Tests added or updated for changed behavior
  • Existing tests cover changed behavior — justification:
  • Tests not applicable — justification:
  • 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: maintainer salvage review completed; all historical CodeRabbit findings and the resolved thread in fix: support reasoning compatible endpoints (Fixes #3279) #3286 were incorporated and revalidated.
  • Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue:

Verification

  • PR description includes the DCO sign-off declaration and every commit appears as Verified in GitHub
  • 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)
  • Quality Gates section completed with required justifications or waivers
  • No secrets, API keys, or credentials committed
  • npm run docs builds without warnings (doc changes only)
  • Doc pages follow the style guide (doc changes only)
  • New doc pages include SPDX header and frontmatter (new pages only)

Signed-off-by: Carlos Villela cvillela@nvidia.com

Summary by CodeRabbit

  • New Features

    • Added a new reasoning mode for compatible OpenAI-style endpoints, with support for saving and restoring this setting during setup.
    • Compatible-endpoint checks now use a more tailored validation flow when reasoning mode is enabled.
  • Bug Fixes

    • Improved handling of resumed onboarding so stale reasoning settings are cleared when switching providers.
    • Increased the default token budget for compatible-endpoint sandbox smoke checks.
  • Documentation

    • Updated setup and platform docs to cover reasoning mode and refreshed reference links.

Supersedes the reviewed implementation in #3286 with clean, verified history.

Co-authored-by: Deepak Jain <deepujain@gmail.com>

Signed-off-by: Deepak Jain <deepujain@gmail.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@cv cv added the v0.0.70 label Jun 28, 2026
@cv cv self-assigned this Jun 28, 2026
@coderabbitai

coderabbitai Bot commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Adds NEMOCLAW_REASONING environment variable support for custom OpenAI-compatible (Option 3) endpoints. A new reasoning-mode.ts module manages the flag; compatibleEndpointReasoning is threaded through Session, OnboardFlowContext, ProviderInferenceState, and SetupNimSelectionState. Probe options adjust dynamically when reasoning is enabled, the smoke-check token budget increases to 512, and documentation is updated.

Changes

Compatible Endpoint Reasoning Mode

Layer / File(s) Summary
Reasoning-mode helpers
src/lib/onboard/reasoning-mode.ts, src/lib/onboard/reasoning-mode.test.ts
Adds normalizeReasoningFlag, configureCompatibleEndpointReasoning, and clearCompatibleEndpointReasoning to manage process.env.NEMOCLAW_REASONING, with unit tests for normalization, defaulting, and clearing.
Inference probe selection
src/lib/onboard/inference-selection-validation.ts, src/lib/onboard/inference-selection-validation.test.ts
validateCustomOpenAiLikeSelection reads NEMOCLAW_REASONING and conditionally disables tool-calling requirements, Responses probing, and streaming for compatible endpoints; new test covers the failing-probe reasoning branch.
Session persistence
src/lib/state/onboard-session.ts, src/lib/state/onboard-session.test.ts, src/lib/onboard/session-updates.ts
Session, SessionUpdates, and DebugSessionSummary gain compatibleEndpointReasoning; session creation, normalization, safe updates, debug output, and the update-input converter are extended, with tests for persistence and null clearing.
Remote model validator
src/lib/onboard/setup-nim-selection.ts, src/lib/onboard/setup-nim-selection.test.ts
SetupNimSelectionState gains compatibleEndpointReasoning; the custom branch of validateSelectedRemoteModel calls configureCompatibleEndpointReasoning and logs a warning when reasoning is "true", with test coverage.
Provider inference handler
src/lib/onboard/machine/handlers/provider-inference.ts, src/lib/onboard/machine/handlers/provider-inference.test.ts
ProviderSelectionResult, handler options, and result types gain compatibleEndpointReasoning; the handler initializes from session, restores or clears on resume, records on all session-update paths, and returns the field; tests cover fresh selection and stale-reasoning clearing.
Flow context and phase plumbing
src/lib/onboard/machine/flow-context.ts, src/lib/onboard/machine/core-flow-phases.ts, src/lib/onboard/machine/flow-context.test.ts, src/lib/onboard/machine/core-flow-phases.test.ts, src/lib/onboard/machine/flow-phases/*, src/lib/onboard/machine/flow-s*.test.ts, src/lib/onboard/machine/initial-flow-phases.test.ts, test/helpers/onboard-final-flow-phases.ts
OnboardFlowContext and ProviderModelSelectedContextUpdate gain compatibleEndpointReasoning; core phases pass it into handleProviderInferenceState and merge the result back; all context fixtures are updated to null.
onboard.ts wiring and session restore
src/lib/onboard.ts
setupNim return type changes to ProviderSelectionResult, threads compatibleEndpointReasoning through local state, wires configureCompatibleEndpointReasoning/clearCompatibleEndpointReasoning into handler creation and core dependencies, and restores the field from the persisted session.
Compatible-endpoint smoke budget
src/lib/onboard/compatible-endpoint-smoke.ts, src/lib/onboard/compatible-endpoint-smoke.test.ts
Default INITIAL_MAX_TOKENS raised from 256 to 512 to accommodate reasoning trace token usage; test assertion updated.
Package-contract integration test
test/package-contract/onboard/compatible-endpoint-reasoning.test.ts
End-to-end test with a fake curl binary verifies onboarding with NEMOCLAW_REASONING set selects a compatible provider, targets /chat/completions without streaming, and omits the reasoning-mode prompt.
Documentation and CI line references
ci/platform-matrix.json, docs/reference/platform-support.mdx, docs/inference/inference-options.mdx, test/generate-platform-docs.test.ts
Updates src/lib/onboard.ts line citations in CI matrix and platform docs; adds NEMOCLAW_REASONING env-var entry, bash example, and warning to inference-options docs; adds generated-docs test for the new entry.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 10.34% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: onboarding support for reasoning-compatible endpoints.
Linked Issues check ✅ Passed The changes implement runtime reasoning-mode support for Option 3 onboarding and session restore, matching #3279.
Out of Scope Changes check ✅ Passed The edits are aligned with the reasoning-endpoint fix and supporting tests/docs; no unrelated scope is evident.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/replace-3286-reasoning-endpoints

Comment @coderabbitai help to get the list of available commands.

@github-code-quality

github-code-quality Bot commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in the codex/replace-3286-r... branch is 96%. Coverage data for the main branch is not yet available.

Show a code coverage summary of the most covered files.
File main codex/replace-3286-r... b391609 +/-
nemoclaw/src/se...cret-scanner.ts 100%
nemoclaw/src/commands/slash.ts 100%
nemoclaw/src/li...bprocess-env.ts 100%
nemoclaw/src/bl...eprint/state.ts 98%
nemoclaw/src/onboard/config.ts 98%
nemoclaw/src/bl...int/snapshot.ts 97%
nemoclaw/src/bl...print/runner.ts 95%
nemoclaw/src/co...ration-state.ts 94%
nemoclaw/src/bl...ate-networks.ts 94%
nemoclaw/src/index.ts 94%

TypeScript / code-coverage/cli

The overall coverage in the codex/replace-3286-r... branch is 68%. Coverage data for the main branch is not yet available.

Show a code coverage summary of the most covered files.
File main codex/replace-3286-r... b391609 +/-
src/lib/actions...all/run-plan.ts 80%
src/lib/state/o...oard-session.ts 79%
src/lib/actions...dbox/rebuild.ts 74%
src/lib/state/sandbox.ts 72%
src/lib/shields/index.ts 70%
src/lib/onboard/preflight.ts 69%
src/lib/actions...licy-channel.ts 59%
src/lib/onboard...er-gpu-patch.ts 59%
src/lib/policy/index.ts 52%
src/lib/onboard.ts 20%

Updated June 29, 2026 03:15 UTC
Code Coverage is in Public Preview. Learn more and provide us with your feedback.

@github-actions

Copy link
Copy Markdown
Contributor

@github-actions

github-actions Bot commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

E2E Advisor Recommendation

Required E2E: onboard-resume-e2e, onboard-repair-e2e, cloud-onboard-e2e, messaging-compatible-endpoint-e2e, inference-routing-e2e
Optional E2E: docs-validation-e2e, runtime-overrides-e2e

Dispatch hint: onboard-resume-e2e,onboard-repair-e2e,cloud-onboard-e2e,messaging-compatible-endpoint-e2e,inference-routing-e2e

Auto-dispatched E2E: onboard-resume-e2e, messaging-compatible-endpoint-e2e via nightly-e2e.yaml at b39160961b522a44ddf70d1b39135cc7ed8186c2nightly run

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E

  • onboard-resume-e2e (medium): Required by the onboarding resume compatibility rule: the PR changes live onboarding machine flow context, core phases, provider-inference handling, session updates, and onboard session persistence. Unit/runtime-boundary tests are not sufficient for these resume state-machine paths.
  • onboard-repair-e2e (medium): Required by the onboarding resume compatibility rule: provider inference repair and resume state updates are changed, so repair of partially completed or drifted onboarding sessions must be validated end-to-end.
  • cloud-onboard-e2e (medium): The shared hosted onboarding path in src/lib/onboard.ts and core flow phases changed. This job validates the normal install/onboard/sandbox creation path with hosted inference credentials.
  • messaging-compatible-endpoint-e2e (medium): The compatible endpoint sandbox smoke script and reasoning-mode handling can affect Telegram/messaging sandboxes that rely on inference.local routing through an OpenAI-compatible endpoint. This existing job specifically validates Telegram plus compatible-endpoint onboarding and sandbox smoke behavior.
  • inference-routing-e2e (medium): Inference selection validation, compatible-endpoint configuration, and route setup changed. This job covers inference routing behavior including compatible endpoint onboarding and inference.local chat assertions.

Optional E2E

  • docs-validation-e2e (low): Optional confidence for the platform matrix and generated docs updates; not merge-blocking for the runtime onboarding changes but useful to catch rendered table or CLI/docs drift.
  • runtime-overrides-e2e (medium): Reasoning mode overlaps with NEMOCLAW_REASONING runtime override semantics. The changed code is primarily onboarding-time compatible-endpoint reasoning, so this is adjacent confidence rather than required.

New E2E recommendations

  • compatible-endpoint-reasoning-onboarding (high): Existing E2E jobs cover compatible-endpoint routing and messaging smoke, but there does not appear to be a dedicated live E2E that sets NEMOCLAW_REASONING=true during non-interactive compatible-endpoint onboarding, verifies the persisted session field across resume/repair, and asserts the generated sandbox openclaw.json model has reasoning enabled while non-compatible providers clear it.
    • Suggested test: Add a selective compatible-endpoint-reasoning-resume E2E scenario using a hermetic OpenAI-compatible mock and NEMOCLAW_REASONING=true, then resume or repair from the recorded session and assert provider route, session persistence, and sandbox config.

Dispatch hint

  • Workflow: nightly-e2e.yaml
  • jobs input: onboard-resume-e2e,onboard-repair-e2e,cloud-onboard-e2e,messaging-compatible-endpoint-e2e,inference-routing-e2e

@github-actions

github-actions Bot commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

Vitest E2E Scenario Recommendation

Required Vitest E2E scenarios: onboard-resume-vitest, onboard-repair-vitest, messaging-compatible-endpoint-vitest, ubuntu-repo-cloud-openclaw
Optional Vitest E2E scenarios: None

Dispatch required Vitest E2E scenarios:

  • gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field jobs=onboard-resume-vitest
  • gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field jobs=onboard-repair-vitest
  • gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field jobs=messaging-compatible-endpoint-vitest
  • gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field scenarios=ubuntu-repo-cloud-openclaw

Workflow run

Full Vitest E2E advisor summary

Vitest E2E Scenario Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required Vitest E2E scenarios

  • onboard-resume-vitest: Changes touch onboarding machine flow context/phases, provider-inference handling, session updates, and persisted onboard-session state. The resume compatibility rule requires the dedicated live Vitest resume job for state-machine resume paths.
    • Dispatch: gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field jobs=onboard-resume-vitest
  • onboard-repair-vitest: The same onboarding state-machine and persisted-session changes can affect repair/backstop execution from existing sessions, so the repair job is required, not optional.
    • Dispatch: gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field jobs=onboard-repair-vitest
  • messaging-compatible-endpoint-vitest: Changes add compatible-endpoint reasoning mode plumbing and adjust the compatible endpoint sandbox smoke path; this wired free-standing live Vitest job directly exercises the fake OpenAI-compatible endpoint with Telegram messaging and inference.local smoke behavior.
    • Dispatch: gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field jobs=messaging-compatible-endpoint-vitest
  • ubuntu-repo-cloud-openclaw: Core onboarding and provider inference setup changed in src/lib/onboard.ts and the machine provider phase; the baseline live-supported OpenClaw cloud scenario is the smallest typed registry scenario covering the primary fresh onboarding path.
    • Dispatch: gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field scenarios=ubuntu-repo-cloud-openclaw

Optional Vitest E2E scenarios

  • None.

Relevant changed files

  • src/lib/onboard.ts
  • src/lib/onboard/compatible-endpoint-smoke.ts
  • src/lib/onboard/inference-selection-validation.ts
  • src/lib/onboard/machine/core-flow-phases.ts
  • src/lib/onboard/machine/flow-context.ts
  • src/lib/onboard/machine/handlers/provider-inference.ts
  • src/lib/onboard/reasoning-mode.ts
  • src/lib/onboard/session-updates.ts
  • src/lib/onboard/setup-nim-selection.ts
  • src/lib/state/onboard-session.ts

@github-actions

github-actions Bot commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor (Nemotron Ultra) — Blocked

Merge posture: Do not merge until addressed
Primary next action: Fix PRA-5: Missing security regression test for validation bypass when NEMOCLAW_REASONING=true; then add or justify PRA-T1.
Open items: 4 required · 17 warnings · 6 suggestions · 8 test follow-ups
Since last review: 2 prior items resolved · 9 still apply · 5 new items found

Action checklist

  • PRA-5 Fix: Missing security regression test for validation bypass when NEMOCLAW_REASONING=true in test/package-contract/onboard/compatible-endpoint-reasoning.test.ts:1
  • PRA-6 Fix: validateCustomOpenAiLikeSelection reads process.env.NEMOCLAW_REASONING directly instead of parameter in src/lib/onboard/inference-selection-validation.ts:125
  • PRA-7 Fix: Platform matrix missing reasoning-mode caveat for compatible-endpoint provider in ci/platform-matrix.json:96
  • PRA-8 Fix: Test file monolith growth continues unchecked (+74 lines, now 801) in src/lib/onboard/machine/handlers/provider-inference.test.ts:1
  • PRA-1 Resolve or justify: Source-of-truth review needed: src/lib/onboard/compatible-endpoint-smoke.ts:120
  • PRA-2 Resolve or justify: Source-of-truth review needed: src/lib/onboard/inference-selection-validation.ts:118-128
  • PRA-3 Resolve or justify: Source-of-truth review needed: src/lib/onboard/reasoning-mode.ts:13
  • PRA-4 Resolve or justify: Source-of-truth review needed: src/lib/onboard/inference-selection-validation.ts:125
  • PRA-9 Resolve or justify: INITIAL_MAX_TOKENS=512 workaround missing explicit removal condition in src/lib/onboard/compatible-endpoint-smoke.ts:120
  • PRA-10 Resolve or justify: Source-of-truth review: validateCustomOpenAiLikeSelection reads process.env directly in src/lib/onboard/inference-selection-validation.ts:125
  • PRA-11 Resolve or justify: Source-of-truth review: configureCompatibleEndpointReasoning mixes normalization with process.env mutation in src/lib/onboard/reasoning-mode.ts:13
  • PRA-12 Resolve or justify: configureCompatibleEndpointReasoning mixes normalization with process.env mutation in src/lib/onboard/reasoning-mode.ts:13
  • PRA-13 Resolve or justify: No unit test verifying reasoning-enabled probe option switching for SUCCESS case in src/lib/onboard/inference-selection-validation.test.ts:1
  • PRA-14 Resolve or justify: No test verifying default behavior (reasoning disabled) probes Responses API with tool calling and streaming in src/lib/onboard/inference-selection-validation.test.ts:1
  • PRA-T1 Add or justify test follow-up: Runtime validation
  • PRA-T2 Add or justify test follow-up: Runtime validation
  • PRA-T3 Add or justify test follow-up: Runtime validation
  • PRA-T4 Add or justify test follow-up: Runtime validation
  • PRA-T5 Add or justify test follow-up: Runtime validation
  • PRA-T6 Add or justify test follow-up: No unit test verifying reasoning-enabled probe option switching for SUCCESS case
  • PRA-T7 Add or justify test follow-up: No test verifying default behavior (reasoning disabled) probes Responses API with tool calling and streaming
  • PRA-T8 Add or justify test follow-up: No test verifying reasoningEnabled takes precedence over NEMOCLAW_PREFERRED_API=openai-responses
  • PRA-19 In-scope improvement: onboard-session.ts monolith growth — monitor inference fields extraction in src/lib/state/onboard-session.ts:1306
  • PRA-20 In-scope improvement: Missing test for reasoning=true success path via chat/completions in src/lib/onboard/inference-selection-validation.test.ts:1
  • PRA-21 In-scope improvement: core-flow-phases.test.ts monolith growth — monitor in src/lib/onboard/machine/core-flow-phases.test.ts:1
  • PRA-22 In-scope improvement: onboard-session.test.ts monolith growth — monitor in src/lib/state/onboard-session.test.ts:1
  • PRA-25 In-scope improvement: Missing security regression test for validation bypass (Category 8 FAIL) in test/package-contract/onboard/compatible-endpoint-reasoning.test.ts:1
  • PRA-27 In-scope improvement: Extract reasoning-mode tests from provider-inference.test.ts monolith in src/lib/onboard/machine/handlers/provider-inference.test.ts:1

Findings index

ID Severity Category Location Required action
PRA-1 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-2 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-3 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-4 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-5 Required security test/package-contract/onboard/compatible-endpoint-reasoning.test.ts:1 Add security regression test in compatible-endpoint-reasoning.test.ts that mocks probeOpenAiLikeEndpoint to return success for chat/completions without tool calling/streaming, and expects validation to either reject or document as accepted limitation with explicit security caveat in docs and platform-matrix.json
PRA-6 Required correctness src/lib/onboard/inference-selection-validation.ts:125 Refactor validateCustomOpenAiLikeSelection to accept reasoningEnabled: boolean as a parameter. Update callers in provider-inference.ts (line ~293) and setup-nim-selection.ts (line ~159) to pass session value. Remove the process.env read and normalization from this function.
PRA-7 Required docs ci/platform-matrix.json:96 Add caveat to platform-matrix.json provider notes: 'WARNING: Reasoning mode (NEMOCLAW_REASONING=true) reduces validation coverage — only /v1/chat/completions is probed without tool-calling requirement. Ensure your endpoint supports tool calling at runtime.'
PRA-8 Required architecture src/lib/onboard/machine/handlers/provider-inference.test.ts:1 Extract reasoning-mode-related tests to provider-inference-reasoning.test.ts or group under describe('compatible-endpoint reasoning mode', ...). Group messaging resume tests under describe('compatible-endpoint resume with messaging', ...). This enables future extraction and improves readability.
PRA-9 Resolve/justify architecture src/lib/onboard/compatible-endpoint-smoke.ts:120 Add explicit removal condition to JSDoc: 'Removal condition: when major providers (OpenRouter, llama.cpp, vLLM) support a non-reasoning output mode flag (e.g., reasoning_effort=none or exclude_reasoning=true) via the same OpenAI-compatible endpoint, revert INITIAL_MAX_TOKENS to 256 and remove the reasoning_content retry logic.'
PRA-10 Resolve/justify architecture src/lib/onboard/inference-selection-validation.ts:125 Refactor to accept reasoningEnabled parameter (same as PRA-5)
PRA-11 Resolve/justify architecture src/lib/onboard/reasoning-mode.ts:13 Refactor to separate normalization from persistence: normalizeReasoningFlag returns normalized value without side effects; a separate function (or caller) stores to session/process.env if needed.
PRA-12 Resolve/justify correctness src/lib/onboard/reasoning-mode.ts:13 Make configureCompatibleEndpointReasoning pure (return normalized value only). Move process.env write to callers that need it (setup-nim-selection, provider-inference).
PRA-13 Resolve/justify tests src/lib/onboard/inference-selection-validation.test.ts:1 Add unit test in inference-selection-validation.test.ts mocking probeOpenAiLikeEndpoint to capture options, set NEMOCLAW_REASONING=true, verify probe receives {requireResponsesToolCalling:false, skipResponsesProbe:true, probeStreaming:false}. Add inverse test for reasoning=false.
PRA-14 Resolve/justify tests src/lib/onboard/inference-selection-validation.test.ts:1 Add test with NEMOCLAW_REASONING unset/false mocking probeOpenAiLikeEndpoint verifying requireResponsesToolCalling=true, skipResponsesProbe=false, probeStreaming=true are passed.
PRA-15 Resolve/justify tests test/package-contract/onboard/compatible-endpoint-reasoning.test.ts:1 Add test with NEMOCLAW_REASONING=true and NEMOCLAW_PREFERRED_API=openai-responses verifying skipResponsesProbe=true is passed to probe (reasoning mode wins).
PRA-16 Resolve/justify tests src/lib/onboard/reasoning-mode.test.ts:1 Add tests for normalizeReasoningFlag('', ' ', 'TrUe', 'FaLsE', 'YeS', 'NO') verifying robust normalization.
PRA-17 Resolve/justify tests src/lib/onboard/setup-nim-selection.test.ts:1 Add test in setup-nim-selection.test.ts mocking configureCompatibleEndpointReasoning and verifying it's called only for 'custom' provider key.
PRA-18 Resolve/justify tests test/package-contract/onboard/compatible-endpoint-reasoning.test.ts:1 Add e2e test covering resume restore and provider switch clearing of compatibleEndpointReasoning.
PRA-19 Improvement architecture src/lib/state/onboard-session.ts:1306 Extract inference session fields to separate type/module when file exceeds 1400 lines.
PRA-20 Improvement tests src/lib/onboard/inference-selection-validation.test.ts:1 Add test mocking probe success with reasoning mode and asserting ok: true, api: 'openai-completions'.

🚨 Required before merge

Address these before merging unless a maintainer explicitly overrides the advisor with rationale.

PRA-5 Required — Missing security regression test for validation bypass when NEMOCLAW_REASONING=true

  • Location: test/package-contract/onboard/compatible-endpoint-reasoning.test.ts:1
  • Category: security
  • Problem: Reasoning mode (NEMOCLAW_REASONING=true) intentionally weakens validation for compatible-endpoint provider by skipping /v1/responses probe, tool-calling requirement, and streaming probe. A malicious or misconfigured endpoint could pass /v1/chat/completions validation (no tool calling, no streaming) but fail at agent runtime when tool calling/streaming is required. No security regression test verifies this configuration is rejected or explicitly documents as accepted limitation with security caveat.
  • Impact: Validation bypass in NemoClaw's inference routing trust boundary; endpoints can pass onboarding but fail at agent runtime, potentially allowing misconfigured or malicious endpoints to be accepted
  • Required action: Add security regression test in compatible-endpoint-reasoning.test.ts that mocks probeOpenAiLikeEndpoint to return success for chat/completions without tool calling/streaming, and expects validation to either reject or document as accepted limitation with explicit security caveat in docs and platform-matrix.json
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Check test/package-contract/onboard/compatible-endpoint-reasoning.test.ts for a test that mocks probeOpenAiLikeEndpoint to succeed on chat/completions without tool calling and expects validation failure or documented acceptance
  • Missing regression test: Security regression test demonstrating validation bypass risk when reasoning mode is enabled
  • Done when: The required change is committed and verification passes: Check test/package-contract/onboard/compatible-endpoint-reasoning.test.ts for a test that mocks probeOpenAiLikeEndpoint to succeed on chat/completions without tool calling and expects validation failure or documented acceptance.
  • Evidence: inference-selection-validation.ts:118-128 sets requireResponsesToolCalling=false, skipResponsesProbe=true, probeStreaming=false when reasoningEnabled=true; no test verifies this weakening is intentional and bounded

PRA-6 Required — validateCustomOpenAiLikeSelection reads process.env.NEMOCLAW_REASONING directly instead of parameter

  • Location: src/lib/onboard/inference-selection-validation.ts:125
  • Category: correctness
  • Problem: validateCustomOpenAiLikeSelection reads process.env.NEMOCLAW_REASONING directly instead of accepting reasoningEnabled as a parameter. This couples validation logic to global process state, prevents unit testing with different reasoning modes, and violates the caller/callee contract established in setup-nim-selection.ts which already computes reasoning state.
  • Impact: Untestable validation logic; hidden global state dependency; caller cannot control validation behavior for testing or different execution contexts
  • Required action: Refactor validateCustomOpenAiLikeSelection to accept reasoningEnabled: boolean as a parameter. Update callers in provider-inference.ts (line ~293) and setup-nim-selection.ts (line ~159) to pass session value. Remove the process.env read and normalization from this function.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Check inference-selection-validation.ts:125 for normalizeReasoningFlag(process.env.NEMOCLAW_REASONING) call; verify callers pass reasoningEnabled parameter
  • Missing regression test: Unit test verifying validation behavior with reasoningEnabled=true/false passed as parameter
  • Done when: The required change is committed and verification passes: Check inference-selection-validation.ts:125 for normalizeReasoningFlag(process.env.NEMOCLAW_REASONING) call; verify callers pass reasoningEnabled parameter.
  • Evidence: inference-selection-validation.ts:125: const reasoningEnabled = normalizeReasoningFlag(process.env.NEMOCLAW_REASONING) === "true";

PRA-7 Required — Platform matrix missing reasoning-mode caveat for compatible-endpoint provider

  • Location: ci/platform-matrix.json:96
  • Category: docs
  • Problem: Platform matrix missing reasoning-mode caveat for compatible-endpoint provider. The 'Other OpenAI-compatible endpoint' provider row documents adapter validation against OpenRouter but omits the critical caveat that NEMOCLAW_REASONING=true reduces validation coverage.
  • Impact: Launch-facing documentation claims 'Tested with limitations' without disclosing the specific validation gap; users and demos may incorrectly assume full validation parity
  • Required action: Add caveat to platform-matrix.json provider notes: 'WARNING: Reasoning mode (NEMOCLAW_REASONING=true) reduces validation coverage — only /v1/chat/completions is probed without tool-calling requirement. Ensure your endpoint supports tool calling at runtime.'
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Search ci/platform-matrix.json for 'reasoning' or 'NEMOCLAW_REASONING' in the compatible-endpoint provider notes
  • Missing regression test: Doc sync test verifying platform-matrix.json contains reasoning caveat for compatible-endpoint provider
  • Done when: The required change is committed and verification passes: Search ci/platform-matrix.json for 'reasoning' or 'NEMOCLAW_REASONING' in the compatible-endpoint provider notes.
  • Evidence: ci/platform-matrix.json provider entry for 'Other OpenAI-compatible endpoint' has notes mentioning OpenRouter validation but no reasoning mode caveat

PRA-8 Required — Test file monolith growth continues unchecked (+74 lines, now 801)

  • Location: src/lib/onboard/machine/handlers/provider-inference.test.ts:1
  • Category: architecture
  • Problem: Test file monolith grew by +74 lines (now 801 lines) with reasoning mode tests added without extraction. Previous review flagged this at +55 lines (782); growth continues unchecked.
  • Impact: Degraded test maintainability; reasoning-mode tests mixed with messaging resume tests; future extraction increasingly difficult
  • Required action: Extract reasoning-mode-related tests to provider-inference-reasoning.test.ts or group under describe('compatible-endpoint reasoning mode', ...). Group messaging resume tests under describe('compatible-endpoint resume with messaging', ...). This enables future extraction and improves readability.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Count lines in provider-inference.test.ts; grep for 'reasoning mode' and 'messaging resume' describe blocks
  • Missing regression test: Architecture lint rule preventing test files > 600 lines without extraction
  • Done when: The required change is committed and verification passes: Count lines in provider-inference.test.ts; grep for 'reasoning mode' and 'messaging resume' describe blocks.
  • Evidence: provider-inference.test.ts grew from 727 to 801 lines; monolithDeltas shows severity blocker
Review findings by urgency: 4 required fixes, 17 items to resolve/justify, 6 in-scope improvements

⚠️ Resolve or justify before merge

Investigate these in the current review; either fix them, explain why they are not applicable, or document the accepted risk.

PRA-1 Resolve/justify — Source-of-truth review needed: src/lib/onboard/compatible-endpoint-smoke.ts:120

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: compatible-endpoint-smoke.test.ts: 'retries a reasoning-only length response before failing the sandbox smoke'
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: compatible-endpoint-smoke.ts INITIAL_MAX_TOKENS=512 (was 256); retry logic at lines 120-170; test covers retry path

PRA-2 Resolve/justify — Source-of-truth review needed: src/lib/onboard/inference-selection-validation.ts:118-128

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as missing.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: MISSING — no security regression test for validation bypass (PRA-4); no unit test for probe option switching success path (PRA-11, PRA-18)
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: inference-selection-validation.ts weakens probes when reasoningEnabled=true; no test verifies this weakening is intentional and bounded

PRA-3 Resolve/justify — Source-of-truth review needed: src/lib/onboard/reasoning-mode.ts:13

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: reasoning-mode.test.ts: 'restores stored state and clears it when the provider changes'
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: reasoning-mode.ts:13-18 configureCompatibleEndpointReasoning normalizes then writes to process.env.NEMOCLAW_REASONING

PRA-4 Resolve/justify — Source-of-truth review needed: src/lib/onboard/inference-selection-validation.ts:125

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as missing.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: MISSING — no unit test with reasoningEnabled parameter controlling probe options (PRA-5, PRA-11)
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: inference-selection-validation.ts:125 reads process.env.NEMOCLAW_REASONING directly; callers have compatibleEndpointReasoning state

PRA-9 Resolve/justify — INITIAL_MAX_TOKENS=512 workaround missing explicit removal condition

  • Location: src/lib/onboard/compatible-endpoint-smoke.ts:120
  • Category: architecture
  • Problem: INITIAL_MAX_TOKENS=512 workaround for reasoning-only endpoints lacks explicit removal condition. The retry logic handles reasoning_content filling token budget, but no documented criteria for when major providers will support non-reasoning output mode flags.
  • Impact: Technical debt without removal pathway; workaround may become permanent if not tracked
  • Recommended action: Add explicit removal condition to JSDoc: 'Removal condition: when major providers (OpenRouter, llama.cpp, vLLM) support a non-reasoning output mode flag (e.g., reasoning_effort=none or exclude_reasoning=true) via the same OpenAI-compatible endpoint, revert INITIAL_MAX_TOKENS to 256 and remove the reasoning_content retry logic.'
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check compatible-endpoint-smoke.ts JSDoc for removal condition at buildCompatibleEndpointSandboxSmokeScript
  • Missing regression test: None needed — documentation debt item
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check compatible-endpoint-smoke.ts JSDoc for removal condition at buildCompatibleEndpointSandboxSmokeScript.
  • Evidence: compatible-endpoint-smoke.ts:120 INITIAL_MAX_TOKENS=512 (was 256); JSDoc mentions reasoning-only endpoints but no removal condition

PRA-10 Resolve/justify — Source-of-truth review: validateCustomOpenAiLikeSelection reads process.env directly

  • Location: src/lib/onboard/inference-selection-validation.ts:125
  • Category: architecture
  • Problem: validateCustomOpenAiLikeSelection reads process.env.NEMOCLAW_REASONING directly. The invalid state is 'reasoning mode enabled globally but validation should be controlled per-call'. The source is the caller (setup-nim-selection/provider-inference) which already has session state. Source cannot be fixed in this PR because it requires signature change across callers. Regression test: new parameterized unit test. Removal: when validateCustomOpenAiLikeSelection accepts reasoningEnabled parameter.
  • Impact: Global env var coupling prevents isolated testing and creates hidden dependency
  • Recommended action: Refactor to accept reasoningEnabled parameter (same as PRA-5)
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Verify inference-selection-validation.ts:125 reads process.env directly; verify callers have reasoning state available
  • Missing regression test: Unit test with reasoningEnabled parameter controlling probe options
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Verify inference-selection-validation.ts:125 reads process.env directly; verify callers have reasoning state available.
  • Evidence: inference-selection-validation.ts:125 reads process.env.NEMOCLAW_REASONING; callers in provider-inference.ts and setup-nim-selection.ts have compatibleEndpointReasoning state

PRA-11 Resolve/justify — Source-of-truth review: configureCompatibleEndpointReasoning mixes normalization with process.env mutation

  • Location: src/lib/onboard/reasoning-mode.ts:13
  • Category: architecture
  • Problem: configureCompatibleEndpointReasoning mixes normalization with process.env mutation. Invalid state: 'reasoning flag normalization and persistence are conflated'. Source: callers need normalized value AND env persistence for downstream probes. Source cannot be fixed in this PR without changing all call sites. Regression test: existing test covers normalization + persistence. Removal: when callers handle normalization separately and persistence is explicit.
  • Impact: Side-effecting function violates single responsibility; hard to test normalization in isolation
  • Recommended action: Refactor to separate normalization from persistence: normalizeReasoningFlag returns normalized value without side effects; a separate function (or caller) stores to session/process.env if needed.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check reasoning-mode.ts:13 — configureCompatibleEndpointReasoning both normalizes AND writes to process.env
  • Missing regression test: Unit test for normalizeReasoningFlag in isolation (already exists); integration test for persistence behavior
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check reasoning-mode.ts:13 — configureCompatibleEndpointReasoning both normalizes AND writes to process.env.
  • Evidence: reasoning-mode.ts:13-18: configureCompatibleEndpointReasoning normalizes then writes to process.env.NEMOCLAW_REASONING

PRA-12 Resolve/justify — configureCompatibleEndpointReasoning mixes normalization with process.env mutation

  • Location: src/lib/onboard/reasoning-mode.ts:13
  • Category: correctness
  • Problem: configureCompatibleEndpointReasoning mixes normalization with process.env mutation. Should be pure normalization function; persistence should be caller's responsibility.
  • Impact: Hidden side effect; callers may not expect env mutation; breaks referential transparency
  • Recommended action: Make configureCompatibleEndpointReasoning pure (return normalized value only). Move process.env write to callers that need it (setup-nim-selection, provider-inference).
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check reasoning-mode.ts configureCompatibleEndpointReasoning function body
  • Missing regression test: Test verifying configureCompatibleEndpointReasoning called without env mutation when not needed
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check reasoning-mode.ts configureCompatibleEndpointReasoning function body.
  • Evidence: reasoning-mode.ts:13-18 mutates process.env.NEMOCLAW_REASONING

PRA-13 Resolve/justify — No unit test verifying reasoning-enabled probe option switching for SUCCESS case

  • Location: src/lib/onboard/inference-selection-validation.test.ts:1
  • Category: tests
  • Problem: No unit test verifying reasoning-enabled probe option switching for SUCCESS case. Existing test only covers failure path (expects retry).
  • Impact: No coverage that reasoning mode correctly configures requireResponsesToolCalling=false, skipResponsesProbe=true, probeStreaming=false on successful validation
  • Recommended action: Add unit test in inference-selection-validation.test.ts mocking probeOpenAiLikeEndpoint to capture options, set NEMOCLAW_REASONING=true, verify probe receives {requireResponsesToolCalling:false, skipResponsesProbe:true, probeStreaming:false}. Add inverse test for reasoning=false.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check inference-selection-validation.test.ts for test mocking probe and asserting options with reasoningEnabled
  • Missing regression test: Unit test verifying probe options for reasoning=true success and reasoning=false success
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check inference-selection-validation.test.ts for test mocking probe and asserting options with reasoningEnabled.
  • Evidence: inference-selection-validation.test.ts only has 'fails reasoning-mode validation when Chat Completions fails' test

PRA-14 Resolve/justify — No test verifying default behavior (reasoning disabled) probes Responses API with tool calling and streaming

  • Location: src/lib/onboard/inference-selection-validation.test.ts:1
  • Category: tests
  • Problem: No test verifying default behavior (reasoning disabled) probes Responses API with tool calling and streaming for compatible-endpoint provider.
  • Impact: Regression risk: default validation behavior could change silently
  • Recommended action: Add test with NEMOCLAW_REASONING unset/false mocking probeOpenAiLikeEndpoint verifying requireResponsesToolCalling=true, skipResponsesProbe=false, probeStreaming=true are passed.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check inference-selection-validation.test.ts for default mode test
  • Missing regression test: Unit test for default validation probe options
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check inference-selection-validation.test.ts for default mode test.
  • Evidence: inference-selection-validation.ts default behavior (reasoningEnabled=false) sets requireResponsesToolCalling=true, skipResponsesProbe=shouldForceCompletionsApi(...), probeStreaming=true

PRA-15 Resolve/justify — No test verifying reasoningEnabled takes precedence over NEMOCLAW_PREFERRED_API=openai-responses

  • Location: test/package-contract/onboard/compatible-endpoint-reasoning.test.ts:1
  • Category: tests
  • Problem: No test verifying reasoningEnabled takes precedence over NEMOCLAW_PREFERRED_API=openai-responses. When both are set, reasoning mode should win (skip responses probe).
  • Impact: Configuration precedence undefined; user could set both expecting Responses API but get chat/completions silently
  • Recommended action: Add test with NEMOCLAW_REASONING=true and NEMOCLAW_PREFERRED_API=openai-responses verifying skipResponsesProbe=true is passed to probe (reasoning mode wins).
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check compatible-endpoint-reasoning.test.ts for precedence test
  • Missing regression test: E2E test for reasoning mode precedence over preferred API
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check compatible-endpoint-reasoning.test.ts for precedence test.
  • Evidence: inference-selection-validation.ts:194 uses `reasoningEnabled || shouldForceCompletionsApi(...)` — reasoning wins but undocumented and untested

PRA-16 Resolve/justify — Missing edge case tests for normalizeReasoningFlag

  • Location: src/lib/onboard/reasoning-mode.test.ts:1
  • Category: tests
  • Problem: Missing edge case tests for normalizeReasoningFlag: '', ' ', 'TrUe', 'FaLsE', 'YeS', 'NO'. Current test covers ' YES ' and ' NO ' but not case variants or empty/whitespace.
  • Impact: Normalization robustness unverified for edge inputs
  • Recommended action: Add tests for normalizeReasoningFlag('', ' ', 'TrUe', 'FaLsE', 'YeS', 'NO') verifying robust normalization.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check reasoning-mode.test.ts for edge case coverage
  • Missing regression test: Unit tests for normalizeReasoningFlag edge cases
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check reasoning-mode.test.ts for edge case coverage.
  • Evidence: reasoning-mode.test.ts only tests 'true', '1', 'yes', 'y', ' YES ', 'false', '0', 'no', 'n', ' NO ', 'maybe'

PRA-17 Resolve/justify — No test mocking configureCompatibleEndpointReasoning for provider scoping

  • Location: src/lib/onboard/setup-nim-selection.test.ts:1
  • Category: tests
  • Problem: No test mocking configureCompatibleEndpointReasoning and verifying it's called ONLY when selected.key === 'custom'.
  • Impact: Could leak reasoning configuration to other providers (nvidia-prod, anthropic, etc.)
  • Recommended action: Add test in setup-nim-selection.test.ts mocking configureCompatibleEndpointReasoning and verifying it's called only for 'custom' provider key.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check setup-nim-selection.test.ts for provider-specific reasoning configuration test
  • Missing regression test: Unit test verifying reasoning config scoped to custom provider
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check setup-nim-selection.test.ts for provider-specific reasoning configuration test.
  • Evidence: setup-nim-selection.ts:159 calls configureCompatibleEndpointReasoning only in 'custom' branch but no test verifies this

PRA-18 Resolve/justify — No e2e test for reasoning state lifecycle (resume + provider switch)

  • Location: test/package-contract/onboard/compatible-endpoint-reasoning.test.ts:1
  • Category: tests
  • Problem: No e2e test verifying: (1) onboard with compatible-endpoint + reasoning=true, (2) resume restores reasoning state, (3) switch provider clears reasoning state.
  • Impact: Session persistence and provider-switching behavior for reasoning mode untested
  • Recommended action: Add e2e test covering resume restore and provider switch clearing of compatibleEndpointReasoning.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check compatible-endpoint-reasoning.test.ts for resume/provider-switch scenarios
  • Missing regression test: E2E test for reasoning state lifecycle across resume and provider change
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check compatible-endpoint-reasoning.test.ts for resume/provider-switch scenarios.
  • Evidence: provider-inference.ts calls clearCompatibleEndpointReasoning when provider !== 'compatible-endpoint' but no e2e test covers this

PRA-23 Resolve/justify — Reasoning mode creates validation/runtime trust boundary gap

  • Location: src/lib/onboard/inference-selection-validation.ts:118
  • Category: security
  • Problem: Reasoning mode intentionally weakens validation (skips tool-calling, streaming, responses probes). This is a trust boundary decision: the agent runtime expects tool calling/streaming but onboarding validates without them when NEMOCLAW_REASONING=true. Documented in docs with Warning but not enforced or tested as security boundary.
  • Impact: Known validation gap; relies on user to enable only when endpoint supports required capabilities
  • Recommended action: Add runtime validation in agent startup that verifies tool calling/streaming work when reasoning mode was used, or add explicit acknowledgment prompt during interactive onboarding when reasoning mode is selected.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check inference-selection-validation.ts:118-128 for probe option logic; check docs/inference/inference-options.mdx for Warning box
  • Missing regression test: Runtime verification test or interactive acknowledgment test
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check inference-selection-validation.ts:118-128 for probe option logic; check docs/inference/inference-options.mdx for Warning box.
  • Evidence: inference-selection-validation.ts weakens probes; docs/inference/inference-options.mdx has Warning box but no enforcement

PRA-24 Resolve/justify — Platform matrix missing security-relevant caveat for reasoning mode

  • Location: ci/platform-matrix.json:96
  • Category: security
  • Problem: Platform matrix missing security-relevant caveat for reasoning mode. The 'caveated' status for compatible-endpoint provider should explicitly note the validation reduction.
  • Impact: Security posture not accurately reflected in launch-facing matrix
  • Recommended action: Add reasoning mode caveat to platform-matrix.json (same as PRA-6)
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Search platform-matrix.json for compatible-endpoint provider notes
  • Missing regression test: Doc sync test for security caveats in platform matrix
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Search platform-matrix.json for compatible-endpoint provider notes.
  • Evidence: ci/platform-matrix.json provider entry lacks reasoning mode warning

PRA-26 Resolve/justify — Reasoning mode degrades holistic security posture

  • Location: src/lib/onboard/reasoning-mode.ts:13
  • Category: security
  • Problem: Reasoning mode creates a false sense of security — onboarding 'succeeds' but agent runtime may fail. Least privilege not followed: validation probes less than runtime requires. No TOCTOU but incomplete validation is a posture degradation.
  • Impact: Users may onboard successfully but hit runtime failures; validation does not match runtime requirements
  • Recommended action: Either: (a) make reasoning mode require explicit acknowledgment of reduced validation, (b) add runtime capability verification, or (c) document as known limitation with clear runtime fallback guidance.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Review docs Warning box and onboarding flow for reasoning mode
  • Missing regression test: Test verifying user acknowledgment or runtime verification when reasoning mode enabled
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Review docs Warning box and onboarding flow for reasoning mode.
  • Evidence: Security review Category 9: validation probes less than runtime requires

💡 In-scope improvements

These are lower-risk, not throwaway. Prefer fixing them in this PR when they are local to changed code; defer only with rationale or a linked follow-up.

PRA-19 Improvement — onboard-session.ts monolith growth — monitor inference fields extraction

  • Location: src/lib/state/onboard-session.ts:1306
  • Category: architecture
  • Problem: onboard-session.ts monolith grew +7 lines (now 1306). Consider extracting inference-related session fields (compatibleEndpointReasoning, preferredInferenceApi, nimContainer, endpointUrl, credentialEnv) into dedicated module or type alias.
  • Impact: Gradual monolith growth; inference state mixed with messaging/sandbox state
  • Suggested action: Extract inference session fields to separate type/module when file exceeds 1400 lines.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Count lines in onboard-session.ts; grep for inference-related fields
  • Missing regression test: None — monitoring item
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: onboard-session.ts delta +7 lines; monolithDeltas shows severity warning

PRA-20 Improvement — Missing test for reasoning=true success path via chat/completions

  • Location: src/lib/onboard/inference-selection-validation.test.ts:1
  • Category: tests
  • Problem: Missing test for reasoning=true success path via chat/completions (endpoint returns valid response without tool calling/streaming).
  • Impact: Happy path for reasoning mode untested; only failure path covered
  • Suggested action: Add test mocking probe success with reasoning mode and asserting ok: true, api: 'openai-completions'.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Check inference-selection-validation.test.ts for reasoning success test
  • Missing regression test: Unit test for reasoning mode validation success
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: inference-selection-validation.test.ts only tests failure path for reasoning mode

PRA-21 Improvement — core-flow-phases.test.ts monolith growth — monitor

  • Location: src/lib/onboard/machine/core-flow-phases.test.ts:1
  • Category: tests
  • Problem: core-flow-phases.test.ts monolith grew +5 lines (now 576). Monitor; consider extraction by feature area when exceeding 600 lines.
  • Impact: Gradual test file growth
  • Suggested action: Monitor; extract when >600 lines.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Line count check
  • Missing regression test: None — monitoring item
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: monolithDeltas shows +5 lines, severity warning

PRA-22 Improvement — onboard-session.test.ts monolith growth — monitor

  • Location: src/lib/state/onboard-session.test.ts:1
  • Category: tests
  • Problem: onboard-session.test.ts monolith grew +8 lines (now 1342). Monitor; consider extraction by feature area when exceeding 1400 lines.
  • Impact: Gradual test file growth
  • Suggested action: Monitor; extract when >1400 lines.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Line count check
  • Missing regression test: None — monitoring item
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: monolithDeltas shows +8 lines, severity warning

PRA-25 Improvement — Missing security regression test for validation bypass (Category 8 FAIL)

  • Location: test/package-contract/onboard/compatible-endpoint-reasoning.test.ts:1
  • Category: security
  • Problem: Category 8 (Security Testing): Missing security regression test for validation bypass. This is a FAIL because the security boundary (inference validation) is intentionally weakened without a test proving the risk is understood and documented.
  • Impact: Security regression could be introduced without detection; trust boundary violation undetectable in CI
  • Suggested action: Add security regression test (same as PRA-4). This is both a security testing gap and a trust boundary validation gap.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Check compatible-endpoint-reasoning.test.ts for security regression test
  • Missing regression test: Security regression test for validation bypass
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Security review Category 8: no test for intentionally weakened validation boundary

PRA-27 Improvement — Extract reasoning-mode tests from provider-inference.test.ts monolith

  • Location: src/lib/onboard/machine/handlers/provider-inference.test.ts:1
  • Category: architecture
  • Problem: provider-inference.test.ts grew +74 lines with reasoning tests mixed with messaging resume tests. Extraction would improve maintainability and enable future modularization.
  • Impact: Test maintainability degraded; mixed concerns in single file
  • Suggested action: Extract reasoning-mode-related tests to provider-inference-reasoning.test.ts or group under describe('compatible-endpoint reasoning mode', ...). Group messaging resume tests under describe('compatible-endpoint resume with messaging', ...).
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Count lines in provider-inference.test.ts; grep for 'reasoning mode' and 'messaging resume' describe blocks
  • Missing regression test: None — architecture improvement
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: monolithDeltas shows +74 lines, severity blocker; 20 named test blocks in changed test file
Simplification opportunities: 1 possible cut, net -150 lines possible

These are safe simplification checks only. Do not remove validation, security controls, data-loss prevention, or required tests.

  • PRA-27 shrink (src/lib/onboard/machine/handlers/provider-inference.test.ts:1): Reasoning mode test blocks (describe('compatible endpoint reasoning mode', ...) and related tests)
    • Replacement: New file provider-inference-reasoning.test.ts or grouped describe blocks with clear separation
    • Net: -150 lines
    • Safety boundary: Must not remove security regression test coverage; must preserve messaging resume test coverage
Test follow-ups to resolve or justify

If these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.

  • PRA-T1 Runtime validation — Security regression test: mock probeOpenAiLikeEndpoint to succeed on chat/completions without tool calling/streaming; expect validation rejection or documented acceptance with security caveat (test/package-contract/onboard/compatible-endpoint-reasoning.test.ts). Runtime/sandbox/infrastructure paths need behavioral runtime validation: ci/platform-matrix.json, docs/inference/inference-options.mdx, docs/reference/platform-support.mdx, src/lib/onboard.ts, src/lib/onboard/compatible-endpoint-smoke.ts, src/lib/onboard/inference-selection-validation.ts, src/lib/onboard/machine/core-flow-phases.ts, src/lib/onboard/machine/flow-context.ts. Current tests cover unit logic but missing security regression, default behavior verification, precedence, edge cases, provider scoping, and lifecycle E2E.
  • PRA-T2 Runtime validation — Unit test: validateCustomOpenAiLikeSelection accepts reasoningEnabled parameter; test both true/false probe option configurations (src/lib/onboard/inference-selection-validation.test.ts). Runtime/sandbox/infrastructure paths need behavioral runtime validation: ci/platform-matrix.json, docs/inference/inference-options.mdx, docs/reference/platform-support.mdx, src/lib/onboard.ts, src/lib/onboard/compatible-endpoint-smoke.ts, src/lib/onboard/inference-selection-validation.ts, src/lib/onboard/machine/core-flow-phases.ts, src/lib/onboard/machine/flow-context.ts. Current tests cover unit logic but missing security regression, default behavior verification, precedence, edge cases, provider scoping, and lifecycle E2E.
  • PRA-T3 Runtime validation — Unit test: default validation probes Responses API with tool calling/streaming when reasoning=false (src/lib/onboard/inference-selection-validation.test.ts). Runtime/sandbox/infrastructure paths need behavioral runtime validation: ci/platform-matrix.json, docs/inference/inference-options.mdx, docs/reference/platform-support.mdx, src/lib/onboard.ts, src/lib/onboard/compatible-endpoint-smoke.ts, src/lib/onboard/inference-selection-validation.ts, src/lib/onboard/machine/core-flow-phases.ts, src/lib/onboard/machine/flow-context.ts. Current tests cover unit logic but missing security regression, default behavior verification, precedence, edge cases, provider scoping, and lifecycle E2E.
  • PRA-T4 Runtime validation — E2E test: reasoning mode precedence over NEMOCLAW_PREFERRED_API=openai-responses (test/package-contract/onboard/compatible-endpoint-reasoning.test.ts). Runtime/sandbox/infrastructure paths need behavioral runtime validation: ci/platform-matrix.json, docs/inference/inference-options.mdx, docs/reference/platform-support.mdx, src/lib/onboard.ts, src/lib/onboard/compatible-endpoint-smoke.ts, src/lib/onboard/inference-selection-validation.ts, src/lib/onboard/machine/core-flow-phases.ts, src/lib/onboard/machine/flow-context.ts. Current tests cover unit logic but missing security regression, default behavior verification, precedence, edge cases, provider scoping, and lifecycle E2E.
  • PRA-T5 Runtime validation — Unit test: normalizeReasoningFlag edge cases ('', ' ', 'TrUe', 'FaLsE', 'YeS', 'NO') (src/lib/onboard/reasoning-mode.test.ts). Runtime/sandbox/infrastructure paths need behavioral runtime validation: ci/platform-matrix.json, docs/inference/inference-options.mdx, docs/reference/platform-support.mdx, src/lib/onboard.ts, src/lib/onboard/compatible-endpoint-smoke.ts, src/lib/onboard/inference-selection-validation.ts, src/lib/onboard/machine/core-flow-phases.ts, src/lib/onboard/machine/flow-context.ts. Current tests cover unit logic but missing security regression, default behavior verification, precedence, edge cases, provider scoping, and lifecycle E2E.
  • PRA-T6 No unit test verifying reasoning-enabled probe option switching for SUCCESS case — Add unit test in inference-selection-validation.test.ts mocking probeOpenAiLikeEndpoint to capture options, set NEMOCLAW_REASONING=true, verify probe receives {requireResponsesToolCalling:false, skipResponsesProbe:true, probeStreaming:false}. Add inverse test for reasoning=false.
  • PRA-T7 No test verifying default behavior (reasoning disabled) probes Responses API with tool calling and streaming — Add test with NEMOCLAW_REASONING unset/false mocking probeOpenAiLikeEndpoint verifying requireResponsesToolCalling=true, skipResponsesProbe=false, probeStreaming=true are passed.
  • PRA-T8 No test verifying reasoningEnabled takes precedence over NEMOCLAW_PREFERRED_API=openai-responses — Add test with NEMOCLAW_REASONING=true and NEMOCLAW_PREFERRED_API=openai-responses verifying skipResponsesProbe=true is passed to probe (reasoning mode wins).
Since last review details

Current findings, using the urgency labels above:

PRA-1 Resolve/justify — Source-of-truth review needed: src/lib/onboard/compatible-endpoint-smoke.ts:120

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: compatible-endpoint-smoke.test.ts: 'retries a reasoning-only length response before failing the sandbox smoke'
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: compatible-endpoint-smoke.ts INITIAL_MAX_TOKENS=512 (was 256); retry logic at lines 120-170; test covers retry path

PRA-2 Resolve/justify — Source-of-truth review needed: src/lib/onboard/inference-selection-validation.ts:118-128

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as missing.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: MISSING — no security regression test for validation bypass (PRA-4); no unit test for probe option switching success path (PRA-11, PRA-18)
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: inference-selection-validation.ts weakens probes when reasoningEnabled=true; no test verifies this weakening is intentional and bounded

PRA-3 Resolve/justify — Source-of-truth review needed: src/lib/onboard/reasoning-mode.ts:13

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: reasoning-mode.test.ts: 'restores stored state and clears it when the provider changes'
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: reasoning-mode.ts:13-18 configureCompatibleEndpointReasoning normalizes then writes to process.env.NEMOCLAW_REASONING

PRA-4 Resolve/justify — Source-of-truth review needed: src/lib/onboard/inference-selection-validation.ts:125

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as missing.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: MISSING — no unit test with reasoningEnabled parameter controlling probe options (PRA-5, PRA-11)
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: inference-selection-validation.ts:125 reads process.env.NEMOCLAW_REASONING directly; callers have compatibleEndpointReasoning state

PRA-5 Required — Missing security regression test for validation bypass when NEMOCLAW_REASONING=true

  • Location: test/package-contract/onboard/compatible-endpoint-reasoning.test.ts:1
  • Category: security
  • Problem: Reasoning mode (NEMOCLAW_REASONING=true) intentionally weakens validation for compatible-endpoint provider by skipping /v1/responses probe, tool-calling requirement, and streaming probe. A malicious or misconfigured endpoint could pass /v1/chat/completions validation (no tool calling, no streaming) but fail at agent runtime when tool calling/streaming is required. No security regression test verifies this configuration is rejected or explicitly documents as accepted limitation with security caveat.
  • Impact: Validation bypass in NemoClaw's inference routing trust boundary; endpoints can pass onboarding but fail at agent runtime, potentially allowing misconfigured or malicious endpoints to be accepted
  • Required action: Add security regression test in compatible-endpoint-reasoning.test.ts that mocks probeOpenAiLikeEndpoint to return success for chat/completions without tool calling/streaming, and expects validation to either reject or document as accepted limitation with explicit security caveat in docs and platform-matrix.json
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Check test/package-contract/onboard/compatible-endpoint-reasoning.test.ts for a test that mocks probeOpenAiLikeEndpoint to succeed on chat/completions without tool calling and expects validation failure or documented acceptance
  • Missing regression test: Security regression test demonstrating validation bypass risk when reasoning mode is enabled
  • Done when: The required change is committed and verification passes: Check test/package-contract/onboard/compatible-endpoint-reasoning.test.ts for a test that mocks probeOpenAiLikeEndpoint to succeed on chat/completions without tool calling and expects validation failure or documented acceptance.
  • Evidence: inference-selection-validation.ts:118-128 sets requireResponsesToolCalling=false, skipResponsesProbe=true, probeStreaming=false when reasoningEnabled=true; no test verifies this weakening is intentional and bounded

PRA-6 Required — validateCustomOpenAiLikeSelection reads process.env.NEMOCLAW_REASONING directly instead of parameter

  • Location: src/lib/onboard/inference-selection-validation.ts:125
  • Category: correctness
  • Problem: validateCustomOpenAiLikeSelection reads process.env.NEMOCLAW_REASONING directly instead of accepting reasoningEnabled as a parameter. This couples validation logic to global process state, prevents unit testing with different reasoning modes, and violates the caller/callee contract established in setup-nim-selection.ts which already computes reasoning state.
  • Impact: Untestable validation logic; hidden global state dependency; caller cannot control validation behavior for testing or different execution contexts
  • Required action: Refactor validateCustomOpenAiLikeSelection to accept reasoningEnabled: boolean as a parameter. Update callers in provider-inference.ts (line ~293) and setup-nim-selection.ts (line ~159) to pass session value. Remove the process.env read and normalization from this function.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Check inference-selection-validation.ts:125 for normalizeReasoningFlag(process.env.NEMOCLAW_REASONING) call; verify callers pass reasoningEnabled parameter
  • Missing regression test: Unit test verifying validation behavior with reasoningEnabled=true/false passed as parameter
  • Done when: The required change is committed and verification passes: Check inference-selection-validation.ts:125 for normalizeReasoningFlag(process.env.NEMOCLAW_REASONING) call; verify callers pass reasoningEnabled parameter.
  • Evidence: inference-selection-validation.ts:125: const reasoningEnabled = normalizeReasoningFlag(process.env.NEMOCLAW_REASONING) === "true";

PRA-7 Required — Platform matrix missing reasoning-mode caveat for compatible-endpoint provider

  • Location: ci/platform-matrix.json:96
  • Category: docs
  • Problem: Platform matrix missing reasoning-mode caveat for compatible-endpoint provider. The 'Other OpenAI-compatible endpoint' provider row documents adapter validation against OpenRouter but omits the critical caveat that NEMOCLAW_REASONING=true reduces validation coverage.
  • Impact: Launch-facing documentation claims 'Tested with limitations' without disclosing the specific validation gap; users and demos may incorrectly assume full validation parity
  • Required action: Add caveat to platform-matrix.json provider notes: 'WARNING: Reasoning mode (NEMOCLAW_REASONING=true) reduces validation coverage — only /v1/chat/completions is probed without tool-calling requirement. Ensure your endpoint supports tool calling at runtime.'
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Search ci/platform-matrix.json for 'reasoning' or 'NEMOCLAW_REASONING' in the compatible-endpoint provider notes
  • Missing regression test: Doc sync test verifying platform-matrix.json contains reasoning caveat for compatible-endpoint provider
  • Done when: The required change is committed and verification passes: Search ci/platform-matrix.json for 'reasoning' or 'NEMOCLAW_REASONING' in the compatible-endpoint provider notes.
  • Evidence: ci/platform-matrix.json provider entry for 'Other OpenAI-compatible endpoint' has notes mentioning OpenRouter validation but no reasoning mode caveat

PRA-8 Required — Test file monolith growth continues unchecked (+74 lines, now 801)

  • Location: src/lib/onboard/machine/handlers/provider-inference.test.ts:1
  • Category: architecture
  • Problem: Test file monolith grew by +74 lines (now 801 lines) with reasoning mode tests added without extraction. Previous review flagged this at +55 lines (782); growth continues unchecked.
  • Impact: Degraded test maintainability; reasoning-mode tests mixed with messaging resume tests; future extraction increasingly difficult
  • Required action: Extract reasoning-mode-related tests to provider-inference-reasoning.test.ts or group under describe('compatible-endpoint reasoning mode', ...). Group messaging resume tests under describe('compatible-endpoint resume with messaging', ...). This enables future extraction and improves readability.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Count lines in provider-inference.test.ts; grep for 'reasoning mode' and 'messaging resume' describe blocks
  • Missing regression test: Architecture lint rule preventing test files > 600 lines without extraction
  • Done when: The required change is committed and verification passes: Count lines in provider-inference.test.ts; grep for 'reasoning mode' and 'messaging resume' describe blocks.
  • Evidence: provider-inference.test.ts grew from 727 to 801 lines; monolithDeltas shows severity blocker

PRA-9 Resolve/justify — INITIAL_MAX_TOKENS=512 workaround missing explicit removal condition

  • Location: src/lib/onboard/compatible-endpoint-smoke.ts:120
  • Category: architecture
  • Problem: INITIAL_MAX_TOKENS=512 workaround for reasoning-only endpoints lacks explicit removal condition. The retry logic handles reasoning_content filling token budget, but no documented criteria for when major providers will support non-reasoning output mode flags.
  • Impact: Technical debt without removal pathway; workaround may become permanent if not tracked
  • Recommended action: Add explicit removal condition to JSDoc: 'Removal condition: when major providers (OpenRouter, llama.cpp, vLLM) support a non-reasoning output mode flag (e.g., reasoning_effort=none or exclude_reasoning=true) via the same OpenAI-compatible endpoint, revert INITIAL_MAX_TOKENS to 256 and remove the reasoning_content retry logic.'
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check compatible-endpoint-smoke.ts JSDoc for removal condition at buildCompatibleEndpointSandboxSmokeScript
  • Missing regression test: None needed — documentation debt item
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check compatible-endpoint-smoke.ts JSDoc for removal condition at buildCompatibleEndpointSandboxSmokeScript.
  • Evidence: compatible-endpoint-smoke.ts:120 INITIAL_MAX_TOKENS=512 (was 256); JSDoc mentions reasoning-only endpoints but no removal condition

PRA-10 Resolve/justify — Source-of-truth review: validateCustomOpenAiLikeSelection reads process.env directly

  • Location: src/lib/onboard/inference-selection-validation.ts:125
  • Category: architecture
  • Problem: validateCustomOpenAiLikeSelection reads process.env.NEMOCLAW_REASONING directly. The invalid state is 'reasoning mode enabled globally but validation should be controlled per-call'. The source is the caller (setup-nim-selection/provider-inference) which already has session state. Source cannot be fixed in this PR because it requires signature change across callers. Regression test: new parameterized unit test. Removal: when validateCustomOpenAiLikeSelection accepts reasoningEnabled parameter.
  • Impact: Global env var coupling prevents isolated testing and creates hidden dependency
  • Recommended action: Refactor to accept reasoningEnabled parameter (same as PRA-5)
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Verify inference-selection-validation.ts:125 reads process.env directly; verify callers have reasoning state available
  • Missing regression test: Unit test with reasoningEnabled parameter controlling probe options
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Verify inference-selection-validation.ts:125 reads process.env directly; verify callers have reasoning state available.
  • Evidence: inference-selection-validation.ts:125 reads process.env.NEMOCLAW_REASONING; callers in provider-inference.ts and setup-nim-selection.ts have compatibleEndpointReasoning state

PRA-11 Resolve/justify — Source-of-truth review: configureCompatibleEndpointReasoning mixes normalization with process.env mutation

  • Location: src/lib/onboard/reasoning-mode.ts:13
  • Category: architecture
  • Problem: configureCompatibleEndpointReasoning mixes normalization with process.env mutation. Invalid state: 'reasoning flag normalization and persistence are conflated'. Source: callers need normalized value AND env persistence for downstream probes. Source cannot be fixed in this PR without changing all call sites. Regression test: existing test covers normalization + persistence. Removal: when callers handle normalization separately and persistence is explicit.
  • Impact: Side-effecting function violates single responsibility; hard to test normalization in isolation
  • Recommended action: Refactor to separate normalization from persistence: normalizeReasoningFlag returns normalized value without side effects; a separate function (or caller) stores to session/process.env if needed.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check reasoning-mode.ts:13 — configureCompatibleEndpointReasoning both normalizes AND writes to process.env
  • Missing regression test: Unit test for normalizeReasoningFlag in isolation (already exists); integration test for persistence behavior
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check reasoning-mode.ts:13 — configureCompatibleEndpointReasoning both normalizes AND writes to process.env.
  • Evidence: reasoning-mode.ts:13-18: configureCompatibleEndpointReasoning normalizes then writes to process.env.NEMOCLAW_REASONING

PRA-12 Resolve/justify — configureCompatibleEndpointReasoning mixes normalization with process.env mutation

  • Location: src/lib/onboard/reasoning-mode.ts:13
  • Category: correctness
  • Problem: configureCompatibleEndpointReasoning mixes normalization with process.env mutation. Should be pure normalization function; persistence should be caller's responsibility.
  • Impact: Hidden side effect; callers may not expect env mutation; breaks referential transparency
  • Recommended action: Make configureCompatibleEndpointReasoning pure (return normalized value only). Move process.env write to callers that need it (setup-nim-selection, provider-inference).
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check reasoning-mode.ts configureCompatibleEndpointReasoning function body
  • Missing regression test: Test verifying configureCompatibleEndpointReasoning called without env mutation when not needed
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check reasoning-mode.ts configureCompatibleEndpointReasoning function body.
  • Evidence: reasoning-mode.ts:13-18 mutates process.env.NEMOCLAW_REASONING

PRA-13 Resolve/justify — No unit test verifying reasoning-enabled probe option switching for SUCCESS case

  • Location: src/lib/onboard/inference-selection-validation.test.ts:1
  • Category: tests
  • Problem: No unit test verifying reasoning-enabled probe option switching for SUCCESS case. Existing test only covers failure path (expects retry).
  • Impact: No coverage that reasoning mode correctly configures requireResponsesToolCalling=false, skipResponsesProbe=true, probeStreaming=false on successful validation
  • Recommended action: Add unit test in inference-selection-validation.test.ts mocking probeOpenAiLikeEndpoint to capture options, set NEMOCLAW_REASONING=true, verify probe receives {requireResponsesToolCalling:false, skipResponsesProbe:true, probeStreaming:false}. Add inverse test for reasoning=false.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check inference-selection-validation.test.ts for test mocking probe and asserting options with reasoningEnabled
  • Missing regression test: Unit test verifying probe options for reasoning=true success and reasoning=false success
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check inference-selection-validation.test.ts for test mocking probe and asserting options with reasoningEnabled.
  • Evidence: inference-selection-validation.test.ts only has 'fails reasoning-mode validation when Chat Completions fails' test

PRA-14 Resolve/justify — No test verifying default behavior (reasoning disabled) probes Responses API with tool calling and streaming

  • Location: src/lib/onboard/inference-selection-validation.test.ts:1
  • Category: tests
  • Problem: No test verifying default behavior (reasoning disabled) probes Responses API with tool calling and streaming for compatible-endpoint provider.
  • Impact: Regression risk: default validation behavior could change silently
  • Recommended action: Add test with NEMOCLAW_REASONING unset/false mocking probeOpenAiLikeEndpoint verifying requireResponsesToolCalling=true, skipResponsesProbe=false, probeStreaming=true are passed.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check inference-selection-validation.test.ts for default mode test
  • Missing regression test: Unit test for default validation probe options
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check inference-selection-validation.test.ts for default mode test.
  • Evidence: inference-selection-validation.ts default behavior (reasoningEnabled=false) sets requireResponsesToolCalling=true, skipResponsesProbe=shouldForceCompletionsApi(...), probeStreaming=true

PRA-15 Resolve/justify — No test verifying reasoningEnabled takes precedence over NEMOCLAW_PREFERRED_API=openai-responses

  • Location: test/package-contract/onboard/compatible-endpoint-reasoning.test.ts:1
  • Category: tests
  • Problem: No test verifying reasoningEnabled takes precedence over NEMOCLAW_PREFERRED_API=openai-responses. When both are set, reasoning mode should win (skip responses probe).
  • Impact: Configuration precedence undefined; user could set both expecting Responses API but get chat/completions silently
  • Recommended action: Add test with NEMOCLAW_REASONING=true and NEMOCLAW_PREFERRED_API=openai-responses verifying skipResponsesProbe=true is passed to probe (reasoning mode wins).
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check compatible-endpoint-reasoning.test.ts for precedence test
  • Missing regression test: E2E test for reasoning mode precedence over preferred API
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check compatible-endpoint-reasoning.test.ts for precedence test.
  • Evidence: inference-selection-validation.ts:194 uses `reasoningEnabled || shouldForceCompletionsApi(...)` — reasoning wins but undocumented and untested

PRA-16 Resolve/justify — Missing edge case tests for normalizeReasoningFlag

  • Location: src/lib/onboard/reasoning-mode.test.ts:1
  • Category: tests
  • Problem: Missing edge case tests for normalizeReasoningFlag: '', ' ', 'TrUe', 'FaLsE', 'YeS', 'NO'. Current test covers ' YES ' and ' NO ' but not case variants or empty/whitespace.
  • Impact: Normalization robustness unverified for edge inputs
  • Recommended action: Add tests for normalizeReasoningFlag('', ' ', 'TrUe', 'FaLsE', 'YeS', 'NO') verifying robust normalization.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check reasoning-mode.test.ts for edge case coverage
  • Missing regression test: Unit tests for normalizeReasoningFlag edge cases
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check reasoning-mode.test.ts for edge case coverage.
  • Evidence: reasoning-mode.test.ts only tests 'true', '1', 'yes', 'y', ' YES ', 'false', '0', 'no', 'n', ' NO ', 'maybe'

PRA-17 Resolve/justify — No test mocking configureCompatibleEndpointReasoning for provider scoping

  • Location: src/lib/onboard/setup-nim-selection.test.ts:1
  • Category: tests
  • Problem: No test mocking configureCompatibleEndpointReasoning and verifying it's called ONLY when selected.key === 'custom'.
  • Impact: Could leak reasoning configuration to other providers (nvidia-prod, anthropic, etc.)
  • Recommended action: Add test in setup-nim-selection.test.ts mocking configureCompatibleEndpointReasoning and verifying it's called only for 'custom' provider key.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check setup-nim-selection.test.ts for provider-specific reasoning configuration test
  • Missing regression test: Unit test verifying reasoning config scoped to custom provider
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check setup-nim-selection.test.ts for provider-specific reasoning configuration test.
  • Evidence: setup-nim-selection.ts:159 calls configureCompatibleEndpointReasoning only in 'custom' branch but no test verifies this

PRA-18 Resolve/justify — No e2e test for reasoning state lifecycle (resume + provider switch)

  • Location: test/package-contract/onboard/compatible-endpoint-reasoning.test.ts:1
  • Category: tests
  • Problem: No e2e test verifying: (1) onboard with compatible-endpoint + reasoning=true, (2) resume restores reasoning state, (3) switch provider clears reasoning state.
  • Impact: Session persistence and provider-switching behavior for reasoning mode untested
  • Recommended action: Add e2e test covering resume restore and provider switch clearing of compatibleEndpointReasoning.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check compatible-endpoint-reasoning.test.ts for resume/provider-switch scenarios
  • Missing regression test: E2E test for reasoning state lifecycle across resume and provider change
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check compatible-endpoint-reasoning.test.ts for resume/provider-switch scenarios.
  • Evidence: provider-inference.ts calls clearCompatibleEndpointReasoning when provider !== 'compatible-endpoint' but no e2e test covers this

PRA-19 Improvement — onboard-session.ts monolith growth — monitor inference fields extraction

  • Location: src/lib/state/onboard-session.ts:1306
  • Category: architecture
  • Problem: onboard-session.ts monolith grew +7 lines (now 1306). Consider extracting inference-related session fields (compatibleEndpointReasoning, preferredInferenceApi, nimContainer, endpointUrl, credentialEnv) into dedicated module or type alias.
  • Impact: Gradual monolith growth; inference state mixed with messaging/sandbox state
  • Suggested action: Extract inference session fields to separate type/module when file exceeds 1400 lines.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Count lines in onboard-session.ts; grep for inference-related fields
  • Missing regression test: None — monitoring item
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: onboard-session.ts delta +7 lines; monolithDeltas shows severity warning

PRA-20 Improvement — Missing test for reasoning=true success path via chat/completions

  • Location: src/lib/onboard/inference-selection-validation.test.ts:1
  • Category: tests
  • Problem: Missing test for reasoning=true success path via chat/completions (endpoint returns valid response without tool calling/streaming).
  • Impact: Happy path for reasoning mode untested; only failure path covered
  • Suggested action: Add test mocking probe success with reasoning mode and asserting ok: true, api: 'openai-completions'.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Check inference-selection-validation.test.ts for reasoning success test
  • Missing regression test: Unit test for reasoning mode validation success
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: inference-selection-validation.test.ts only tests failure path for reasoning mode

Workflow run details

This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision.

@github-actions

github-actions Bot commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor — Changes requested

Merge posture: Do not merge yet
Primary next action: Resolve or justify PRA-1: Reasoning mode intentionally skips tool and streaming validation.
Open items: 0 required · 1 warning · 0 suggestions · 2 test follow-ups
Since last review: 2 prior items resolved · 1 still applies · 0 new items found

Action checklist

  • PRA-1 Resolve or justify: Reasoning mode intentionally skips tool and streaming validation in src/lib/onboard/inference-selection-validation.ts:189
  • PRA-T1 Add or justify test follow-up: Runtime validation
  • PRA-T2 Add or justify test follow-up: Runtime validation

Findings index

ID Severity Category Location Required action
PRA-1 Resolve/justify security src/lib/onboard/inference-selection-validation.ts:189 Resolve or explicitly justify this accepted risk in the PR. If maintainers want stronger protection, add a local guard or confirmation for reasoning mode when messaging/tool-dependent paths are selected; otherwise keep the docs warning, console warning, and reduced-probe tests synchronized with the exact skipped probes.
Review findings by urgency: 0 required fixes, 1 item to resolve/justify, 0 in-scope improvements

⚠️ Resolve or justify before merge

Investigate these in the current review; either fix them, explain why they are not applicable, or document the accepted risk.

PRA-1 Resolve/justify — Reasoning mode intentionally skips tool and streaming validation

  • Location: src/lib/onboard/inference-selection-validation.ts:189
  • Category: security
  • Problem: When `NEMOCLAW_REASONING` normalizes to true for a custom OpenAI-compatible endpoint, validation now accepts Chat Completions only by setting `requireResponsesToolCalling: false`, `skipResponsesProbe: true`, and `probeStreaming: false`. The PR adds a console warning, docs warning, and package-contract coverage, so this appears intentional, but it remains a reduced validation mode at the inference trust boundary.
  • Impact: A custom endpoint can pass onboarding in reasoning mode even if tool calling or streaming is unsupported. Agents or messaging flows that rely on those capabilities may fail only after sandbox creation rather than being rejected during provider validation.
  • Recommended action: Resolve or explicitly justify this accepted risk in the PR. If maintainers want stronger protection, add a local guard or confirmation for reasoning mode when messaging/tool-dependent paths are selected; otherwise keep the docs warning, console warning, and reduced-probe tests synchronized with the exact skipped probes.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read `src/lib/onboard/inference-selection-validation.ts` around line 189 and confirm the reasoning-enabled branch passes `{ requireResponsesToolCalling: false, skipResponsesProbe: true, probeStreaming: false }`; then read `src/lib/onboard/setup-nim-selection.ts` and `docs/inference/inference-options.mdx` to confirm the warning text remains aligned.
  • Missing regression test: Existing coverage in `src/lib/onboard/inference-selection-validation.test.ts` and `test/package-contract/onboard/compatible-endpoint-reasoning.test.ts` proves the reduced probe shape and warning. Add a behavior test named `reasoning mode with messaging-compatible endpoint warns that tool and streaming validation are skipped before sandbox creation` if the team wants the warning specifically pinned on messaging/tool-dependent flows.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read `src/lib/onboard/inference-selection-validation.ts` around line 189 and confirm the reasoning-enabled branch passes `{ requireResponsesToolCalling: false, skipResponsesProbe: true, probeStreaming: false }`; then read `src/lib/onboard/setup-nim-selection.ts` and `docs/inference/inference-options.mdx` to confirm the warning text remains aligned.
  • Evidence: `validateCustomOpenAiLikeSelection()` derives `reasoningEnabled` from `NEMOCLAW_REASONING` and disables Responses, tool-call, and streaming probes; the package-contract test asserts `/responses` and streaming `-N` are not used.

💡 In-scope improvements

These are lower-risk, not throwaway. Prefer fixing them in this PR when they are local to changed code; defer only with rationale or a linked follow-up.

  • None.
Test follow-ups to resolve or justify

If these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.

  • PRA-T1 Runtime validation — Add or identify coverage named `non-interactive custom endpoint with NEMOCLAW_REASONING=yes patches the staged Dockerfile and generated openclaw.json with reasoning true`.. Changed behavior crosses provider selection, process-env normalization, session persistence, resume shortcuts, validation probes, Dockerfile/OpenClaw config patching, and sandbox smoke. The PR includes strong unit and package-contract coverage, including the previously missing stale-clear artifact boundary, but a positive runtime/artifact-path check for the enabled Option 3 mode would further reduce integration risk.
  • PRA-T2 Runtime validation — If maintainers want stronger protection for tool-dependent flows, add coverage named `reasoning mode with messaging-compatible endpoint warns that tool and streaming validation are skipped before sandbox creation`.. Changed behavior crosses provider selection, process-env normalization, session persistence, resume shortcuts, validation probes, Dockerfile/OpenClaw config patching, and sandbox smoke. The PR includes strong unit and package-contract coverage, including the previously missing stale-clear artifact boundary, but a positive runtime/artifact-path check for the enabled Option 3 mode would further reduce integration risk.
Since last review details

Current findings, using the urgency labels above:

PRA-1 Resolve/justify — Reasoning mode intentionally skips tool and streaming validation

  • Location: src/lib/onboard/inference-selection-validation.ts:189
  • Category: security
  • Problem: When `NEMOCLAW_REASONING` normalizes to true for a custom OpenAI-compatible endpoint, validation now accepts Chat Completions only by setting `requireResponsesToolCalling: false`, `skipResponsesProbe: true`, and `probeStreaming: false`. The PR adds a console warning, docs warning, and package-contract coverage, so this appears intentional, but it remains a reduced validation mode at the inference trust boundary.
  • Impact: A custom endpoint can pass onboarding in reasoning mode even if tool calling or streaming is unsupported. Agents or messaging flows that rely on those capabilities may fail only after sandbox creation rather than being rejected during provider validation.
  • Recommended action: Resolve or explicitly justify this accepted risk in the PR. If maintainers want stronger protection, add a local guard or confirmation for reasoning mode when messaging/tool-dependent paths are selected; otherwise keep the docs warning, console warning, and reduced-probe tests synchronized with the exact skipped probes.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read `src/lib/onboard/inference-selection-validation.ts` around line 189 and confirm the reasoning-enabled branch passes `{ requireResponsesToolCalling: false, skipResponsesProbe: true, probeStreaming: false }`; then read `src/lib/onboard/setup-nim-selection.ts` and `docs/inference/inference-options.mdx` to confirm the warning text remains aligned.
  • Missing regression test: Existing coverage in `src/lib/onboard/inference-selection-validation.test.ts` and `test/package-contract/onboard/compatible-endpoint-reasoning.test.ts` proves the reduced probe shape and warning. Add a behavior test named `reasoning mode with messaging-compatible endpoint warns that tool and streaming validation are skipped before sandbox creation` if the team wants the warning specifically pinned on messaging/tool-dependent flows.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read `src/lib/onboard/inference-selection-validation.ts` around line 189 and confirm the reasoning-enabled branch passes `{ requireResponsesToolCalling: false, skipResponsesProbe: true, probeStreaming: false }`; then read `src/lib/onboard/setup-nim-selection.ts` and `docs/inference/inference-options.mdx` to confirm the warning text remains aligned.
  • Evidence: `validateCustomOpenAiLikeSelection()` derives `reasoningEnabled` from `NEMOCLAW_REASONING` and disables Responses, tool-call, and streaming probes; the package-contract test asserts `/responses` and streaming `-N` are not used.

Workflow run details

This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision.

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@github-actions

Copy link
Copy Markdown
Contributor

Selective E2E Results — ✅ All requested jobs passed

Run: 28338590826
Target ref: 96768240d67a7316a8736efa8e7df01667ef932d
Workflow ref: main
Requested jobs: onboard-resume-e2e,messaging-compatible-endpoint-e2e
Summary: 2 passed, 0 failed, 0 cancelled, 0 skipped

Job Result
messaging-compatible-endpoint-e2e ✅ success
onboard-resume-e2e ✅ success

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (2)
src/lib/state/onboard-session.ts (1)

103-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Narrow the persisted reasoning flag to its normalized union.

normalizeReasoningFlag() already reduces this value to "true", "false", or null, but the session/update/debug contracts widen it back to string | null. That makes typos persist to disk and only get silently coerced later on resume. Tightening these fields to the literal union would keep the contract honest end to end.

Also applies to: 173-173, 203-203

🤖 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/state/onboard-session.ts` at line 103, Narrow the persisted reasoning
flag to the normalized literal union so the session contract stays honest end to
end. Update the relevant types around compatibleEndpointReasoning in the onboard
session model and the related update/debug contracts to use "true" | "false" |
null instead of string | null, and make sure normalizeReasoningFlag remains the
single place that converts incoming values into that union.
src/lib/onboard/machine/handlers/provider-inference.test.ts (1)

155-158: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert the handler result instead of recordStepComplete internals.

These expectations couple the test to the current session-write plumbing. handleProviderInferenceState() already returns the reasoning state, so this is more robust if you assert the returned state/session rather than the mock-call payload.
As per path instructions, "Prefer observable outcomes through the public boundary over source-text, private-shape, or mock-call assertions."

Also applies to: 217-223

🤖 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/machine/handlers/provider-inference.test.ts` around lines 155
- 158, The test is asserting `recordStepComplete` mock internals instead of the
observable result from `handleProviderInferenceState()`. Update the affected
expectations in `provider-inference.test.ts` to verify the handler’s returned
state/session outcome (including reasoning fields like
`compatibleEndpointReasoning` and `provider`) rather than the payload passed
into `recordStepComplete`, and apply the same change to the other matching
assertion block.

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 `@src/lib/onboard/inference-selection-validation.ts`:
- Around line 188-193: The reasoning-mode check in the inference probe path is
using a raw `process.env.NEMOCLAW_REASONING === "true"` comparison, which
bypasses the shared alias-normalization logic. Update `runOpenAiLikeProbe` setup
in `inference-selection-validation` to read the flag through the existing shared
reasoning helper used elsewhere in the codebase, and then use that normalized
boolean for `requireResponsesToolCalling`, `skipResponsesProbe`, and
`probeStreaming` so this path has a single source of truth.

In `@src/lib/onboard/machine/handlers/provider-inference.ts`:
- Around line 291-294: The provider selection flow in provider-inference should
explicitly clear NEMOCLAW_REASONING whenever the chosen provider is not
"compatible-endpoint", since the current path only preserves
selection.compatibleEndpointReasoning and can leave stale reasoning state
behind. Update the provider-selection handling around
compatibleEndpointReasoning and setupNim so non-compatible selections call the
clearing path (same behavior as the resume bridge) and only compatible-endpoint
retains or configures the flag.

---

Nitpick comments:
In `@src/lib/onboard/machine/handlers/provider-inference.test.ts`:
- Around line 155-158: The test is asserting `recordStepComplete` mock internals
instead of the observable result from `handleProviderInferenceState()`. Update
the affected expectations in `provider-inference.test.ts` to verify the
handler’s returned state/session outcome (including reasoning fields like
`compatibleEndpointReasoning` and `provider`) rather than the payload passed
into `recordStepComplete`, and apply the same change to the other matching
assertion block.

In `@src/lib/state/onboard-session.ts`:
- Line 103: Narrow the persisted reasoning flag to the normalized literal union
so the session contract stays honest end to end. Update the relevant types
around compatibleEndpointReasoning in the onboard session model and the related
update/debug contracts to use "true" | "false" | null instead of string | null,
and make sure normalizeReasoningFlag remains the single place that converts
incoming values into that union.
🪄 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: d80b0fda-816b-48ed-ab02-3351c78b795f

📥 Commits

Reviewing files that changed from the base of the PR and between 7b9267d and 9676824.

📒 Files selected for processing (28)
  • ci/platform-matrix.json
  • docs/inference/inference-options.mdx
  • docs/reference/platform-support.mdx
  • src/lib/onboard.ts
  • src/lib/onboard/compatible-endpoint-smoke.test.ts
  • src/lib/onboard/compatible-endpoint-smoke.ts
  • src/lib/onboard/inference-selection-validation.ts
  • src/lib/onboard/machine/core-flow-phases.test.ts
  • src/lib/onboard/machine/core-flow-phases.ts
  • src/lib/onboard/machine/flow-context.test.ts
  • src/lib/onboard/machine/flow-context.ts
  • src/lib/onboard/machine/flow-phases/agent-policy-finalization.test.ts
  • src/lib/onboard/machine/flow-phases/preflight-gateway.test.ts
  • src/lib/onboard/machine/flow-phases/provider-sandbox.test.ts
  • src/lib/onboard/machine/flow-sequence.test.ts
  • src/lib/onboard/machine/flow-slices.test.ts
  • src/lib/onboard/machine/handlers/provider-inference.test.ts
  • src/lib/onboard/machine/handlers/provider-inference.ts
  • src/lib/onboard/machine/initial-flow-phases.test.ts
  • src/lib/onboard/reasoning-mode.test.ts
  • src/lib/onboard/reasoning-mode.ts
  • src/lib/onboard/session-updates.ts
  • src/lib/onboard/setup-nim-selection.test.ts
  • src/lib/onboard/setup-nim-selection.ts
  • src/lib/state/onboard-session.test.ts
  • src/lib/state/onboard-session.ts
  • test/helpers/onboard-final-flow-phases.ts
  • test/package-contract/onboard/compatible-endpoint-reasoning.test.ts

Comment thread src/lib/onboard/inference-selection-validation.ts Outdated
Comment thread src/lib/onboard/machine/handlers/provider-inference.ts
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@github-actions

Copy link
Copy Markdown
Contributor

Vitest E2E Scenario Results — ✅ All requested jobs passed

Run: 28338756522
Workflow ref: codex/replace-3286-reasoning-endpoints
Requested scenarios: (default — all supported)
Requested jobs: onboard-resume-vitest
Summary: 1 passed, 0 failed, 0 cancelled, 0 skipped

Job Result
onboard-resume-vitest ✅ success

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@docs/inference/inference-options.mdx`:
- Around line 240-241: The reasoning flag docs mention an unsupported truthy
alias, so align the documentation with `normalizeReasoningFlag()` behavior.
Update the `NEMOCLAW_REASONING` onboarding text to list only the aliases
actually accepted by the normalizer (`true`, `1`, `yes`, `y`), or, if intended,
update `normalizeReasoningFlag()` and its callers to accept `on` as part of the
same change.
🪄 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: d0ed7435-d0af-4319-ae65-14d3f57c18f0

📥 Commits

Reviewing files that changed from the base of the PR and between 9676824 and adc85b8.

📒 Files selected for processing (4)
  • docs/inference/inference-options.mdx
  • src/lib/onboard/inference-selection-validation.ts
  • src/lib/onboard/setup-nim-selection.ts
  • test/generate-platform-docs.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/lib/onboard/inference-selection-validation.ts
  • src/lib/onboard/setup-nim-selection.ts

Comment thread docs/inference/inference-options.mdx Outdated
@github-actions

Copy link
Copy Markdown
Contributor

Vitest E2E Scenario Results — ✅ All requested jobs passed

Run: 28338756552
Workflow ref: codex/replace-3286-reasoning-endpoints
Requested scenarios: (default — all supported)
Requested jobs: messaging-compatible-endpoint-vitest
Summary: 1 passed, 0 failed, 0 cancelled, 0 skipped

Job Result
messaging-compatible-endpoint-vitest ✅ success

@github-actions

Copy link
Copy Markdown
Contributor

Selective E2E Results — ⚠️ Run cancelled — no signal

Run: 28338843791
Target ref: adc85b828b5767aa40d742eb49e7134d758c71b0
Workflow ref: main
Requested jobs: onboard-resume-e2e,messaging-compatible-endpoint-e2e
Summary: 0 passed, 0 failed, 2 cancelled, 0 skipped

Job Result
messaging-compatible-endpoint-e2e ⚠️ cancelled
onboard-resume-e2e ⚠️ cancelled

@github-actions

Copy link
Copy Markdown
Contributor

Selective E2E Results — ⚠️ Run cancelled — no signal

Run: 28338828434
Target ref: codex/replace-3286-reasoning-endpoints
Requested jobs: onboard-repair-e2e,cloud-onboard-e2e
Summary: 0 passed, 0 failed, 2 cancelled, 0 skipped

Job Result
cloud-onboard-e2e ⚠️ cancelled
onboard-repair-e2e ⚠️ cancelled

@github-actions

Copy link
Copy Markdown
Contributor

Vitest E2E Scenario Results — ⚠️ Run cancelled — no signal

Run: 28338756546
Workflow ref: codex/replace-3286-reasoning-endpoints
Requested scenarios: (default — all supported)
Requested jobs: onboard-repair-vitest
Summary: 0 passed, 0 failed, 1 cancelled, 0 skipped

Job Result
onboard-repair-vitest ⚠️ cancelled

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@github-actions

Copy link
Copy Markdown
Contributor

Vitest E2E Scenario Results — ✅ All requested jobs passed

Run: 28338925985
Workflow ref: codex/replace-3286-reasoning-endpoints
Requested scenarios: (default — all supported)
Requested jobs: messaging-compatible-endpoint-vitest
Summary: 1 passed, 0 failed, 0 cancelled, 0 skipped

Job Result
messaging-compatible-endpoint-vitest ✅ success

@github-actions

Copy link
Copy Markdown
Contributor

Vitest E2E Scenario Results — ✅ All requested jobs passed

Run: 28338925944
Workflow ref: codex/replace-3286-reasoning-endpoints
Requested scenarios: (default — all supported)
Requested jobs: onboard-resume-vitest
Summary: 1 passed, 0 failed, 0 cancelled, 0 skipped

Job Result
onboard-resume-vitest ✅ success

@github-actions

Copy link
Copy Markdown
Contributor

Vitest E2E Scenario Results — ✅ All requested jobs passed

Run: 28338925937
Workflow ref: codex/replace-3286-reasoning-endpoints
Requested scenarios: (default — all supported)
Requested jobs: onboard-repair-vitest
Summary: 1 passed, 0 failed, 0 cancelled, 0 skipped

Job Result
onboard-repair-vitest ✅ success

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@github-actions

Copy link
Copy Markdown
Contributor

Selective E2E Results — ✅ All requested jobs passed

Run: 28339119308
Target ref: c027b33d489f1d0073fdbda9c93e0ba428a0fb9c
Workflow ref: main
Requested jobs: onboard-resume-e2e,messaging-compatible-endpoint-e2e
Summary: 2 passed, 0 failed, 0 cancelled, 0 skipped

Job Result
messaging-compatible-endpoint-e2e ✅ success
onboard-resume-e2e ✅ success

@github-actions

Copy link
Copy Markdown
Contributor

Selective E2E Results — ⚠️ Some jobs cancelled — partial pass

Run: 28339069594
Target ref: codex/replace-3286-reasoning-endpoints
Requested jobs: onboard-resume-e2e,onboard-repair-e2e,cloud-onboard-e2e,messaging-compatible-endpoint-e2e
Summary: 3 passed, 0 failed, 1 cancelled, 0 skipped

Job Result
cloud-onboard-e2e ✅ success
messaging-compatible-endpoint-e2e ✅ success
onboard-repair-e2e ⚠️ cancelled
onboard-resume-e2e ✅ success

@github-actions

Copy link
Copy Markdown
Contributor

Vitest E2E Scenario Results — ⚠️ Some jobs cancelled — partial pass

Run: 28339070409
Workflow ref: codex/replace-3286-reasoning-endpoints
Requested scenarios: (default — all supported)
Requested jobs: onboard-resume-vitest,onboard-repair-vitest,messaging-compatible-endpoint-vitest
Summary: 2 passed, 0 failed, 1 cancelled, 0 skipped

Job Result
messaging-compatible-endpoint-vitest ✅ success
onboard-repair-vitest ⚠️ cancelled
onboard-resume-vitest ✅ success

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/inference-selection-validation.test.ts`:
- Around line 4-12: The afterEach hook in inference-selection-validation.test.ts
is unconditionally deleting NEMOCLAW_REASONING, which can clobber an existing
suite baseline; capture the original process.env.NEMOCLAW_REASONING value in the
test setup and restore that exact value after each test instead of always
deleting it. Update the test cleanup around
createInferenceSelectionValidationHelpers to preserve caller-provided
environment state.
🪄 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: 0d673f82-9832-4a07-83ed-f5589e233dd4

📥 Commits

Reviewing files that changed from the base of the PR and between c027b33 and 8947ccc.

📒 Files selected for processing (3)
  • docs/inference/inference-options.mdx
  • src/lib/onboard/compatible-endpoint-smoke.ts
  • src/lib/onboard/inference-selection-validation.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/lib/onboard/compatible-endpoint-smoke.ts
  • docs/inference/inference-options.mdx

Comment thread src/lib/onboard/inference-selection-validation.test.ts Outdated
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@cv

cv commented Jun 28, 2026

Copy link
Copy Markdown
Collaborator Author

Advisor follow-up for final head d9f6f81ec4f9b3c867e70c696b7a88aa53f7e817:

  • PRA-2 / PRA-16 — normalized state boundary: fixed the raw comparison in inference-selection-validation.ts; it now calls the shared normalizeReasoningFlag() parser. process.env.NEMOCLAW_REASONING is the intentional dynamic cross-phase handoff: fresh selection canonicalizes it, resume restores it from session.compatibleEndpointReasoning, and provider changes clear it before downstream config generation. Passing session state separately into this validator would create a second value and more plumbing; a module-level constant would cache stale state.
  • PRA-3 / PRA-8 / PRA-T6 — validation-bypass regression: inference-selection-validation.test.ts now proves that reasoning mode sets skipResponsesProbe=true, disables tool/streaming requirements, and still fails when the mandatory Chat Completions probe fails. A fake successful /responses result cannot bypass this path because /responses is deliberately not consulted; the Chat Completions result remains authoritative. The package-contract test independently proves that reasoning onboarding invokes /chat/completions, not /responses, and does not pass curl -N.
  • PRA-4 / PRA-11 — explicit maintainer override: keep the provider-inference tests co-located. The file is 749 lines, below the advisor's ~760-line threshold, and this PR is net +22 lines there after replacing internal mock-payload assertions with observable-result assertions. Extracting the few reasoning cases would duplicate the shared state-machine fixture and add bookkeeping without improving the tested boundary.
  • PRA-1 / PRA-5 / PRA-6 / PRA-7 — bounded compatibility behavior: this is an explicit Option 3 opt-in, not automatic validation weakening. Chat Completions remains mandatory. The source comments state that reasoning mode is OpenAI-compatible only (Anthropic/native providers use other formats), and the smoke JSDoc now records the invalid state and removal condition: reasoning-only endpoints may exhaust 512 tokens in reasoning_content; finish_reason=length retries at 1024 until compatible providers offer non-reasoning output. compatible-endpoint-smoke.test.ts pins that retry and final failure behavior.
  • PRA-15 — session-file growth rationale: the one durable field belongs in the existing session schema because resume must restore it. It uses the existing safe-update, explicit-null-clear, load, and debug-summary paths; extracting a one-field domain layer would add an abstraction without reducing coupling. onboard-session.test.ts covers persistence, safe output, and explicit clearing.
  • PRA-9 / PRA-T7 — default behavior: reasoning-disabled providers retain the existing Responses/tool/streaming probe defaults; the new logic changes only the explicit normalized-true branch. Existing onboard-probes.test.ts covers Responses/tool and streaming behavior, while the new test pins the inverse reasoning branch.
  • PRA-10 / PRA-T8 — preferred API precedence: the focused unit assertion verifies skipResponsesProbe=true directly when reasoning is enabled. That boolean is ORed ahead of NEMOCLAW_PREFERRED_API, so an openai-responses preference cannot re-enable the skipped probe; another env-matrix case would restate the same expression rather than cover a new boundary.
  • PRA-12 / PRA-13 / PRA-14: reasoning-mode.test.ts covers true/false aliases, whitespace/case, invalid/default-false, stored restore, and provider-change clear. setup-nim-selection.test.ts covers the custom-provider configuration hook; the call is structurally inside the selected.key === "custom" branch. Session tests cover persist/null-clear, and the resume E2Es below exercise the durable boundary. The public docs now list only supported aliases (true, 1, yes, y).

Runtime and acceptance follow-ups (PRA-T1PRA-T5 and the E2E advisor) are covered at this exact commit by these passing runs:

I am not adding a second combined live scenario: the package contract already combines the flag, custom selection, normalization, and probe path, while the required live resume/repair/messaging/cloud scenarios cover the external runtime boundaries. A new scenario would duplicate substantial setup for no distinct state transition.

All ordinary PR checks and automated advisor checks pass, all CodeRabbit threads are resolved, local commit/push hooks pass, npm run docs reports zero errors (two pre-existing warnings), and the documentation-writer pass found no remaining user-facing gap.

@wscurran wscurran added area: inference Inference routing, serving, model selection, or outputs area: onboarding Onboarding FSM, provider setup, sandbox launch, or first-run flow area: providers Inference provider integrations and provider behavior bug-fix PR fixes a bug or regression labels Jun 29, 2026
@cv cv added v0.0.71 and removed v0.0.70 labels Jun 29, 2026
Signed-off-by: Carlos Villela <cvillela@nvidia.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
test/package-contract/onboard/compatible-endpoint-reasoning.test.ts (1)

110-110: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Drop the log-text assertion.

Line 110 checks a specific console phrase instead of behavior. This test already proves the reasoning path through the returned selection and the captured curl invocations, so a harmless rewording would fail it unnecessarily.

As per path instructions, "Prefer observable outcomes through the public boundary over source-text, private-shape, or mock-call assertions."

Suggested change
-  assert.ok(payload.lines.some((line: string) => line.includes("tools and streaming")));
🤖 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 `@test/package-contract/onboard/compatible-endpoint-reasoning.test.ts` at line
110, Remove the log-text assertion from the compatible-endpoint-reasoning test
because it is checking a specific console phrase rather than observable
behavior. Update the test in compatible-endpoint-reasoning.test.ts to rely on
the returned selection and captured curl invocations only, and drop the
payload.lines.some(...includes("tools and streaming")) expectation so wording
changes won’t break it.

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.

Nitpick comments:
In `@test/package-contract/onboard/compatible-endpoint-reasoning.test.ts`:
- Line 110: Remove the log-text assertion from the compatible-endpoint-reasoning
test because it is checking a specific console phrase rather than observable
behavior. Update the test in compatible-endpoint-reasoning.test.ts to rely on
the returned selection and captured curl invocations only, and drop the
payload.lines.some(...includes("tools and streaming")) expectation so wording
changes won’t break it.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: abbb89a8-84ba-46b9-bf4f-079814b9a548

📥 Commits

Reviewing files that changed from the base of the PR and between d9f6f81 and 3ec53ff.

📒 Files selected for processing (5)
  • docs/inference/inference-options.mdx
  • src/lib/onboard/machine/handlers/provider-inference.test.ts
  • src/lib/onboard/setup-nim-selection.test.ts
  • src/lib/onboard/setup-nim-selection.ts
  • test/package-contract/onboard/compatible-endpoint-reasoning.test.ts
✅ Files skipped from review due to trivial changes (1)
  • docs/inference/inference-options.mdx
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/lib/onboard/setup-nim-selection.test.ts
  • src/lib/onboard/setup-nim-selection.ts

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@github-actions

Copy link
Copy Markdown
Contributor

Selective E2E Results — ⚠️ Some jobs cancelled — partial pass

Run: 28345606745
Target ref: 3ec53ff0b63c07853385410178e85141e7d74489
Workflow ref: main
Requested jobs: onboard-resume-e2e,messaging-compatible-endpoint-e2e
Summary: 1 passed, 0 failed, 1 cancelled, 0 skipped

Job Result
messaging-compatible-endpoint-e2e ⚠️ cancelled
onboard-resume-e2e ✅ success

@github-actions

Copy link
Copy Markdown
Contributor

Selective E2E Results — ✅ All requested jobs passed

Run: 28345801240
Target ref: 55705082ad821962c4094955369a05c50d1941b5
Workflow ref: main
Requested jobs: onboard-resume-e2e,messaging-compatible-endpoint-e2e
Summary: 2 passed, 0 failed, 0 cancelled, 0 skipped

Job Result
messaging-compatible-endpoint-e2e ✅ success
onboard-resume-e2e ✅ success

@github-actions

Copy link
Copy Markdown
Contributor

Vitest E2E Scenario Results — ✅ All requested jobs passed

Run: 28345713436
Workflow ref: codex/replace-3286-reasoning-endpoints
Requested scenarios: (default — all supported)
Requested jobs: onboard-resume-vitest,onboard-repair-vitest,messaging-compatible-endpoint-vitest
Summary: 3 passed, 0 failed, 0 cancelled, 0 skipped

Job Result
messaging-compatible-endpoint-vitest ✅ success
onboard-repair-vitest ✅ success
onboard-resume-vitest ✅ success

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@github-actions

Copy link
Copy Markdown
Contributor

Selective E2E Results — ✅ All requested jobs passed

Run: 28345712658
Target ref: codex/replace-3286-reasoning-endpoints
Requested jobs: onboard-resume-e2e,onboard-repair-e2e,cloud-onboard-e2e,messaging-compatible-endpoint-e2e
Summary: 4 passed, 0 failed, 0 cancelled, 0 skipped

Job Result
cloud-onboard-e2e ✅ success
messaging-compatible-endpoint-e2e ✅ success
onboard-repair-e2e ✅ success
onboard-resume-e2e ✅ success

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/lib/onboard/machine/handlers/provider-inference.test.ts (1)

235-245: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Avoid manufacturing the Dockerfile outcome inside the setupInference mock.

This assertion is currently proving the behavior of the test's own patchStagedDockerfile(...) call more than the handler's real resume path. If the production inference flow stopped preserving NEMOCLAW_REASONING, this unit would still pass. Keep this test focused on the cleared env / cleared reasoning state, and cover the Dockerfile contract in a higher-level path that exercises the real setup flow. As per path instructions, "**/*.test.{ts,js,mts,mjs,cts,cjs}: Review tests for behavioral confidence rather than implementation lock-in" and "Flag copied production algorithms, broad mocks that bypass the behavior under test."

Also applies to: 259-261

🤖 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/machine/handlers/provider-inference.test.ts` around lines 235
- 245, The `setupInference` mock is too implementation-heavy because it
manufactures the Dockerfile state by calling `patchStagedDockerfile(...)`, which
makes the test assert the mock’s behavior instead of the handler’s resume path.
Remove that Dockerfile mutation from the `setupInference` mock in
`provider-inference.test.ts` and keep this unit focused on the cleared
`NEMOCLAW_REASONING`/env-state assertions. Verify the Dockerfile contract in a
higher-level test that exercises the real setup flow through the handler or the
relevant inference path, using the existing `setupInference` and
`patchStagedDockerfile` symbols to locate the affected spots.

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.

Nitpick comments:
In `@src/lib/onboard/machine/handlers/provider-inference.test.ts`:
- Around line 235-245: The `setupInference` mock is too implementation-heavy
because it manufactures the Dockerfile state by calling
`patchStagedDockerfile(...)`, which makes the test assert the mock’s behavior
instead of the handler’s resume path. Remove that Dockerfile mutation from the
`setupInference` mock in `provider-inference.test.ts` and keep this unit focused
on the cleared `NEMOCLAW_REASONING`/env-state assertions. Verify the Dockerfile
contract in a higher-level test that exercises the real setup flow through the
handler or the relevant inference path, using the existing `setupInference` and
`patchStagedDockerfile` symbols to locate the affected spots.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 2c9a4a1d-c9ac-4c31-bbeb-66b619db8448

📥 Commits

Reviewing files that changed from the base of the PR and between 5570508 and b391609.

📒 Files selected for processing (1)
  • src/lib/onboard/machine/handlers/provider-inference.test.ts

@cv

cv commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator Author

Maintainer follow-up for b39160961:

I am accepting the reduced validation coverage in reasoning mode, with the current warning and tests, rather than adding another confirmation/guard.

  • NEMOCLAW_REASONING=true is an explicit compatibility opt-in for endpoints that can serve a reasoning model through Chat Completions but cannot pass the normal Responses/tool/streaming probes. Requiring those probes in this mode would make the mode unusable.
  • The residual risk is a capability mismatch and late runtime failure; this mode does not widen egress, expose credentials, or bypass sandbox isolation.
  • The CLI warns before sandbox creation that tools and streaming are unverified, and the canonical inference-options documentation carries the same warning.
  • compatible-endpoint-reasoning.test.ts is the requested acceptance regression: its fake endpoint succeeds only on /chat/completions, returns no tool call, is never streamed, and the test asserts that Responses and curl -N are absent and that the warning is emitted.
  • The exact reduced probe tuple is pinned in inference-selection-validation.test.ts. A second success-case test would exercise the same tuple without adding another invariant.
  • The positive flow is covered by the package contract; stale resume/provider-switch state is now covered in one vertical test through the real handler and real Dockerfile patcher. The detached artifact-only test was removed, leaving the final follow-up net +1 line.

I am also keeping the process environment as the single normalized cross-phase handoff. Provider selection restores or clears it before validation and artifact generation; threading a second boolean through the same callers would create parallel state that can diverge. The stitched regression proves the boundary that matters.

The platform support matrix remains a summary of tested providers; the mode-specific caveat belongs in the runtime warning and inference-options page, where users encounter the option. Duplicating it in generated support metadata would create another synchronization point.

For the remaining Nemotron items: the reasoning tests are now grouped under describe("compatible endpoint reasoning mode", ...), the repository test-file-size budget passes, normalization is already tested as a pure function separately from the intentional persistence helper, and the monolith/extraction notes are follow-up refactoring ideas rather than correctness blockers. No further code changes are planned for those items.

@github-actions

Copy link
Copy Markdown
Contributor

Selective E2E Results — ✅ All requested jobs passed

Run: 28346230583
Target ref: b39160961b522a44ddf70d1b39135cc7ed8186c2
Workflow ref: main
Requested jobs: onboard-resume-e2e,messaging-compatible-endpoint-e2e
Summary: 2 passed, 0 failed, 0 cancelled, 0 skipped

Job Result
messaging-compatible-endpoint-e2e ✅ success
onboard-resume-e2e ✅ success

@github-actions

Copy link
Copy Markdown
Contributor

Selective E2E Results — ✅ All requested jobs passed

Run: 28346217031
Target ref: codex/replace-3286-reasoning-endpoints
Requested jobs: cloud-onboard-e2e,onboard-repair-e2e,onboard-resume-e2e,messaging-compatible-endpoint-e2e
Summary: 4 passed, 0 failed, 0 cancelled, 0 skipped

Job Result
cloud-onboard-e2e ✅ success
messaging-compatible-endpoint-e2e ✅ success
onboard-repair-e2e ✅ success
onboard-resume-e2e ✅ success

@github-actions

Copy link
Copy Markdown
Contributor

Vitest E2E Scenario Results — ✅ All selected jobs passed

Run: 28346216992
Workflow ref: codex/replace-3286-reasoning-endpoints
Requested scenarios: messaging-compatible-endpoint,onboard-repair,onboard-resume
Requested jobs: (default — all default-enabled free-standing jobs; explicit-only jobs such as jetson-nvmap-gpu-vitest and sandbox-rlimits-connect-vitest are skipped unless selected)
Summary: 3 passed, 0 failed, 0 cancelled, 0 skipped

Job Result
messaging-compatible-endpoint-vitest ✅ success
onboard-repair-vitest ✅ success
onboard-resume-vitest ✅ success

@cv
cv merged commit c6113be into main Jun 29, 2026
191 checks passed
@cv
cv deleted the codex/replace-3286-reasoning-endpoints branch June 29, 2026 03:30
@miyoungc miyoungc mentioned this pull request Jun 30, 2026
21 tasks
jyaunches pushed a commit that referenced this pull request Jun 30, 2026
## Summary
Refreshes the v0.0.70 release docs from the release announcement and the
`v0.0.69..v0.0.70` commit range.
It also documents the `channels start` policy restoration behavior that
was missing from the shared OpenClaw and Hermes command references, and
bumps the Fern CLI version used for docs validation.

## Changes
- Replaced the stale `v0.0.70` release-notes entry with the actual
release themes, including CLI, onboarding, inference, messaging,
Windows, documentation, and release-validation changes.
- Documented that `channels start` reapplies the matching built-in
network policy preset before rebuild and rolls back to disabled if
policy restoration fails.
- Bumped `fern/fern.config.json` from `5.55.0` to `5.59.0` for the docs
refresh.
- Source summary:
- #5754 -> `docs/about/release-notes.mdx`: Notes Docker Desktop gateway
bridge retry behavior during onboarding.
- #5930 -> `docs/about/release-notes.mdx`: Links `nemoclaw use` default
sandbox selection to the command reference.
- #5948 -> `docs/about/release-notes.mdx`: Links reasoning-compatible
endpoint validation to inference documentation.
- #5950 -> `docs/about/release-notes.mdx`: Links Windows bootstrap WSL
recovery behavior to Windows preparation and troubleshooting docs.
- #5856 -> `docs/about/release-notes.mdx`: Notes rebuilt policy preset
registry repair.
- #5882 and #5949 -> `docs/about/release-notes.mdx`: Notes Hermes stale
base-image state repair.
- #6016 -> `docs/reference/commands.mdx`,
`docs/reference/commands-nemohermes.mdx`, and
`docs/manage-sandboxes/messaging-channels.mdx`: Documents channel policy
restoration and rollback on `channels start`.
- #5859 -> `docs/about/release-notes.mdx`: Links quickstart network
approval guidance.
- #5863 -> `docs/about/release-notes.mdx`: Links Teams allowlist
guidance in the messaging page.
- #5756, #5926, #6010, and #6011 -> `docs/about/release-notes.mdx`:
Summarizes the Vitest E2E validation cutover.

## 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: doc-only prose refresh with
no runtime behavior change.
- [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
- [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)
- [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)

`npm run docs` exited 0 and Fern reported one existing light-mode accent
contrast warning.
`fern check --warnings` confirmed the warning is the site theme contrast
ratio, not content introduced by this PR.

---
Signed-off-by: Miyoung Choi <miyoungc@nvidia.com>
Hadar301 pushed a commit to Hadar301/NemoClaw-OpenShift that referenced this pull request Jul 12, 2026
<!-- markdownlint-disable MD041 -->
## Summary
<!-- 1-3 sentences: what this PR does and why. -->

Custom OpenAI-compatible providers can opt into reasoning-mode
validation through `NEMOCLAW_REASONING`, preserving that choice through
provider handoff and resume. This clean-history replacement supersedes
NVIDIA#3286 while retaining Deepak Jain's attribution and all fixes from its
review.

## Related Issue
<!-- Fixes #NNN or Closes #NNN. Remove this section if none. -->

Fixes NVIDIA#3279.
Supersedes NVIDIA#3286.

## Changes
<!-- Bullet list of key changes. -->

- Normalize common reasoning aliases and persist explicit set and clear
operations in onboarding session state.
- Skip Responses API, tool-call, and streaming probes that
reasoning-only compatible endpoints reject.
- Retry length-limited `reasoning_content` and `reasoning` smoke
responses with a larger output budget, then fail clearly if final
content remains empty.
- Carry reasoning state through provider selection, resume, flow
context, debug summaries, and sandbox creation.
- Add focused source and package-contract coverage for aliases, probe
selection, persistence, null clearing, resume, and response fallback
behavior.
- Keep `src/lib/onboard.ts` net-neutral and synchronize generated
platform documentation citations.
- Validation: CLI build and typecheck, focused source tests, the
package-contract test, 18 platform-doc generator tests, full commit
hooks, and push hooks pass. `npm run docs` reports zero errors and two
pre-existing Fern warnings.

## Type of Change

- [ ] Code change (feature, bug fix, or refactor)
- [x] Code change with doc updates
- [ ] Doc only (prose changes, no code sample modifications)
- [ ] 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. -->
- [x] Tests added or updated for changed behavior
- [ ] Existing tests cover changed behavior — justification:
- [ ] Tests not applicable — justification:
- [x] Docs updated for user-facing behavior changes
- [ ] Docs not applicable — justification:
- [x] Sensitive paths changed (security, policy, credentials, preflight,
onboarding, inference, runner, sandbox, or messaging)
- [x] Sensitive-path review completed or maintainer-approved waiver
recorded — reviewer/approval link/justification: maintainer salvage
review completed; all historical CodeRabbit findings and the resolved
thread in NVIDIA#3286 were incorporated
and revalidated.
- [ ] 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
- [x] 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)
- [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: Carlos Villela <cvillela@nvidia.com>


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **New Features**
* Added a new reasoning mode for compatible OpenAI-style endpoints, with
support for saving and restoring this setting during setup.
* Compatible-endpoint checks now use a more tailored validation flow
when reasoning mode is enabled.

* **Bug Fixes**
* Improved handling of resumed onboarding so stale reasoning settings
are cleared when switching providers.
* Increased the default token budget for compatible-endpoint sandbox
smoke checks.

* **Documentation**
* Updated setup and platform docs to cover reasoning mode and refreshed
reference links.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Signed-off-by: Deepak Jain <deepujain@gmail.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Hadar301 pushed a commit to Hadar301/NemoClaw-OpenShift that referenced this pull request Jul 12, 2026
## Summary
Refreshes the v0.0.70 release docs from the release announcement and the
`v0.0.69..v0.0.70` commit range.
It also documents the `channels start` policy restoration behavior that
was missing from the shared OpenClaw and Hermes command references, and
bumps the Fern CLI version used for docs validation.

## Changes
- Replaced the stale `v0.0.70` release-notes entry with the actual
release themes, including CLI, onboarding, inference, messaging,
Windows, documentation, and release-validation changes.
- Documented that `channels start` reapplies the matching built-in
network policy preset before rebuild and rolls back to disabled if
policy restoration fails.
- Bumped `fern/fern.config.json` from `5.55.0` to `5.59.0` for the docs
refresh.
- Source summary:
- NVIDIA#5754 -> `docs/about/release-notes.mdx`: Notes Docker Desktop gateway
bridge retry behavior during onboarding.
- NVIDIA#5930 -> `docs/about/release-notes.mdx`: Links `nemoclaw use` default
sandbox selection to the command reference.
- NVIDIA#5948 -> `docs/about/release-notes.mdx`: Links reasoning-compatible
endpoint validation to inference documentation.
- NVIDIA#5950 -> `docs/about/release-notes.mdx`: Links Windows bootstrap WSL
recovery behavior to Windows preparation and troubleshooting docs.
- NVIDIA#5856 -> `docs/about/release-notes.mdx`: Notes rebuilt policy preset
registry repair.
- NVIDIA#5882 and NVIDIA#5949 -> `docs/about/release-notes.mdx`: Notes Hermes stale
base-image state repair.
- NVIDIA#6016 -> `docs/reference/commands.mdx`,
`docs/reference/commands-nemohermes.mdx`, and
`docs/manage-sandboxes/messaging-channels.mdx`: Documents channel policy
restoration and rollback on `channels start`.
- NVIDIA#5859 -> `docs/about/release-notes.mdx`: Links quickstart network
approval guidance.
- NVIDIA#5863 -> `docs/about/release-notes.mdx`: Links Teams allowlist
guidance in the messaging page.
- NVIDIA#5756, NVIDIA#5926, NVIDIA#6010, and NVIDIA#6011 -> `docs/about/release-notes.mdx`:
Summarizes the Vitest E2E validation cutover.

## 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: doc-only prose refresh with
no runtime behavior change.
- [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
- [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)
- [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)

`npm run docs` exited 0 and Fern reported one existing light-mode accent
contrast warning.
`fern check --warnings` confirmed the warning is the site theme contrast
ratio, not content introduced by this PR.

---
Signed-off-by: Miyoung Choi <miyoungc@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: inference Inference routing, serving, model selection, or outputs area: onboarding Onboarding FSM, provider setup, sandbox launch, or first-run flow area: providers Inference provider integrations and provider behavior bug-fix PR fixes a bug or regression

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NEMOCLAW_REASONING not configurable for Option 3 providers; reasoning-only models fail silently

2 participants