Skip to content

fix(provider): honor NaN DeepSeek output limits - #2359

Merged
decode2 merged 1 commit into
mainfrom
fix/nan-reviewer-token-limits
Oct 17, 2026
Merged

decode2 merged 1 commit into
mainfrom
fix/nan-reviewer-token-limits

Conversation

@decode2

@decode2 decode2 commented Oct 17, 2026 •

Copy link
Copy Markdown
Member

Summary

Closes #2357
Refs #1259

Approved coding targets

Approved issue Coding scope in this PR Closure
#2357 — status:approved DeepSeek output-limit wire compatibility and isolated model snapshots. Closed by this PR.
#1259 — status:approved Partial implementation: safe completion facts for EMPTY_OUTPUT, with relay/public projection tests. Other refusal sites and requested-limit evidence remain outside this PR. Reference only; keep the issue open.
  • Send NaN DeepSeek's existing output limit as max_tokens, the field enforced by the tested endpoint, using the SDK's existing compatibility option.
  • Preserve bounded model identity, valid token counters and content-shape counters when an in-process completion has no text. Do not copy provider errors, answers, thinking, signatures, auth or arbitrary payload fields into that diagnostic.
  • Keep the model, effort, 16,384-token metadata, endpoint, failure classification and single-attempt behavior unchanged.

Type

  • Bug fix (type:bug)

Why

The controlled comparison in #2357 used the same model, synthetic prompt and reasoning_effort: none: max_completion_tokens: 64 produced 799 tokens with a normal finish, while max_tokens: 64 stopped at 64. The existing SDK defaults the model metadata into the former field unless compatibility is specified.

This proves the observed field mismatch, not universal server behavior or the cause of historical empty-output failures. Those failures lacked the counters needed for diagnosis. This PR does not change the user's routing or claim that every historical empty completion is fixed.

Review path

Area Change
lib/nan-provider.ts DeepSeek-only compatibility; clone the nested compatibility object so returned model snapshots stay isolated. Other models and configured caps/efforts stay unchanged.
lib/inprocess-reviewer.ts Allowlisted, finite non-negative safe-integer diagnostics on EMPTY_OUTPUT; unknown facts are omitted and model identity is bounded. Remove raw errorMessage passthrough from this branch only.
tests/nan-provider.test.ts Real public-SDK serialization with intercepted HTTP; snapshot/provider/live-catalog isolation. No live key or model call.
tests/inprocess-reviewer.test.ts Empty/length cases, malformed and missing counters, bounded identity, privacy and exactly one completion.
tests/review-host-relay.test.ts, tests/review-relay-transport-agent.test.ts Preserve the safe facts through typed relay failure mapping and the public failure report.

Verification

  • Observed test-first RED: 149 passed / 6 failed, exit 1 (missing compatibility/evidence and raw error disclosure).
  • GREEN: 156 passed / 0 failed, exit 0, using node --experimental-strip-types --test tests/nan-provider.test.ts tests/inprocess-reviewer.test.ts tests/review-host-relay.test.ts tests/review-relay-transport-agent.test.ts.
  • Independent read-only verifier: the same 156 tests plus nine offline adversarial cases passed; no blocking findings in the six-file diff.
  • pnpm typecheck: zero diagnostics, no regressions.
  • pnpm run check:runtime-modules: all 16 generated modules match.
  • node scripts/verify-package-files.mjs: 215 files / 69 pinned artifacts validated.
  • pnpm run test:packed-package: packed installation E2E passed (gentle-pi 4.3.0 / Gentle AI 4.2.0).
  • git diff --check: clean.
  • Remote PR CI completed successfully: CI and cross-platform hidden-process checks; all nine checks, including CodeRabbit, passed.

Full local suite — explicitly not green

With the pinned native binary installed and GENTLE_PI_REQUIRE_NATIVE_BINARY=1, the candidate's pnpm test exited 1: 6,959 passed / 11 failed / 72 skipped. An isolated unchanged main at abc4b2786, under the same command and native flag, exited 1: 6,954 passed / 10 failed / 72 skipped.

All ten baseline failures also occur on the candidate: five installer-bundle tests report missing zip; five skill-registry/watcher tests fail locally. The candidate additionally failed a matching FIFO cannot block presence discovery under the full-suite run. An independent focused comparison selected that test exactly once per checkout: candidate passed in 127 ms; baseline passed in 123 ms. Its test and implementation are byte-identical across both trees, and its 1,000 ms child-process deadline includes startup and module loading. This favors timing/load sensitivity, but does not erase the original failure or prove its exact cause. The provider-contract and runtime-harness stages pass on both trees. No check is skipped, softened or hidden by this PR.

The remote CI for the same base commit is green. Remote PR CI must pass before this change is merged.

Scope and rollback

This is one 168-line work unit across six files. Revert this commit to restore the prior request field and empty-refusal evidence; no settings, profiles, native authority or binary pin needs migration.

Refs #1259 is intentional: this only advances its EMPTY_OUTPUT evidence. Other post-completion refusals and requested-limit diagnostics remain outside this PR, so that issue must remain open. No retry behavior from #1307 / #1318 is included. No #2349 code or review state is changed.

Checklist

Summary by CodeRabbit

  • Bug Fixes
    • Empty-output reports now include sanitized completion details, such as the model, recognized stop reason, valid usage counts, and content measurements. Sensitive provider errors, private thinking data, and other unapproved fields are excluded.
    • DeepSeek V4 Flash requests now use the expected max_tokens field while retaining the existing output limit. Other models’ request settings remain unchanged.

Use the existing max_tokens compatibility field for DeepSeek without changing model selection, effort or the configured cap. Retain bounded, allowlisted completion facts on empty-output refusals without copying provider text.

Refs #2357, #1259
@decode2 decode2 added the type:bug Bug fix label Oct 17, 2026
@coderabbitai

coderabbitai Bot commented Oct 17, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: e382ace7-a1c5-4599-baed-5bdab540bd65
📥 Commits

Reviewing files that changed from the base of the PR and between abc4b27 and f069c4c.

📒 Files selected for processing: 6
  • lib/inprocess-reviewer.ts
  • lib/nan-provider.ts
  • tests/inprocess-reviewer.test.ts
  • tests/nan-provider.test.ts
  • tests/review-host-relay.test.ts
  • tests/review-relay-transport-agent.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.

📝 Walkthrough

The PR adds structured, sanitized evidence to empty reviewer-output refusals and configures DeepSeek V4 Flash requests to use max_tokens. Tests cover evidence validation and relay reporting, request serialization, and compatibility-state isolation.

Changes: Empty-output evidence

Layer / File(s) Summary
Build and attach sanitized evidence
lib/inprocess-reviewer.ts, tests/inprocess-reviewer.test.ts
Empty-output refusals now include a bounded model identifier, recognized stop reason, valid usage counters, and content-block statistics. Tests cover omitted invalid or unavailable values and excluded provider fields.
Verify evidence in relay reports
tests/review-host-relay.test.ts, tests/review-relay-transport-agent.test.ts
Relay tests check that reports include selected completion facts and omit error messages, thinking signatures, authorization data, and provider payloads.

DeepSeek token-field compatibility

Layer / File(s) Summary
Configure and verify the output-limit field
lib/nan-provider.ts, tests/nan-provider.test.ts
DeepSeek V4 Flash uses max_tokens, and model clones preserve compatibility settings. Tests check request serialization and isolation across snapshots, providers, and catalog refreshes.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: alan-thegentleman

Merge Risk: ⚪ Minimal · up to f069c

The targeted tests and reported package checks passed. No concrete issue currently blocks merging, though the full-suite failure and pending CI still warrant normal follow-up.

Pre-merge checks: Passed 3 · Failed 2

Failed Failed checks: 2 warnings

Check name Status Explanation Resolution
Out of Scope Changes check Warning The EMPTY_OUTPUT evidence builder in lib/inprocess-reviewer.ts and its relay tests add separate diagnostic behavior. The [#2357] issue explicitly assigns empty-output diagnostic evidence to [#1259… Remove the EMPTY_OUTPUT diagnostic and relay changes, with their dedicated tests, from this PR. Submit them under [#1259], or add [#1259] as an approved directly linked coding target before keeping that work in scope.
Docstring Coverage Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 6 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 3 passed
Check name Status Explanation
Linked Issues check Passed For [#2357], deepseek-v4-flash now sets compat.maxTokensField to max_tokens. The existing maxTokens value remains 16,384. The intercepted completeSimple test verifies the DeepSeek endpoint r…
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly identifies the primary change: fixing the NaN DeepSeek provider to honor its output token limit through the compatibility setting.
Full details: Linked Issues check

Explanation

For [#2357], deepseek-v4-flash now sets compat.maxTokensField to max_tokens. The existing maxTokens value remains 16,384. The intercepted completeSimple test verifies the DeepSeek endpoint receives max_tokens: 16384 and does not receive max_completion_tokens. The test also verifies unchanged reasoning, model identity, other-model compatibility, and snapshot isolation. No routing, retry, endpoint, or native-review behavior changes appear in the diff.

Full details: Out of Scope Changes check

Explanation

The EMPTY_OUTPUT evidence builder in lib/inprocess-reviewer.ts and its relay tests add separate diagnostic behavior. The [#2357] issue explicitly assigns empty-output diagnostic evidence to [#1259] and excludes it from this issue. These changes are not needed to prove the DeepSeek token-field compatibility fix. The PR description confirms that this separate work is included.

Resolution

Remove the EMPTY_OUTPUT diagnostic and relay changes, with their dedicated tests, from this PR. Submit them under [#1259], or add [#1259] as an approved directly linked coding target before keeping that work in scope.

  • Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@decode2
decode2 merged commit c531315 into main Oct 17, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type:bug Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(provider): NaN DeepSeek does not enforce the output-limit field selected by the SDK

1 participant