fix(onboard): accept reasoning-mode models in the inference smoke probe - #3356
fix(onboard): accept reasoning-mode models in the inference smoke probe#3356latenighthackathon wants to merge 2 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe smoke probe for compatible-endpoint validation now budgets higher token limits and tolerates reasoning-only responses by accepting non-empty reasoning fields. Response parsing is defensive against malformed choices arrays. Tests cover token budget, reasoning acceptance, and safe choices handling. ChangesThinking Model Compatibility
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~10 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)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/onboard.ts`:
- Around line 2579-2593: The parsing code can crash or mis-detect valid
responses: fix by guarding access to choices and by selecting a string-only
reasoning field. First, ensure you safely extract message by verifying
data.get("choices") is a non-empty list and that choices[0] is a dict before
calling .get (replace the current data.get("choices", [{}])[0] usage with an
explicit check for a list and len>0, then assign message = choices[0] if it's a
dict). Second, change the reasoning selection so you don't hide a string
reasoning_content behind a truthy non-string reasoning: explicitly test
message.get("reasoning_content") and message.get("reasoning") for str and
non-empty (prefer reasoning_content if it's a non-empty str, else reasoning if
it's a non-empty str) before treating it as a valid liveness signal; also ensure
content is only accepted if it's a str and non-empty.
🪄 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: d750ee9d-f538-4a9f-8895-63cc18cc13a6
📒 Files selected for processing (2)
src/lib/onboard.tstest/onboard.test.ts
Two edge cases CodeRabbit flagged on PR NVIDIA#3356 (NVIDIA#3341 follow-up): 1. `data.get("choices", [{}])[0]` defaults to `[{}]` only when `choices` is missing; it still raised `IndexError` when the endpoint returned `choices: []` (which some vLLM error paths do). Guard explicitly: require a non-empty list whose first element is a dict before indexing; emit a clear failure when not. 2. `message.get("reasoning") or message.get("reasoning_content")` short-circuits on any truthy value. If `reasoning` was a non-string truthy value (e.g. `{}` or `[]` from a malformed provider response), the `or` would return it and a valid string `reasoning_content` was never seen. Iterate over both candidates and pick the first non-empty string, mirroring the `content` check shape. Adds a third test case asserting the choices-empty guard pattern is present in the generated script. The Python parser was exercised against eight payloads (normal, reasoning-only, reasoning_content, empty `choices`, missing `choices`, non-string `reasoning` masking valid `reasoning_content`, non-dict `choices[0]`, empty payload) and behaves as expected: three pass with a smoke-OK line, five fail cleanly with a descriptive error. Signed-off-by: latenighthackathon <support@latenighthackathon.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/onboard.test.ts (1)
706-722: ⚡ Quick winTighten parser-hardening assertions to match the stated fix scope
At Line 706 and Line 714, the new tests check key branches, but they don’t explicitly lock in the “non-empty string only” and “non-dict choice entry” guards described in the PR notes. Adding those assertions would prevent silent regressions in parser validation behavior.
Suggested test additions
it("accepts a reasoning-only response as a valid smoke signal for thinking models (`#3341`)", () => { const script = buildCompatibleEndpointSandboxSmokeScript("Qwen/Qwen3.6-27B"); assert.match(script, /message\.get\("reasoning"\)/); assert.match(script, /message\.get\("reasoning_content"\)/); + // Ensure fallback only accepts non-empty string reasoning payloads. + assert.match(script, /isinstance\([^,]+,\s*str\)/); + assert.match(script, /\.strip\(\)/); assert.match(script, /reasoning-only response/); }); it("guards against empty or malformed choices arrays in the smoke parser (`#3341`)", () => { const script = buildCompatibleEndpointSandboxSmokeScript("Qwen/Qwen3.6-27B"); assert.match(script, /not isinstance\(choices, list\) or not choices/); + // Ensure first choice shape is validated before message access. + assert.match(script, /not isinstance\([^,]+,\s*dict\)/); assert.doesNotMatch(script, /data\.get\("choices", \[\{\}\]\)\[0\]/); });🤖 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 706 - 722, Update the two smoke-parser tests that use buildCompatibleEndpointSandboxSmokeScript to also assert the tightened guards: for the reasoning-only branch assert the parser checks that reasoning is a non-empty string (e.g., match a pattern like 'not isinstance\\(reasoning, str\\) or not reasoning' or similar) and for the choices guard assert there is an explicit check that each choice is a dict (e.g., match 'not all\\(isinstance\\(c, dict\\) for c in choices\\)' or 'not isinstance\\(choice, dict\\)') so the tests lock in both the “non-empty string only” and “non-dict choice entry” validations in the smoke parser used by buildCompatibleEndpointSandboxSmokeScript.
🤖 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/onboard.test.ts`:
- Around line 706-722: Update the two smoke-parser tests that use
buildCompatibleEndpointSandboxSmokeScript to also assert the tightened guards:
for the reasoning-only branch assert the parser checks that reasoning is a
non-empty string (e.g., match a pattern like 'not isinstance\\(reasoning, str\\)
or not reasoning' or similar) and for the choices guard assert there is an
explicit check that each choice is a dict (e.g., match 'not
all\\(isinstance\\(c, dict\\) for c in choices\\)' or 'not isinstance\\(choice,
dict\\)') so the tests lock in both the “non-empty string only” and “non-dict
choice entry” validations in the smoke parser used by
buildCompatibleEndpointSandboxSmokeScript.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5d4a7388-e7a1-42de-bf37-c233676eca29
📒 Files selected for processing (2)
src/lib/onboard.tstest/onboard.test.ts
|
✨ Thanks for submitting this detailed PR to fix the onboard issue with thinking models like Qwen3.6. This change aims to improve the inference smoke probe by bumping max_tokens to 256 and treating non-empty message.reasoning or message.reasoning_content as a valid signal. Related open issues: |
…guards CodeRabbit nit on PR NVIDIA#3356: the existing smoke-parser tests asserted the high-level branches (reasoning fallback present, choices-list guard present) but did not lock in the two finer guarantees the prior fixup explicitly added: - The reasoning fallback only treats a non-empty STRING reasoning payload as a liveness signal (the prior `or`-chain regression where a truthy non-string `reasoning` masked a valid string `reasoning_content`). Add `assert.match(script, /isinstance(value, str) and value.strip()/)` to the reasoning-only test so a future refactor that drops the isinstance check fails CI. - The choices guard validates `choices[0]` is a dict before calling `.get("message", {})`, which is what catches the `choices=["str"]` / `choices=[null]` shapes from misbehaving providers. Add `assert.match(script, /not isinstance(choices[0], dict)/)` so the same regression in the choice-shape path is caught. Source-only test, no behavior change. The PR NVIDIA#3356 source already satisfies both assertions; this commit just makes the test surface match the prose in the original commit message. Signed-off-by: latenighthackathon <support@latenighthackathon.com>
12c9526 to
9218417
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/onboard/compatible-endpoint-smoke.test.ts`:
- Around line 57-59: The negative assertion in the test currently checks for the
exact string '"max_tokens": 32,' which misses variants without the trailing
comma; update the assertion that uses the variable script in
compatible-endpoint-smoke.test.ts (the expect(script).not... line) to use a
regex-based negative match that rejects any occurrence of "max_tokens" followed
by optional whitespace, a colon, optional whitespace, and the value 32 (so it
fails for both '"max_tokens": 32,' and '"max_tokens": 32'), leaving the positive
check for 256 intact.
🪄 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: 3422c17a-424f-4fc0-a50c-3b43e524700c
📒 Files selected for processing (2)
src/lib/onboard/compatible-endpoint-smoke.test.tssrc/lib/onboard/compatible-endpoint-smoke.ts
CodeRabbit nit on PR NVIDIA#3356: the negative assertion checked the literal substring `"max_tokens": 32,` which only catches the comma-trailed form. A regression to `"max_tokens": 32` (no trailing comma — e.g. if it were the last field) would slip past the guard. Switch to a regex that anchors on the value with `\b` so both forms fail the test. Signed-off-by: latenighthackathon <support@latenighthackathon.com>
Closes NVIDIA#3341. The Option-3 compatible-endpoint smoke probe sent max_tokens=32 and required choices[0].message.content to be a non-empty string. Thinking-mode models like Qwen3.6 in vLLM with --reasoning-parser exhaust the entire 32-token budget on the reasoning chain and return content=null with finish_reason=length, even though the endpoint is healthy and the round-trip succeeded. Validation then rejected the response and onboard exited without the user having a way to use the model. Three changes in the embedded smoke script (now in `src/lib/onboard/compatible-endpoint-smoke.ts` after the NVIDIA#3297 extract): 1. Bump max_tokens 32 -> 256 so short reasoning chains have room to finish before content starts. 256 is still a cheap probe (~$0.001 on most paid providers for a single "say PONG") but gives a thinking model enough headroom to land a content token. 2. Treat a non-empty message.reasoning or message.reasoning_content as a valid smoke signal when content is null/empty. vLLM emits "reasoning" (per the qwen3 reasoning parser, observed in the NVIDIA#3341 trace) and OpenAI o1-style endpoints emit "reasoning_content"; both indicate the endpoint round-tripped a chat completion. Use a next() over both candidates so a truthy non-string value in one field cannot mask a valid string in the other. 3. Harden the parser against empty or malformed `choices` arrays: the previous `data.get("choices", [{}])[0]` defaulted only when `choices` was missing; it raised IndexError on `choices: []` and AttributeError on `choices: ["str"]`. Validate that choices is a non-empty list whose first element is a dict before indexing, and coerce a missing message to an empty dict. Three new test cases in `compatible-endpoint-smoke.test.ts` assert the bumped max_tokens, the reasoning fallback shape (including the isinstance+strip guard), and the choices-array hardening. Signed-off-by: latenighthackathon <support@latenighthackathon.com>
CodeRabbit nit on PR NVIDIA#3356: the negative assertion checked the literal substring `"max_tokens": 32,` which only catches the comma-trailed form. A regression to `"max_tokens": 32` (no trailing comma — e.g. if it were the last field) would slip past the guard. Switch to a regex that anchors on the value with `\b` so both forms fail the test. Signed-off-by: latenighthackathon <support@latenighthackathon.com>
cccfa80 to
f1e6bf4
Compare
|
Closing as superseded by #3514, which landed 2026-05-13 and addresses the same root cause #3341. #3514 takes a more thorough approach: it distinguishes route/config failures from model-budget failures, retries with a larger token budget (256 → 1024) when reasoning_content exhausts the initial budget, and adds an executable regression test for the MiniMax-shaped reasoning-only response. The compatible-endpoint smoke probe was also refactored in #3297 since this PR opened, so a rebase would have to be substantially reworked anyway. Thanks for the review attention here. Cheers! |
Summary
Closes #3341. The Option-3 compatible-endpoint smoke probe sent
max_tokens=32and requiredchoices[0].message.contentto be a non-empty string. Thinking-mode models like Qwen3.6 in vLLM with--reasoning-parser qwen3exhaust the entire 32-token budget on the reasoning chain and returncontent=nullwithfinish_reason="length", even though the endpoint is healthy and the round-trip succeeded. Onboard then rejected the response and exited at step[7/8] Validating inference.Related Issue
Closes #3341
Changes
Two changes in the embedded smoke script in
src/lib/onboard.ts:max_tokensfrom 32 to 256 so short reasoning chains have room to finish before content starts. 256 is still a cheap probe (~$0.001 on most paid providers for a single "say PONG") but gives a thinking model enough headroom to emit a content token.message.reasoningormessage.reasoning_contentas a valid smoke signal whencontentis null or empty. vLLM emitsreasoning(per the qwen3 reasoning parser, observed in the Cannot use thinking models like Qwen 3.6 27B - Nemoclaw validation request token length too small. #3341 trace) and OpenAI o1-style endpoints emitreasoning_content; both indicate the endpoint round-tripped a chat completion. The probe is a liveness check, not a response-shape contract, so a reasoning-only response is enough to declare the model reachable. The success line becomesINFERENCE_SMOKE_OK (reasoning-only response, N chars)so the distinction is visible in onboard logs.Type of Change
Verification
npx prek run --all-filespasses for the staged files.test/onboard.test.ts(#3341): one asserts the bumpedmax_tokensis in the generated script, one asserts the reasoning-fallback branch is present.npx vitest run -t 'smoke|reasoning|max_tokens' test/onboard.test.ts— 6/6 pass (4 existing smoke tests + the 2 new ones).reasoning_contento1 shape, empty response). The first three pass withINFERENCE_SMOKE_OK, the empty payload fails as expected. Output:npm run build:cliclean.Rebased on current
upstream/main(commiteb15e55e).Signed-off-by: latenighthackathon latenighthackathon@users.noreply.github.com
Summary by CodeRabbit
Tests
Improvements