fix(core): parse agent & workflow integer env vars strictly - #5679
Conversation
|
@qwen-code /triage |
|
Thanks for the PR @he-yufeng! Template looks good ✓ On direction: this is a clear, low-risk correctness fix. On approach: scope is tight — four call sites, one helper swap, new tests for the previously-accepted edge cases. Prettier reformatting on the touched test file is noise but unavoidable. No scope creep, no drive-by refactors. Moving on to code review + test run. 🔍 中文说明感谢贡献 @he-yufeng! 模板完整 ✓ 方向:这是低风险的正确性修复。 方案:范围紧凑 —— 四个调用点、一次 helper 替换、针对之前被接受的边界情况补充测试。测试文件的 prettier 重排是附带噪音但不可避免。没有范围蔓延,也没有顺手重构。 进入代码审查和测试阶段 🔍 — Qwen Code · qwen3.7-max |
Code reviewIndependent proposal before reading the diff: the four outlier resolvers ( The PR's solution matches this proposal exactly. No correctness bugs, no regressions, no missed edge cases. The helper's contract ( TestingPR's own test suite — Before/after behavioral reproduction (ran resolvers directly against Before (main —
|
|
Reflection: this is the kind of fix that quietly makes the codebase better. The PR's approach matches my independent proposal exactly — reuse the existing The fix is genuinely minimal — four 223 PR tests pass, 11/11 targeted behavioral assertions confirm the fix across all four resolvers. Non-user-visible, so the behavioral delta is the evidence — and it's clean. If I had to maintain this in six months, I'd thank the author: one helper, four identical-looking call sites, one mental model to keep in sync. Approving. ✅ 中文说明反思:这是那种 quietly 让代码变好的修复。PR 的方案与我的独立提案完全一致 —— 复用已有的 修复是真的最小化 —— 四次 PR 223 个测试通过,11/11 个目标行为断言在四个 resolver 上确认了修复。这是不可见的改动,所以行为差异本身就是证据 —— 而且很干净。 如果六个月后我要维护这段代码,我会感谢作者:一个 helper、四个看起来完全一致的调用点、一个要维护的心智模型。批准。✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
✅ Maintainer local verification — real
|
| Resolver | env var (value) | BASE — Number() + isInteger |
This PR — parsePositiveIntegerEnv |
|---|---|---|---|
resolveMaxAgentsPerRun |
MAX_WORKFLOW_AGENTS=0x2 |
honored = 2 → 3rd agent() rejected → workflow failed: exceeded the maximum of 2 agent() calls per run |
rejected → default 1000 → 3/3 agents, completed |
resolveMaxTokensPerWorkflow |
MAX_TOKENS_PER_WORKFLOW=0x2BF20 |
honored = 180000 → /workflows detail cap: 180k |
rejected → null → cap: (no cap) |
resolveMaxConcurrentBackgroundAgents |
MAX_BACKGROUND_AGENTS=0x2 |
honored = 2 → 3rd /fork rejected: maximum concurrent background agents (2) reached |
rejected → default 10 → all 3 forks accepted |
resolveConcurrencyLimit |
MAX_WORKFLOW_CONCURRENCY=0x10 |
honored = 16 (silent) | rejected → cpu-derived default + debug warning |
The single /workflows listing shows both workflow runs side by side:
wf_805c… failed captest · 2/2 agents · 52/180kt — Workflow exceeded the maximum of 2 agent() calls per run ← BASE (0x2 honored)
wf_f150… completed captest · 3/3 agents · 91t ← this PR (0x2 rejected)
Debug-log A/B (QWEN_DEBUG_LOG_FILE=1): this PR's session logs 3 × Invalid QWEN_CODE_MAX_… warnings; the BASE session logs 0 — i.e. it silently accepted the hex/scientific values, which is the bug.
No-regression probe: FIXED binary with a plain decimal MAX_WORKFLOW_AGENTS=2 still honors it → cap 2 → exceeded the maximum of 2. So the fix rejects only the non-decimal forms; valid decimal integers pass through unchanged.
Notes (non-blocking)
resolveConcurrencyLimitis the only one without a behavioural divergence here (with sequential agents both caps are ≥ 3), so it's confirmed via the debug warning rather than a visible effect — the other three are confirmed behaviourally at a user-visible surface.- Minor UX (pre-existing, not introduced by this PR): when a value like
0x2BF20is rejected, the TUI shows the generic "Workflows have no per-run token cap…" notice; the "this value was invalid" detail only lands in the debug log. Correct and safe, just a little opaque for someone who fat-fingered hex. Possible future polish, not a merge blocker.
Behaviour matches the diff and the PR description; aligning these four sites with the rest of the codebase is the right call. 👍
🇨🇳 中文版
✅ 维护者本地验证 —— 真实 qwen 二进制 A/B
结论:通过 —— 可以合并。 我在本地构建了两个分支并驱动真实 CLI(不是单元测试、也不是 import 直接调函数)。修复在全部四个 resolver 上都改变了可观测的运行时行为,与 PR 描述完全一致;同时合法的纯十进制值仍然生效(无回归)。
方法
同一个 CLI 二进制,只把 core 的 dist/ 在本 PR(19de577)与其 merge-base(01d28a1)之间通过重新构建来回切换 —— 每一侧运行前都先在构建产物 dist 里 grep 确认。真实 TUI,qwen3.7-max,--approval-mode yolo。每个环境变量都设成「Number() 接受、但严格十进制解析必须拒绝」的值:0x2→2,0x2BF20→180000,0x10→16。
- workflow 的两个上限:用保存的
/captest工作流(三次顺序agent()调用)触发。 - 后台 agent 上限:用三个并发
/fork(用sleep让它们保持运行)触发。
A/B 结果
| Resolver | 环境变量(值) | BASE —— Number() + isInteger |
本 PR —— parsePositiveIntegerEnv |
|---|---|---|---|
resolveMaxAgentsPerRun |
MAX_WORKFLOW_AGENTS=0x2 |
被当作 2 → 第 3 个 agent() 被拒 → 工作流失败:exceeded the maximum of 2 agent() calls per run |
被拒绝 → 默认 1000 → 3/3 agents,完成 |
resolveMaxTokensPerWorkflow |
MAX_TOKENS_PER_WORKFLOW=0x2BF20 |
被当作 180000 → /workflows 详情 cap: 180k |
被拒绝 → null → cap: (no cap) |
resolveMaxConcurrentBackgroundAgents |
MAX_BACKGROUND_AGENTS=0x2 |
被当作 2 → 第 3 个 /fork 被拒:maximum concurrent background agents (2) reached |
被拒绝 → 默认 10 → 三个 fork 全部接受 |
resolveConcurrencyLimit |
MAX_WORKFLOW_CONCURRENCY=0x10 |
被当作 16(静默) | 被拒绝 → cpu 派生默认值 + debug 警告 |
同一个 /workflows 列表同时显示两次运行:
wf_805c… failed captest · 2/2 agents · 52/180kt — Workflow exceeded the maximum of 2 agent() calls per run ← BASE(0x2 被采纳)
wf_f150… completed captest · 3/3 agents · 91t ← 本 PR(0x2 被拒绝)
Debug 日志 A/B(QWEN_DEBUG_LOG_FILE=1):本 PR 的会话打印了 3 条 Invalid QWEN_CODE_MAX_… 警告;BASE 会话打印 0 条 —— 也就是说它静默接受了 hex/科学计数值,这正是 bug 本身。
无回归探针: FIXED 二进制配上纯十进制 MAX_WORKFLOW_AGENTS=2 仍然生效 → 上限 2 → exceeded the maximum of 2。所以这个修复只拒绝非十进制形式;合法的十进制整数原样通过。
备注(不阻塞合并)
resolveConcurrencyLimit是唯一在本场景下没有行为差异的(顺序 agent 时两边的上限都 ≥ 3),所以它靠 debug 警告确认,而非可见效果 —— 另外三个都在用户可见的界面上得到了行为级确认。- 轻微 UX(既有行为,非本 PR 引入):当
0x2BF20这类值被拒绝时,TUI 显示的是泛化的「Workflows have no per-run token cap…」提示;「这个值无效」的细节只落在 debug 日志里。行为是正确且安全的,只是对手滑打错 hex 的用户略不直观。可作为后续打磨,不是合并阻塞项。
行为与 diff、PR 描述一致;把这四处与代码库其余部分对齐是正确的做法。👍
Local real-binary verification on macOS (worktree build + tmux A/B). Workflow/background features enabled via QWEN_CODE_ENABLE_WORKFLOWS=1.
What this PR does
Routes the four agent/workflow integer env overrides —
QWEN_CODE_MAX_BACKGROUND_AGENTS,QWEN_CODE_MAX_TOKENS_PER_WORKFLOW,QWEN_CODE_MAX_WORKFLOW_AGENTS, andQWEN_CODE_MAX_WORKFLOW_CONCURRENCY— through the existingparsePositiveIntegerEnvhelper instead of a bareNumber(raw)+Number.isIntegercheck. Each function keeps its existing fallback, debug warning, and clamping behavior; only the validity check is tightened.Why it's needed
Number(raw)accepts hex, scientific, and trailing-zero-float literals, andNumber.isIntegeronly checks the result. SoQWEN_CODE_MAX_WORKFLOW_AGENTS=0x10was silently honored as 16,=1e3as 1000, and=1.0as 1 — none of which a user typing a count would expect, and a fat-fingered1e9could push the cap all the way to the hard ceiling. The rest of the codebase already parses positive-integer env vars withparsePositiveIntegerEnv(/^\d+$/+Number.isSafeInteger) — e.g.coreToolScheduler,modelConfigResolver,serve.ts. These four agent/workflow caps were the remaining sites still on the loose pattern; this aligns them with the rest, and as a bonus rejects unsafe-integer strings thatNumber.isIntegerlet through.Reviewer Test Plan
How to verify
Each resolver should accept only plain decimal integers and fall back otherwise (to its default, to
nullfor the token cap, or to the cpu-derived value for the concurrency limit). New unit tests cover the previously-accepted0x10/1e2/1e3/1e6/1.0/5.0inputs; the existing decimal, zero/negative, non-numeric, and over-ceiling-clamp cases are unchanged and still pass.Evidence (Before & After)
N/A — non-user-visible env-parsing fix.
Tested on