Skip to content

fix(core): parse max output token env strictly - #5491

Merged
wenshao merged 1 commit into
QwenLM:mainfrom
tt-a1i:fix/max-output-token-env-strict
Jun 20, 2026
Merged

fix(core): parse max output token env strictly#5491
wenshao merged 1 commit into
QwenLM:mainfrom
tt-a1i:fix/max-output-token-env-strict

Conversation

@tt-a1i

@tt-a1i tt-a1i commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

What this PR does

Parses QWEN_CODE_MAX_OUTPUT_TOKENS as a strict positive integer for both OpenAI-compatible and Anthropic content generators, so malformed values no longer get accepted through parseInt partial parsing.

Why it's needed

Before this change, values like 1.5 and 2k were treated as 1 and 2. That can silently override the intended capped default with a much smaller token budget. The env override should only apply when the value is a whole positive integer; malformed values should fall back to the existing default behavior.

Reviewer Test Plan

How to verify

Set QWEN_CODE_MAX_OUTPUT_TOKENS to malformed values such as 1.5, 2k, or abc and confirm OpenAI-compatible and Anthropic requests keep the capped default 8000. Set it to a valid value such as 9000 and confirm both paths honor the override.

Evidence (Before & After)

N/A. This is a non-UI parsing fix covered by unit tests.

Tested on

OS Status
🍏 macOS ✅ tested
🪟 Windows ⚠️ not tested
🐧 Linux ⚠️ not tested

Environment (optional)

Local Node/npm workspace on macOS.

Risk & Scope

  • Main risk or tradeoff: stricter parsing rejects partially numeric env values that were previously accepted accidentally.
  • Not validated / out of scope: full cross-platform CI; no Windows or Linux local run.
  • Breaking changes / migration notes: users with malformed QWEN_CODE_MAX_OUTPUT_TOKENS values will now get the capped default instead of a partial parse.

Linked Issues

Fixes #5490

中文说明

What this PR does

这个 PR 让 OpenAI-compatible 和 Anthropic 两条生成路径都严格按正整数解析 QWEN_CODE_MAX_OUTPUT_TOKENS,不再让 parseInt 接受半截数字。

Why it's needed

改动前,1.5 会被当成 12k 会被当成 2。这会悄悄覆盖原本的 8000 capped default,导致输出 token 预算异常变小。这个环境变量只有在值是完整正整数时才应该生效;格式不对时应该回退到现有默认行为。

Reviewer Test Plan

How to verify

QWEN_CODE_MAX_OUTPUT_TOKENS 设成 1.52kabc 这类非法值,确认 OpenAI-compatible 和 Anthropic 请求仍使用 capped default 8000。再设成 9000 这类合法值,确认两条路径都正常使用覆盖值。

Evidence (Before & After)

N/A。这是非 UI 解析修复,已用单测覆盖。

Tested on

OS Status
🍏 macOS ✅ tested
🪟 Windows ⚠️ not tested
🐧 Linux ⚠️ not tested

Environment (optional)

macOS 本地 Node/npm workspace。

Risk & Scope

  • Main risk or tradeoff: 更严格的解析会拒绝以前因为 parseInt 而被意外接受的半截数字。
  • Not validated / out of scope: 没有在本地跑完整跨平台 CI,也没有本地 Windows/Linux 验证。
  • Breaking changes / migration notes: 如果用户配置了格式错误的 QWEN_CODE_MAX_OUTPUT_TOKENS,现在会回退到 capped default,不再使用半截解析出来的数字。

Linked Issues

Fixes #5490

AI Assistance Disclosure

I used Codex to review the changes, sanity-check the implementation against existing patterns, and help spot potential edge cases.

@wenshao

wenshao commented Jun 20, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓ — all required sections present, bilingual, linked issue.

On direction: this is a clean bug fix — parseInt('1.5', 10) silently returning 1 and overriding the 8000 capped default with a 1-token budget is a real footgun. Claude Code had the same CLAUDE_CODE_MAX_OUTPUT_TOKENS issue and fixed it similarly. Solid alignment.

On approach: scope is minimal and correct. One shared parsePositiveIntegerEnvValue() helper in tokenLimits.ts, applied in both generators where the bug lived. No drive-by changes, no unnecessary abstractions. The regex + Number.isSafeInteger() check is slightly more robust than a plain Number() swap would be (catches 9e99 type edge cases). Clean.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓ — 所有必填章节齐全,双语,关联了 issue。

方向:这是一个干净的 bug 修复 — parseInt('1.5', 10) 静默返回 1,会把 8000 的 capped default 覆盖成 1 token 预算,是个真实的坑。Claude Code 也遇到过同样的 CLAUDE_CODE_MAX_OUTPUT_TOKENS 问题并做了类似修复,方向一致。

方案:范围最小且正确。在 tokenLimits.ts 里抽了一个共享的 parsePositiveIntegerEnvValue() 函数,在两处有 bug 的生成器里替换。没有夹带改动,没有多余抽象。regex + Number.isSafeInteger() 比直接换 Number() 更严谨(能挡住 9e99 这类边界情况)。干净。

进入代码审查 🔍

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Code Review

Independent proposal: I'd add a strict parser for the env value — reject anything that isn't a clean positive integer, fall back to the capped default otherwise. Extract it to tokenLimits.ts since both generators need it.

The PR does exactly this and slightly exceeds my proposal by also adding Number.isSafeInteger() for extra safety against astronomically large values.

No blockers. The parsePositiveIntegerEnvValue() function is correct: /^\d+$/ regex rejects decimals, suffixes, negatives; Number.isSafeInteger() catches overflow; parsed <= 0 filters zero. Both generators updated consistently. Tests properly save/restore env in beforeEach/afterEach to avoid cross-test pollution.

Typecheck and lint both clean on the changed files.

Testing

Before/After parsing behavior

Reproduced the bug and verified the fix by running the old parseInt logic vs the new parsePositiveIntegerEnvValue logic side by side:

$ node tmp/parse-compare.mjs
Value      | Before (old)   | After (new)
-----------|----------------|------------
'1.5'      | 1              | fallback(8000)
'2k'       | 2              | fallback(8000)
'abc'      | fallback(8000) | fallback(8000)
'9000'     | 9000           | 9000
'0'        | fallback(8000) | fallback(8000)
'-3'       | fallback(8000) | fallback(8000)
''         | fallback(8000) | fallback(8000)
'  42  '   | 42             | 42

1.5 and 2k — the exact cases from the PR description — now correctly fall back to 8000 instead of being silently parsed as 1 and 2. Valid values like 9000 and whitespace-trimmed 42 still work.

Unit tests

$ npx vitest run src/core/anthropicContentGenerator/anthropicContentGenerator.test.ts src/core/openaiContentGenerator/provider/default.test.ts

 RUN  v3.2.4 packages/core
      Coverage enabled with v8

 ✓ src/core/openaiContentGenerator/provider/default.test.ts (26 tests) 28ms
 ✓ src/core/anthropicContentGenerator/anthropicContentGenerator.test.ts (69 tests) 1181ms

 Test Files  2 passed (2)
      Tests  95 passed (95)

All 95 tests pass, including 4 new tests covering malformed env values (1.5, 2k, abc) and valid overrides (9000) for both generators.

中文说明

代码审查

独立方案:我会给 env 值加严格解析 — 不是干净的正整数就拒绝,回退到 capped default。抽到 tokenLimits.ts 因为两个生成器都要用。

PR 正好这么做的,而且比我多了一层 Number.isSafeInteger() 防止超大值。

没有阻塞项。parsePositiveIntegerEnvValue() 正确:/^\d+$/ 拒绝小数、后缀、负数;Number.isSafeInteger() 防溢出;parsed <= 0 过滤零。两个生成器一致更新。测试在 beforeEach/afterEach 正确保存/恢复 env,避免跨测试污染。

Typecheck 和 lint 均通过。

测试

并排对比了旧 parseInt 和新 parsePositiveIntegerEnvValue 的解析行为。1.52k(PR 描述里的两个场景)现在正确回退到 8000,不再被静默解析为 1 和 2。合法值如 9000 和带空格的 42 仍然正常工作。

95 个单测全部通过,包括 4 个新测试覆盖非法值(1.52kabc)和合法覆盖值(9000)。

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

This is a textbook bug fix: real problem, minimal scope, correct implementation, good tests.

The before/after table tells the whole story — parseInt('1.5') silently producing 1 is exactly the kind of thing that makes someone's session generate a single token and wonder why. The fix is the simplest thing that works: one shared function, two call-site updates, four new tests. No refactoring tangents, no "while I'm here" changes.

My independent proposal was to swap parseInt for Number() — the PR's regex + Number.isSafeInteger() is slightly better because it also rejects things like 9e99 that Number() would accept. Good judgment call.

95 tests pass, typecheck clean, lint clean. Ships it.

中文说明

这是一个教科书级的 bug 修复:真实问题、最小范围、正确实现、测试到位。

Before/After 表格说明了一切 — parseInt('1.5') 静默产出 1,就是那种让用户的 session 只生成一个 token 然后一脸懵的坑。修复方式是最简单的有效方案:一个共享函数、两处调用点更新、四个新测试。没有顺手重构,没有"顺便改改"。

我的独立方案是把 parseInt 换成 Number() — PR 的 regex + Number.isSafeInteger() 更好一点,因为还能挡住 9e99 这种 Number() 会接受的值。判断到位。

95 个测试通过,typecheck 通过,lint 通过。可以合。

Qwen Code · qwen3.7-max

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM, looks ready to ship. ✅

@wenshao

wenshao commented Jun 20, 2026

Copy link
Copy Markdown
Collaborator

✅ Verification report — local real-world testing (LGTM, one non-blocking follow-up)

I verified this PR locally on Linux (Node 22.22.2) with unit tests, a before/after regression check, an edge-case battery against the shipped bundle, and a live tmux end-to-end run of the real CLI against a mock OpenAI server that captures the actual wire max_tokens. The core fix is correct, well-tested, and confirmed end-to-end. I also found one pre-existing, non-blocking consistency gap worth a follow-up (details below).

What the bug was

Both content generators parsed QWEN_CODE_MAX_OUTPUT_TOKENS with parseInt(envVal, 10), which partial-parses: 1.5 → 1, 2k → 2. A malformed value silently became a tiny token budget. The PR adds a strict parsePositiveIntegerEnvValue (/^\d+$/ + Number.isSafeInteger + > 0) and routes both generators through it, so malformed values fall back to the capped default (8000).

Verification performed

1. Affected suites — 154/154 pass (tokenLimits 59, openai provider/default 26, anthropic 69), including the 4 new env-parsing tests.

2. Before/after (the new tests are load-bearing). I reverted only the helper body to the old parseInt behavior and re-ran:

  • malformed tests FAIL — OpenAI got max_tokens: 1 and Anthropic got max_tokens: 1 (from 1.5) instead of 8000.
  • "respects a valid value" tests still pass (9000 works under both).

So the new tests genuinely catch the bug rather than passing incidentally.

3. Edge-case battery against the shipped, exported helper — 20/20. Each input compared to what old parseInt produced; 8 inputs diverge, all corrected:

input new old parseInt
1.5 reject→8000 1 ⟵ fixed
2k reject→8000 2 ⟵ fixed
1e5 reject→8000 1 ⟵ fixed
100abc reject→8000 100 ⟵ fixed
+9000 reject→8000 9000 ⟵ fixed
9999999999999999999999 reject→8000 9.99e21 (unsafe) ⟵ fixed
9007199254740992 (MAX_SAFE+1) reject accepted (unsafe) ⟵ fixed
9007199254740991 (MAX_SAFE) 9007199254740991 same honored exactly
9000 / 8000 / 1 / 9000 / 08000 honored same valid
abc / 0 / -5 / `` / 0x10 reject reject already rejected

4. Live tmux E2E — real bundled CLI → mock OpenAI server, observing the actual outgoing max_tokens. Alternating malformed/valid proves the wire value tracks each env value (not a monotonic artifact):

ENV=1.5   (malformed) → max_tokens=8000   ✓
ENV=9000  (valid)     → max_tokens=9000   ✓
ENV=2k    (malformed) → max_tokens=8000   ✓
ENV=16000 (valid)     → max_tokens=16000  ✓
ENV unset (default)   → max_tokens=8000   ✓

(gpt-4, samplingParams unset so the env path is exercised. Each scenario captured 2 identical requests.)

5. Static + CI. eslint clean · prettier --check clean · git diff --check clean · tsc (core) 0 errors · GitHub CI all green (Lint, Test ubuntu/macos/windows, CodeQL).


⚠️ Non-blocking follow-up — escalation gate is not consistent with the strict parse

This PR touches the two content generators but not geminiChat.ts:1983, which still decides whether the user "has a max-tokens override" with a raw presence check:

const hasUserMaxTokensOverride =
  (cgConfig?.samplingParams?.max_tokens != null) ||
  !!process.env['QWEN_CODE_MAX_OUTPUT_TOKENS'];   // ← true for ANY non-empty string, incl. "1.5"

hasUserMaxTokensOverride gates the 8K→64K MAX_TOKENS escalation retry (geminiChat.ts:2384-2388, !hasUserMaxTokensOverride). So after this PR, a malformed value diverges:

env generators max_tokens escalation 8K→64K
unset 8000 enabled baseline
1.5 (malformed) 8000 (now correct) suppressed ⚠ differs from unset
9000 (valid) 9000 suppressed (intended) ok

Net: a malformed value still silently disables the truncation-recovery retry, so it does not fully "fall back to existing default behavior" as the PR intends. This is pre-existing and strictly better than before (which was max_tokens=1 and no escalation), so not a blocker — but for completeness consider using the same helper there:

parsePositiveIntegerEnvValue(process.env['QWEN_CODE_MAX_OUTPUT_TOKENS']) !== undefined

Minor notes

  • No direct unit test for parsePositiveIntegerEnvValue. It's covered transitively by 4 generator cases (1.5/2k/abc/9000); the interesting edges (unsafe integers, 1e5, leading +) aren't. A few direct unit tests in tokenLimits.test.ts would lock in the isSafeInteger/regex behavior. (I verified those edges myself — 20/20.)
  • Behavior change for the changelog: +9000 and out-of-safe-range values that old parseInt accepted are now rejected (→ capped default). Reasonable, but worth a one-line migration note beyond the 1.5/2k examples already given.

Verdict

LGTM — approve / merge. Correct, minimal, well-targeted fix; regression tests proven to catch the bug; verified end-to-end on the real bundle and live CLI; all checks green. Recommend the geminiChat.ts consistency tweak as a fast follow-up (this PR or another).

🇨🇳 中文版(点击展开)

✅ 验证报告 —— 本地真实测试(建议合并,附一个非阻断后续项)

作为维护者,我在 Linux(Node 22.22.2) 上完成了验证:单测、前后对照回归、针对构建产物的边界用例电池,以及 用真实 CLI 在 tmux 里打到 mock OpenAI 服务器、抓取真实出网 max_tokens 的端到端测试。核心修复 正确、测试到位、端到端确认有效。同时发现一处 既有的、非阻断的一致性缺口,建议作为后续项处理(见下)。

漏洞本身

两条生成路径此前用 parseInt(envVal, 10) 解析 QWEN_CODE_MAX_OUTPUT_TOKENS,会半截解析:1.5→12k→2,导致非法值悄悄变成极小的 token 预算。本 PR 新增严格的 parsePositiveIntegerEnvValue/^\d+$/ + Number.isSafeInteger + >0),两条路径都改走它,非法值回退到 capped default(8000)。

已完成的验证

  1. 受影响测试套件 154/154 通过tokenLimits 59、openai provider/default 26、anthropic 69),含 4 个新增用例。
  2. 前后对照(证明新测试有效):仅把 helper 改回旧 parseInt 行为重跑 → 非法值用例 失败(OpenAI/Anthropic 都得到 max_tokens: 1,来自 1.5),合法值(9000)仍通过。说明新测试确实能捕获该 bug。
  3. 针对构建产物中导出的 helper 跑边界电池 20/20:每个输入与旧 parseInt 对比,8 个输入产生分歧且全是改进1.5/2k/1e5/100abc/+9000→拒绝;超出安全整数范围的大数→拒绝;MAX_SAFE_INTEGER 边界值精确保留)。
  4. tmux 实机 E2E(真实打包 CLI → mock OpenAI,抓真实出网 max_tokens:交替合法/非法值,证明出网值精确跟随每个 env:
ENV=1.5   (非法) → max_tokens=8000   ✓
ENV=9000  (合法) → max_tokens=9000   ✓
ENV=2k    (非法) → max_tokens=8000   ✓
ENV=16000 (合法) → max_tokens=16000  ✓
ENV 未设  (默认) → max_tokens=8000   ✓
  1. 静态检查 + CIeslintprettier --checkgit diff --check 均干净;tsc(core)0 错误;GitHub CI 全绿。

⚠️ 非阻断后续项 —— 升级(escalation)判定与严格解析不一致

本 PR 改了两条生成路径,但 没有geminiChat.ts:1983,那里仍用裸的存在性判断:

!!process.env['QWEN_CODE_MAX_OUTPUT_TOKENS']   // 任意非空字符串(含 "1.5")都为 true

hasUserMaxTokensOverride关闭 8K→64K 的 MAX_TOKENS 升级重试(geminiChat.ts:2384-2388)。因此本 PR 后,非法值 出现分歧:

env 生成路径 max_tokens 8K→64K 升级
未设 8000 启用
1.5(非法) 8000(现已正确) 被抑制 ⚠ 与“未设”不一致
9000(合法) 9000 被抑制(符合预期)

即:非法值仍会悄悄关掉截断恢复重试,未能完全“回退到既有默认行为”。该问题 属既有问题,且明显优于改动前(之前是 max_tokens=1 且无升级),因此 不阻断合并;但为完整起见,建议那里也复用同一 helper:parsePositiveIntegerEnvValue(...) !== undefined

次要建议

  • parsePositiveIntegerEnvValue 无直接单测:仅通过 4 个生成器用例间接覆盖;有意思的边界(不安全大整数、1e5、前导 +)没覆盖到。建议在 tokenLimits.test.ts 加几条直接单测锁定 isSafeInteger/正则行为(这些我已自测 20/20)。
  • changelog 行为变更:旧 parseInt 接受的 +9000、超安全范围的大数,现在会被拒绝(回退默认)。合理,但建议在迁移说明里除 1.5/2k 外补一句。

结论

建议合并(approve / merge):修复正确、最小、定位精准;回归测试已证明能捕获漏洞;已在真实构建产物与实机 CLI 上端到端验证;各项检查全绿。建议把 geminiChat.ts 的一致性微调作为快速后续项(本 PR 或另开)。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(core): QWEN_CODE_MAX_OUTPUT_TOKENS accepts partial numeric values

3 participants