fix: support reasoning compatible endpoints (Fixes #3279)#3286
Open
deepujain wants to merge 4 commits into
Open
fix: support reasoning compatible endpoints (Fixes #3279)#3286deepujain wants to merge 4 commits into
deepujain wants to merge 4 commits into
Conversation
Fixes NVIDIA#3279 Signed-off-by: Deepak Jain <deepujain@gmail.com>
Contributor
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds normalization and configuration helpers for a compatible-endpoint reasoning mode, persists the flag in onboarding session state, applies it during provider selection/resume, conditions custom OpenAI-like endpoint validation and smoke probes on the flag (including larger token budget and reasoning-field fallbacks), and adds unit/integration tests. ChangesReasoning mode support for OpenAI-compatible endpoints
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/onboard-selection.test.ts (1)
2075-2094: ⚡ Quick winAssert probe path behavior explicitly in reasoning mode.
Line [2077] and Line [2150] currently validate end-state only; this test can still pass if
/responses(or streaming probe) is called and then fallback succeeds. Please log curl args and assert reasoning mode avoids/responses/-Nwhile still hitting/chat/completions.Suggested test hardening
@@ const scriptPath = path.join(tmpDir, "custom-openai-reasoning-check.js"); + const curlArgsLog = path.join(tmpDir, "custom-openai-reasoning-curl-args.log"); @@ path.join(fakeBin, "curl"), `#!/usr/bin/env bash +args_log=${JSON.stringify(curlArgsLog)} +printf '%s\\n' "$*" >> "$args_log" body='{"error":{"message":"bad request"}}' status="400" outfile="" url="" @@ assert.equal(payload.result.model, "reasoning-model"); assert.equal(payload.result.preferredInferenceApi, "openai-completions"); assert.equal(payload.reasoning, "true"); + const curlInvocations = fs.readFileSync(curlArgsLog, "utf-8"); + assert.match(curlInvocations, /chat\/completions/); + assert.doesNotMatch(curlInvocations, /\/responses/); + assert.doesNotMatch(curlInvocations, /(^|\s)-N(\s|$)/); assert.ok( payload.messages.every( (message: string) => !/Enable reasoning mode for this model/.test(message), ), );Also applies to: 2150-2160
🤖 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/onboard-selection.test.ts` around lines 2075 - 2094, The fake curl script written to path.join(fakeBin, "curl") should record its received arguments so the test can assert probe behavior: modify the heredoc to append the full "$@" or each processed arg into a temp log file (e.g., "$outfile.args.log" or a known capture file in the test dir) before handling -o/url logic, and keep returning the same body/status logic; then update the test assertions to read that captured args log and explicitly assert that in reasoning mode the captured args do not include '/responses' nor the '-N' flag and do include a call to '/chat/completions'. Ensure you reference the same fakeBin/curl generation code and the test's reasoning-mode invocation when adding the new assertions.test/onboard.test.ts (1)
391-410: ⚡ Quick winAdd an explicit assertion for the unset/default reasoning path.
This test validates aliases, but not the key default behavior when
NEMOCLAW_REASONINGis unset. Locking that in will better guard the PR’s intended fallback semantics.✅ Suggested test addition
expect(normalizeReasoningFlag("maybe")).toBeNull(); + delete process.env.NEMOCLAW_REASONING; + await expect(configureCompatibleEndpointReasoning()).resolves.toBe("false"); + expect(process.env.NEMOCLAW_REASONING).toBe("false"); + process.env.NEMOCLAW_REASONING = "yes"; await expect(configureCompatibleEndpointReasoning()).resolves.toBe("true"); expect(process.env.NEMOCLAW_REASONING).toBe("true");🤖 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/onboard.test.ts` around lines 391 - 410, Add an explicit assertion that the "unset/default" path is covered: before setting NEMOCLAW_REASONING to "yes", delete process.env.NEMOCLAW_REASONING (or set it to undefined) and call await expect(configureCompatibleEndpointReasoning()).resolves.toBe(normalizeReasoningFlag(undefined)); this ensures the test asserts the function's fallback behavior when NEMOCLAW_REASONING is not present and references the existing normalizeReasoningFlag and configureCompatibleEndpointReasoning symbols.
🤖 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.ts`:
- Around line 1508-1511: The configureCompatibleEndpointReasoning function
currently stores the normalized NEMOCLAW_REASONING in process.env only, causing
stale or missing values across onboarding flows; update it to persist the
normalized value into the selected provider state object (use the same
normalization via normalizeReasoningFlag) instead of relying solely on
process.env, and ensure that when provider selection changes the stored flag is
cleared from that provider state and process.env is restored/updated
accordingly; locate configureCompatibleEndpointReasoning and related calls to
normalizeReasoningFlag and modify the provider selection/restore logic where
provider state is mutated so the flag is saved with the provider, removed on
selection change, and used to seed process.env when a provider is active (also
apply the same change pattern to the other occurrence referenced near the file's
later provider-state handling).
---
Nitpick comments:
In `@test/onboard-selection.test.ts`:
- Around line 2075-2094: The fake curl script written to path.join(fakeBin,
"curl") should record its received arguments so the test can assert probe
behavior: modify the heredoc to append the full "$@" or each processed arg into
a temp log file (e.g., "$outfile.args.log" or a known capture file in the test
dir) before handling -o/url logic, and keep returning the same body/status
logic; then update the test assertions to read that captured args log and
explicitly assert that in reasoning mode the captured args do not include
'/responses' nor the '-N' flag and do include a call to '/chat/completions'.
Ensure you reference the same fakeBin/curl generation code and the test's
reasoning-mode invocation when adding the new assertions.
In `@test/onboard.test.ts`:
- Around line 391-410: Add an explicit assertion that the "unset/default" path
is covered: before setting NEMOCLAW_REASONING to "yes", delete
process.env.NEMOCLAW_REASONING (or set it to undefined) and call await
expect(configureCompatibleEndpointReasoning()).resolves.toBe(normalizeReasoningFlag(undefined));
this ensures the test asserts the function's fallback behavior when
NEMOCLAW_REASONING is not present and references the existing
normalizeReasoningFlag and configureCompatibleEndpointReasoning symbols.
🪄 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: 78ba4585-9297-4c89-b6ae-5ad64ecf87cb
📒 Files selected for processing (3)
src/lib/onboard.tstest/onboard-selection.test.tstest/onboard.test.ts
Signed-off-by: Test User <test@example.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/state/onboard-session.ts (1)
707-717:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPersist
compatibleEndpointReasoningin the safe update whitelist.
SessionUpdatesadds this field (Line 131), butfilterSafeUpdates()never forwards it, so updates passed viamarkStepComplete()/completeSession()are silently dropped. That breaks both setting and clearing (null) this value during provider transitions.Suggested fix
export function filterSafeUpdates(updates: SessionUpdates): Partial<Session> { const safe: Partial<Session> = {}; if (!isObject(updates)) return safe; if (typeof updates.sandboxName === "string") safe.sandboxName = updates.sandboxName; if (typeof updates.provider === "string") safe.provider = updates.provider; if (typeof updates.model === "string") safe.model = updates.model; if (typeof updates.endpointUrl === "string") safe.endpointUrl = redactUrl(updates.endpointUrl); if (typeof updates.credentialEnv === "string") safe.credentialEnv = updates.credentialEnv; if (typeof updates.preferredInferenceApi === "string") safe.preferredInferenceApi = updates.preferredInferenceApi; + if (typeof updates.compatibleEndpointReasoning === "string") { + safe.compatibleEndpointReasoning = updates.compatibleEndpointReasoning; + } else if (updates.compatibleEndpointReasoning === null) { + safe.compatibleEndpointReasoning = null; + } if (typeof updates.nimContainer === "string") safe.nimContainer = updates.nimContainer;🤖 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` around lines 707 - 717, filterSafeUpdates currently omits the SessionUpdates field compatibleEndpointReasoning so updates from markStepComplete/completeSession are dropped; update the function filterSafeUpdates to accept and propagate compatibleEndpointReasoning (including allowing null to clear it) into the returned Partial<Session> when typeof updates.compatibleEndpointReasoning is "string" or when updates.compatibleEndpointReasoning === null, mirroring how other nullable/optional fields are handled so provider transitions can set and clear this value.
🤖 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.
Outside diff comments:
In `@src/lib/state/onboard-session.ts`:
- Around line 707-717: filterSafeUpdates currently omits the SessionUpdates
field compatibleEndpointReasoning so updates from
markStepComplete/completeSession are dropped; update the function
filterSafeUpdates to accept and propagate compatibleEndpointReasoning (including
allowing null to clear it) into the returned Partial<Session> when typeof
updates.compatibleEndpointReasoning is "string" or when
updates.compatibleEndpointReasoning === null, mirroring how other
nullable/optional fields are handled so provider transitions can set and clear
this value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 06d7e88a-98bf-4a0d-89a9-ddd0edb6c09a
📒 Files selected for processing (4)
src/lib/onboard.tssrc/lib/state/onboard-session.tstest/onboard-selection.test.tstest/onboard.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- test/onboard-selection.test.ts
- test/onboard.test.ts
- src/lib/onboard.ts
Signed-off-by: Test User <test@example.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/state/onboard-session.ts (1)
145-166: ⚡ Quick winInclude
compatibleEndpointReasoningin debug summary output.The new persisted field is not exposed by
DebugSessionSummary/summarizeForDebug, which makes reasoning-mode resume issues harder to diagnose from debug dumps.Proposed patch
export interface DebugSessionSummary { version: number; sessionId: string; status: string; resumable: boolean; mode: string; startedAt: string; updatedAt: string; sandboxName: string | null; provider: string | null; model: string | null; endpointUrl: string | null; credentialEnv: string | null; preferredInferenceApi: string | null; + compatibleEndpointReasoning: string | null; nimContainer: string | null; policyPresets: string[] | null; gpuPassthrough: boolean; lastStepStarted: string | null; lastCompletedStep: string | null; failure: SessionFailure | null; steps: Record<string, StepState>; } @@ endpointUrl: redactUrl(session.endpointUrl), credentialEnv: session.credentialEnv, preferredInferenceApi: session.preferredInferenceApi, + compatibleEndpointReasoning: session.compatibleEndpointReasoning, nimContainer: session.nimContainer, policyPresets: session.policyPresets,Also applies to: 852-867
🤖 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` around lines 145 - 166, DebugSessionSummary and the summarizeForDebug output are missing the new persisted field compatibleEndpointReasoning, so add compatibleEndpointReasoning: string | null (matching the persisted type) to the DebugSessionSummary interface and update the summarizeForDebug function to read the session-compatibleEndpointReasoning value and include it in the returned summary object (alongside fields like failure, steps, nimContainer, preferredInferenceApi, etc.) so debug dumps expose reasoning-mode compatibility; update any related spots mentioned (e.g., the summarizeForDebug implementation around the lastStep/lastCompletedStep logic) to populate this field.
🤖 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/state/onboard-session.ts`:
- Around line 145-166: DebugSessionSummary and the summarizeForDebug output are
missing the new persisted field compatibleEndpointReasoning, so add
compatibleEndpointReasoning: string | null (matching the persisted type) to the
DebugSessionSummary interface and update the summarizeForDebug function to read
the session-compatibleEndpointReasoning value and include it in the returned
summary object (alongside fields like failure, steps, nimContainer,
preferredInferenceApi, etc.) so debug dumps expose reasoning-mode compatibility;
update any related spots mentioned (e.g., the summarizeForDebug implementation
around the lastStep/lastCompletedStep logic) to populate this field.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f8e80300-8c3d-41f3-be29-8da2e7314cd8
📒 Files selected for processing (1)
src/lib/state/onboard-session.ts
Signed-off-by: Test User <test@example.com>
wscurran
added
enhancement: provider
Contributor
|
✨ Thanks for submitting this detailed PR about supporting reasoning-compatible endpoints for custom OpenAI providers. This change aims to improve the compatibility of NemoClaw with reasoning-mode validation through the NEMOCLAW_REASONING flag. Related open issues: |
wscurran
added
enhancement: inference
This was referenced May 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Custom OpenAI-compatible providers can now opt into reasoning-mode validation through
NEMOCLAW_REASONING. When that flag is enabled, NemoClaw skips the Responses/tool-call probe that reasoning-only wrappers often reject, and the sandbox smoke check accepts reasoning text as valid output.Fixes #3279.
Changes
NEMOCLAW_REASONINGaliases such asyes,1,no, and0.reasoning_contentorreasoningwhenmessage.contentis empty.Testing
npm run build:clipassed.npm test -- test/onboard.test.ts -t "normalizes compatible-endpoint reasoning"passed.npm test -- test/onboard-selection.test.ts -t "honors NEMOCLAW_REASONING"passed.HOME=/private/tmp/nemoclaw-test-home-3279b npm test -- --reporter=dotwas attempted. The local full suite still fails in existing installer/preinstall tests, includingtest/install-preflight.test.tsandtest/preinstall-node-version.test.ts.Evidence it works
The focused custom endpoint test simulates a reasoning-only chat response with empty
contentand non-emptyreasoning_content; withNEMOCLAW_REASONING=yes, onboarding accepts the provider and keepspreferredInferenceApion chat completions.Summary by CodeRabbit
New Features
Tests