Skip to content

fix(core): validate run_in_background parameter type - #7597

Closed
yiliang114 wants to merge 3 commits into
QwenLM:mainfrom
yiliang114:cx/7571-background-fail-fast
Closed

fix(core): validate run_in_background parameter type#7597
yiliang114 wants to merge 3 commits into
QwenLM:mainfrom
yiliang114:cx/7571-background-fail-fast

Conversation

@yiliang114

@yiliang114 yiliang114 commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

Rejects non-boolean run_in_background values at the Agent tool's raw parameter boundary, before an invocation is created.

Why it's needed

The Agent tool uses custom parameter validation and does not invoke the base JSON Schema validator. OpenAI-compatible tool arguments are parsed from model-provided JSON and reach this validator without property-level type enforcement. A value such as "true" therefore bypasses the strict boolean guard added by #7593 and then participates in background routing through JavaScript truthiness: a top-level call runs in the background, while a nested call silently falls back to the foreground.

Rejecting malformed values keeps the execution contract consistent and prevents non-boolean inputs from bypassing the nested-background fix.

Reviewer Test Plan

How to verify

  1. Supply run_in_background: "true" as a raw Agent tool parameter and confirm validation returns a boolean-type error before invocation construction.
  2. Confirm valid true, false, and omitted values retain their existing behavior.
  3. Confirm nested explicit boolean true remains handled by the guard merged in fix(core): reject nested background requests #7593.

Evidence (Before & After)

Before: latest main accepts a raw string value and routes it according to JavaScript truthiness.

After: the malformed value is rejected during parameter validation. The complete core Agent test file passes 198/198 tests; build, typecheck, and ESLint also pass. Screenshots are N/A because this is a non-UI parameter-validation change.

Tested on

OS Status
🍏 macOS
🪟 Windows ⚠️
🐧 Linux ⚠️

Environment (optional)

Local Node.js 22 development environment.

Risk & Scope

  • Main risk or tradeoff: malformed non-boolean values that were previously interpreted through JavaScript truthiness now fail validation.
  • Not validated / out of scope: restoring base schema validation for all Agent parameters; that would also affect existing normalization and compatibility behavior.
  • Breaking changes / migration notes: none for schema-compliant callers.

Linked Issues

Follow-up to #7593

Related: #7571

中文说明

这个 PR 做了什么

在创建 invocation 之前,于 Agent 工具的原始参数边界拒绝非 boolean 类型的 run_in_background 值。

为什么需要

Agent 工具使用自定义参数校验,并不会调用基础 JSON Schema 校验器。OpenAI 兼容的工具参数从模型提供的 JSON 中解析后,在没有属性级类型约束的情况下到达该校验逻辑。因此,"true" 这样的值会绕过 #7593 添加的严格 boolean 保护,并通过 JavaScript truthiness 参与后台路由:顶层调用会在后台执行,嵌套调用则会静默回退到前台。

拒绝异常类型的值可以保持执行契约一致,并防止非 boolean 输入绕过嵌套后台修复。

Reviewer 测试计划

如何验证

  1. run_in_background: "true" 作为 Agent 工具原始参数传入,确认校验会在构造 invocation 之前返回 boolean 类型错误。
  2. 确认合法的 truefalse 以及省略该参数时,原有行为保持不变。
  3. 确认嵌套场景中显式传入 boolean true 时,仍由 fix(core): reject nested background requests #7593 合入的保护逻辑处理。

前后证据

修复前:最新 main 会接受原始字符串值,并根据 JavaScript truthiness 对其进行路由。

修复后:异常类型的值会在参数校验阶段被拒绝。完整 core Agent 测试文件 198/198 通过,build、typecheck 和 ESLint 也均通过。该变更属于非 UI 参数校验,因此截图不适用。

测试平台

OS 状态
🍏 macOS
🪟 Windows ⚠️
🐧 Linux ⚠️

环境

本地 Node.js 22 开发环境。

风险与范围

  • 主要风险或取舍:此前会通过 JavaScript truthiness 解释的异常非 boolean 值,现在会校验失败。
  • 未验证或不在范围内:恢复所有 Agent 参数的基础 schema 校验;这样做还会影响现有的参数归一化与兼容行为。
  • 破坏性变更或迁移说明:对于符合 schema 的调用方没有影响。

关联 Issue

#7593 的后续补强

相关:#7571

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓

Problem: this is an observed bug with real evidence, not theoretical hardening. #7571 documents a captured session where an explicit run_in_background: true from a non-top-level context silently ran foreground — blocking the parent turn for ~192 seconds and burning ~991K tokens before cancellation. The silent execution-mode change is the defect, independent of the ALS leak (#7156/#7194) that originally produced the nested context.

Direction: aligned. The issue asks for exactly this contract — an explicit background request should either be honored or fail fast, never silently downgrade. Honoring nested background is out of scope for v1 (a nested launcher can't honor the completion contract: send_message/task_stop are excluded from its toolset), so failing fast with an actionable message is the right minimal choice. CHANGELOG has no direct reference, but the area (background delegation / session management) is active and relevant.

Size: core path (packages/core/src/tools/agent/agent.ts), but tiny — roughly 15 production lines changed in agent.ts plus test updates (43+/15- across both files). Well under any threshold; no maintainer-awareness flag needed.

Approach: scope feels right. The guard reuses the existing buildSpawnBlockedResult helper and sits right next to the analogous nested-fork rejection, so the result shape stays consistent. The added non-boolean validateToolParams check is a small, directly-related hardening — a truthy non-boolean like 'true' would slip past the strict === true guard, so this protects the very guard being added rather than being scope creep. No drive-by changes spotted.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓

问题:这是一个已观测到的 bug,有真实证据,不是理论性加固。#7571 记录了一次捕获的会话:在非顶层上下文中显式传入 run_in_background: true 时被静默地以前台执行——阻塞父级 turn 约 192 秒,消耗约 99 万 token 才被取消。静默改变执行模式本身就是缺陷,与最初产生嵌套上下文的 ALS 泄漏(#7156/#7194)相互独立。

方向:对齐。issue 要求的正是这个契约——显式后台请求要么被正常执行,要么快速失败,绝不应静默降级。v1 不支持嵌套后台执行(嵌套启动方无法履行完成契约:其工具集不含 send_message/task_stop),因此用一条可执行的清晰信息快速失败是最合理的最小方案。CHANGELOG 没有直接引用,但该领域(后台委派 / 会话管理)活跃且相关。

规模:触及核心路径(packages/core/src/tools/agent/agent.ts),但非常小——agent.ts 约 15 行生产逻辑改动加测试更新(两个文件合计 43+/15-)。远低于任何阈值,无需维护者关注标记。

方案:范围合理。保护逻辑复用了现有的 buildSpawnBlockedResult 辅助方法,并紧挨着类似的嵌套 fork 拒绝逻辑放置,结果形状保持一致。新增的非 boolean validateToolParams 校验是一个小而直接相关的加固——像 'true' 这样的 truthy 非 boolean 值会绕过严格的 === true 判断,因此它保护的是正在添加的这个守卫本身,而非范围蔓延。未发现顺手改动。

进入代码审查 🔍

Qwen Code · qwen3.8-max-preview

Reviewed at cda792b29453ea364a69b99707e01696a86105f5 · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Code review

Before reading the diff, my independent take on #7571 was: detect an explicit run_in_background: true in a non-top-level session inside execute() and return a clear failed ToolResult via the existing buildSpawnBlockedResult helper (the same one the nested-fork and depth-limit guards use), then update the tool description and tests. The PR does exactly this — it matches my proposal, and I didn't find a simpler path it missed.

What I verified against the code:

  • The guard is correctly scoped. It uses strict params.run_in_background === true && !isTopLevelSession(), so only an explicit true from a nested context is rejected. Omitted flags and config-derived defaults still flow through to the existing shouldRunInBackground = backgroundRequested && isTopLevelSession() downgrade at the bottom of execute() — the implicit foreground behavior is untouched.
  • Result shape is consistent. Reusing buildSpawnBlockedResult means the rejection carries error.message (full guidance text, so the failure path tells the model how to recover) plus a task_execution / failed display with terminateReason. No child runtime is created and nothing is registered — confirmed by the test asserting createAgentHeadless and registry.register are not called.
  • The non-boolean validation closes a real gap. A truthy non-boolean (e.g. 'true') would not match the strict === true guard but would be treated as truthy by the ?? background-classification fallback — so without the validateToolParams check the guard could be bypassed. This hardens the guard being added; it's minimal and on-point, not scope creep.
  • Downstream consumers stay in sync. The two UI classifiers noted in the source (toolClassification.ts, tool-matching.ts) replicate the classification rule for actual launches, which is unchanged; the rejected path returns a failed task_execution before any runtime exists, same as the pre-existing nested-fork rejection.
  • Docs match behavior. The schema and agent-guidance text now state plainly that an explicit true from a nested agent is rejected, so the model stops retrying.

No correctness, security, or convention issues found. No duplication — the existing helper is reused.

Real-scenario testing

This change is a core scheduling contract with no UI surface, and the real-world trigger — a model choosing run_in_background: true from inside a genuinely nested subagent — is nondeterministic in a live TUI, so a tmux before/after can't reliably drive the changed path (the PR author makes the same call). The authoritative verification is the deterministic test, which exercises the real AgentToolInvocation.execute() production path with a simulated nested context. Full agent.test.ts run (198/198) plus the three behavior-defining tests:

 ✓ src/tools/agent/agent.test.ts (198 tests) 1273ms

 Test Files  1 passed (1)
      Tests  198 passed (198)

 ✓ AgentTool > Agent-level background: true > rejects an explicit background request from a nested sub-agent 3ms
 ✓ AgentTool > validateToolParams > rejects a non-boolean run_in_background value 6ms
 ✓ AgentTool > Agent-level background: true > keeps an omitted background flag in the foreground for nested sub-agents 4ms

 # surrounding behavior preserved:
 ✓ AgentTool > Agent-level background: true > runs a top-level subagent in the background when the flag is omitted 2ms
 ✓ AgentTool > Agent-level background: true > should run in background when run_in_background is true even without background config 8ms
 ✓ AgentTool > AgentToolInvocation > allows working_dir for a background:true subagent that downgrades to foreground when nested 26ms

The "before" evidence is the issue's captured session (silent foreground, ~192s / ~991K tokens) plus the old test that asserted the downgrade (toContain('Monitor done'), registered as isBackgrounded: false); the diff replaces it with the rejection assertion. ESLint and tsc --noEmit both pass clean across packages.

中文说明

代码审查

在读 diff 之前,我对 #7571 的独立方案是:在 execute() 中检测非顶层会话下显式的 run_in_background: true,并通过现有的 buildSpawnBlockedResult 辅助方法(嵌套 fork 和深度限制守卫使用的同一个)返回清晰的失败 ToolResult,然后更新工具描述和测试。PR 正是这么做的——与我的方案一致,我没有找到它遗漏的更简路径。

针对代码核实的内容:

  • 守卫范围正确。 使用严格的 params.run_in_background === true && !isTopLevelSession(),因此只有嵌套上下文下显式true 会被拒绝。省略的参数和由配置产生的默认值仍会走到底部现有的 shouldRunInBackground = backgroundRequested && isTopLevelSession() 降级路径——隐式前台行为不受影响。
  • 结果形状一致。 复用 buildSpawnBlockedResult 意味着拒绝结果带有 error.message(完整指引文本,使失败路径能告诉模型如何恢复)以及 task_execution / failed 展示和 terminateReason。不会创建子运行时,也不会注册任何东西——测试断言 createAgentHeadlessregistry.register 未被调用,已确认。
  • 非 boolean 校验堵住了真实漏洞。 一个 truthy 的非 boolean 值(如 'true')不会匹配严格的 === true 守卫,却会被 ?? 后台分类回退视为 truthy——因此没有 validateToolParams 校验时守卫可能被绕过。这是对正在添加的守卫的加固;最小且切题,不是范围蔓延。
  • 下游消费者保持同步。 源码中注明的两个 UI 分类器(toolClassification.tstool-matching.ts)复制的是实际启动的分类规则,该规则未变;被拒绝的路径在任何运行时存在之前就返回失败的 task_execution,与既有的嵌套 fork 拒绝一致。
  • 文档与行为一致。 schema 和 agent 指引文本现在明确说明嵌套 agent 显式传 true 会被拒绝,因此模型不会反复重试。

未发现正确性、安全性或规范问题。无重复代码——复用了现有辅助方法。

真实场景测试

此变更是一个没有 UI 表面的核心调度契约,而真实世界的触发条件——模型在真正的嵌套 subagent 内部选择 run_in_background: true——在实时 TUI 中是不确定的,因此 tmux before/after 无法可靠地驱动被改动的路径(PR 作者也做了同样判断)。权威的验证是确定性测试,它用模拟的嵌套上下文驱动真实的 AgentToolInvocation.execute() 生产路径。完整 agent.test.ts 运行(198/198)加上三个定义行为的测试(见上方代码块)。

"修复前"证据是 issue 捕获的会话(静默前台,约 192 秒 / 约 99 万 token)加上旧测试对降级的断言;diff 将其替换为拒绝断言。ESLint 和 tsc --noEmit 在各 package 均干净通过。

Qwen Code · qwen3.8-max-preview

Reviewed at cda792b29453ea364a69b99707e01696a86105f5 · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 5/5 — a textbook minimal fix: reuses the existing spawn-block helper, changes nothing beyond the intended contract, and is fully covered by deterministic tests that drive the real code path.

Stepping back: this solves a real, evidenced problem (the ~192s / ~991K-token silent-foreground incident in #7571), and it does the smallest thing that satisfies the issue's contract — an explicit background request from a nested context now fails fast with an actionable message instead of silently changing execution mode. The implementation matches my independent proposal; I found no simpler path it missed. Every line in the diff earns its place — the guard, the helper reuse, the description updates, and the non-boolean check that keeps the guard from being bypassed. There are no drive-by edits.

The behavior change is intentional and correctly bounded: only an explicit run_in_background: true from a nested session is newly rejected; top-level background launches, omitted flags, and config-derived defaults are all unchanged (the surrounding tests confirm this). If I had to maintain this in six months I'd thank the author — the guard sits next to the analogous nested-fork rejection, the result shape is shared, and the comment now accurately distinguishes explicit-reject from implicit-downgrade.

The one thing this PR doesn't have is a model-driven TUI before/after, but that's the right call: the trigger is nondeterministic and there's no UI surface, so the deterministic test against the real execute() path is the meaningful evidence — and it's green (198/198), with lint and typecheck clean. Approving. ✅

中文说明

置信度:5/5 —— 一个教科书式的最小修复:复用现有的 spawn-block 辅助方法,除目标契约外没有任何改动,并由驱动真实代码路径的确定性测试完整覆盖。

退一步看:这解决了一个真实、有证据的问题(#7571 中约 192 秒 / 约 99 万 token 的静默前台事件),并且用满足 issue 契约的最小方式实现——嵌套上下文中的显式后台请求现在会带着可执行的信息快速失败,而不是静默改变执行模式。实现与我的独立方案一致;我没有找到它遗漏的更简路径。diff 中每一行都物有所值——守卫、辅助方法复用、描述更新,以及防止守卫被绕过的非 boolean 校验。没有顺手改动。

行为变更是有意的且边界正确:只有嵌套会话中显式的 run_in_background: true 被新近拒绝;顶层后台启动、省略的参数、由配置产生的默认值都不变(周边测试证实了这一点)。如果六个月后由我维护,我会感谢作者——守卫紧挨着类似的嵌套 fork 拒绝放置,结果形状共享,注释现在也准确地区分了"显式拒绝"与"隐式降级"。

这个 PR 唯一没有的是模型驱动的 TUI before/after,但这是正确的选择:触发条件不确定且没有 UI 表面,因此针对真实 execute() 路径的确定性测试才是有意义的证据——它是绿的(198/198),lint 和 typecheck 也干净。批准。✅

Qwen Code · qwen3.8-max-preview

Reviewed at cda792b29453ea364a69b99707e01696a86105f5 · re-run with @qwen-code /triage

@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. ✅

@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.

Reviewed. Suggestions are inline.

— qwen3.7-max via Qwen Code /review

Comment thread packages/core/src/tools/agent/agent.ts
yiliang114 and others added 2 commits July 23, 2026 19:35
…il-fast

# Conflicts:
#	packages/core/src/tools/agent/agent.test.ts
#	packages/core/src/tools/agent/agent.ts
@yiliang114

Copy link
Copy Markdown
Collaborator Author

Merged the latest main to clear the conflict and kept the nested background rejection debug trace in place. Verified with packages/core Agent test and packages/web-shell index HTML test. No review suggestions were declined.

@gwinthis gwinthis 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.

Review: APPROVE (C=0)

Minimal defensive validation (+17/-0): rejects run_in_background: 'true' (string) with a clear error message. Models sometimes pass string "true" instead of boolean true — this catches it at the validation boundary before the value reaches the agent launcher.

Verification: Test covers the exact case ('true' string → /run_in_background.*boolean/i error). The @ts-expect-error annotation correctly documents that raw model parameters are untrusted.

Pattern: Validate model-supplied parameters at the tool boundary. The model's JSON schema says boolean, but the actual payload is untrusted — typeof checks are the right defense.

中文说明

评审:APPROVE (C=0)

最小化防御性校验(+17/-0):拒绝 run_in_background: 'true'(字符串)。模型有时传字符串 "true" 而非布尔 true——在工具边界处捕获。

模式: 在工具边界校验模型提供的参数。JSON schema 说 boolean,但实际 payload 不可信——typeof 检查是正确的防御。

— qwen3.7-max via Qwen Code /review

@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.

⚠️ Downgraded from Approve to Comment: CI still running. Reviewed.

— qwen3.7-max via Qwen Code /review

@yiliang114 yiliang114 changed the title fix(core): reject nested background agent requests fix(core): validate run_in_background parameter type Jul 23, 2026
@yiliang114

Copy link
Copy Markdown
Collaborator Author

Closing this follow-up because #7593 already fixed the failure mode reported in #7571. The original session’s Agent calls used boolean run_in_background values; this PR only handles a separate malformed string input that was not involved in the incident. If AgentTool’s broader schema-validation inconsistency is observed in practice, it should be addressed as a separate, evidence-driven change rather than as a one-field follow-up here.

@yiliang114 yiliang114 closed this Jul 23, 2026
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.

3 participants