fix(core): align monitor limit parameter schemas#6413
Conversation
Co-authored-by: chatgpt-codex-connector[bot] <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com>
|
Thanks for the PR! Template looks good ✓ — all required sections present and filled in. Problem: This is a real, observable inconsistency — the public tool schema declares Direction: Aligned. Schema accuracy is a core concern for tool callers, and aligning the declared type with the enforced type is straightforward and correct. No CHANGELOG reference needed — this is a small consistency fix. Size: 2 production lines changed (number → integer, twice), 11 test lines added. Not applicable for size thresholds. Approach: Scope is exactly right — two type changes in the schema plus a focused test assertion to prevent regression. No drive-by changes, no scope creep. This is what a minimal fix looks like. Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ — 所有必填部分齐全。 问题:这是一个真实存在的、可观测的不一致——公开 tool schema 把 方向:对齐。schema 准确性对工具调用方很重要,把声明类型和执行类型对齐是正确且直接的修复。无需 CHANGELOG 引用——这是一个小一致性修复。 规模:2 行生产代码变更(number → integer,两处),11 行测试代码。不涉及规模阈值。 方案:范围完全正确——两处类型变更加一个聚焦的测试断言防止回归。没有顺手改动,没有范围蔓延。这就是最小修复该有的样子。 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code Review (2a)Independent proposal: To fix schema/validator mismatch for monitor tool parameters, I would change Comparison with PR: The PR's approach matches exactly — two type changes in the schema plus a focused Correctness: Clean. The schema Downstream impact: None. The only consumers of Reuse check: N/A — this is a metadata fix, not new logic. No blockers found. Test Results (2b)Before (baseline — main branch, no PR)After (PR applied)73 → 74 tests. The new 中文说明代码审查 (2a)独立方案: 要修复 monitor 工具 schema/校验器的不一致,我会把 与 PR 对比: PR 方案完全一致——两处类型变更加一个聚焦的 正确性: 干净。 下游影响: 无。 复用检查: 不适用——这是元数据修复,不是新逻辑。 未发现阻断性问题。 测试结果 (2b)修改前(main 分支,无 PR):73 个测试全部通过。 所有现有运行时校验测试继续通过。构建不受影响。 — Qwen Code · qwen3.7-max |
|
Updated after the author's follow-up commit ( Assessment unchanged from the prior run — the production change is two literal string swaps ( Open suggestion (non-blocking): Verdict: approve. ✅ 中文说明作者在后续 commit ( 评估和上次一致——生产代码变更是两处字符串替换( 未解决的建议(不阻断): 结论:批准。✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
| } | ||
| ).createInvocation(params); | ||
|
|
||
| describe('schema', () => { |
There was a problem hiding this comment.
[Suggestion] The new schema test asserts the static type string, but the existing validation tests (negative, zero, over-limit) never cover fractional/float values like 1.5. The Number.isInteger() rejection path at monitor.ts:740,752 has no test.
Consider adding fractional rejection tests alongside the existing validation tests:
it('rejects fractional max_events', () => {
expect(validate({ command: 'tail -f log', max_events: 1.5 })).toBe(
'max_events must be a positive integer.',
);
});
it('rejects fractional idle_timeout_ms', () => {
expect(validate({ command: 'tail -f log', idle_timeout_ms: 500.5 })).toBe(
'idle_timeout_ms must be a positive integer.',
);
});— qwen3.7-max via Qwen Code /review
| }, | ||
| max_events: { | ||
| type: 'number', | ||
| type: 'integer', |
There was a problem hiding this comment.
[Suggestion] After this change, AJV enforces type: 'integer' at the schema level, making the manual Number.isInteger() checks at lines 740/752 redundant for the normal code path. However, the schema declares no minimum/maximum, so the manual range checks remain load-bearing. There's no comment explaining which validation layer owns which constraint.
Other tools in the codebase document this explicitly (e.g., shell.test.ts has "Non-number timeout (schema validation catches this first)"). A brief comment at the top of validateToolParamValues would help future maintainers:
// AJV enforces type: 'integer' from the schema. This method adds
// range checks (min/max) that the schema does not express.
— qwen3.7-max via Qwen Code /review
Co-authored-by: chatgpt-codex-connector[bot] <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com>
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
[Suggestion] The documentation at docs/developers/tools/monitor.md (lines 24, 27) still describes both max_events and idle_timeout_ms as (number, optional). Since this PR tightens the schema to integer, the docs should be updated to (integer, optional) for consistency.
— qwen3.7-max via Qwen Code /review
| }); | ||
|
|
||
| it('rejects fractional max_events', () => { | ||
| expect(validate({ command: 'tail -f log', max_events: 1.5 })).toBe( |
There was a problem hiding this comment.
[Suggestion] The new fractional-rejection tests (and the schema assertion test above) call validateToolParamValues directly via the private-access helper, bypassing AJV schema validation. In production, validateToolParams runs AJV first — which now rejects fractional values at the schema layer before the custom validator ever runs.
Consider adding at least one integration-level test that exercises the full validateToolParams chain with a fractional input, e.g.:
it('rejects fractional max_events via schema validation', () => {
const result = monitorTool.validateToolParams({
command: 'tail -f log',
max_events: 1.5,
});
expect(result).not.toBeNull();
});This would confirm that AJV actually enforces type: 'integer' end-to-end, making the schema change observable in tests.
— qwen3.7-max via Qwen Code /review
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
This is mostly a schema-alignment PR rather than a runtime bug fix. The monitor runtime validator already rejects fractional values for these fields, so maintainers should feel free to accept this as a small consistency improvement or close it if the current runtime-only guard is considered sufficient.
What this PR does
This PR changes the monitor tool schema for
max_eventsandidle_timeout_msfromnumbertointeger, matching the validation behavior that already requires both values to be positive integers. It also adds a focused schema assertion so the declaration stays aligned with the runtime validation contract.Why it's needed
max_eventsandidle_timeout_msare discrete limits: one is an event count, and the other is a millisecond timeout value. The monitor validator already treats them that way by rejecting non-integer values withmax_events must be a positive integer.andidle_timeout_ms must be a positive integer.Before this change, the public tool schema still exposed both fields as
number. That could make tool callers believe fractional values were schema-valid even though the monitor tool would reject them at runtime. This PR makes the schema describe the same contract that the validator already enforces.This does not expand monitor behavior or change process lifetime handling. It only tightens the declared parameter shape for values that were already invalid at the tool boundary.
Reviewer Test Plan
How to verify
A reviewer can confirm that the monitor schema now declares
max_eventsandidle_timeout_msasinteger, while the existing runtime validation still rejects invalid fractional or non-positive values. The new unit test covers the schema contract directly, and the existing monitor validation tests continue to cover the runtime guard.Evidence (Before & After)
Before:
After:
Runtime behavior was already protected by
Number.isInteger(...), so the visible behavior for invalid fractional values is unchanged: they remain rejected.Tested on
Environment (optional)
Windows local validation used Node.js
v24.15.0/ npm11.12.1. WSL validation used Ubuntu-22.04 with Node.jsv24.15.0/ npm11.12.1.Commands run on Windows:
Results on Windows:
Commands run on WSL Ubuntu-22.04:
Results on WSL Ubuntu-22.04:
Risk & Scope
Linked Issues
N/A — no linked issue. This is a small consistency cleanup discovered during local tool-schema review.
中文说明
这更多是一个 schema 对齐 PR,而不是运行时 bug 修复。monitor 的运行时校验已经会拒绝小数,所以 maintainer 如果认为当前运行时保护已经足够,也完全可以不接受这个 PR。
What this PR does
这个 PR 将 monitor 工具的
max_events和idle_timeout_msschema 从number改成integer,使它们和已有运行时校验保持一致。PR 也补了一个聚焦的 schema 断言,防止后续声明再次和校验语义分离。Why it's needed
max_events和idle_timeout_ms都是离散限制:一个是事件数量,另一个是毫秒超时时间。monitor validator 当前已经通过Number.isInteger(...)要求它们必须是正整数,并会返回max_events must be a positive integer./idle_timeout_ms must be a positive integer.。修改前,公开 tool schema 仍然把这两个字段声明为
number。这会让 schema-aware 的调用方误以为小数是合法输入,但运行时又会拒绝它们。这个 PR 只是让 schema 描述和 validator 已经执行的契约一致。这不会扩展 monitor 行为,也不会改变后台进程生命周期管理。它只收紧两个本来就不合法的参数声明。
Reviewer Test Plan
How to verify
Reviewer 可以确认 monitor schema 现在将
max_events和idle_timeout_ms声明为integer,同时现有运行时校验仍然拒绝非法小数或非正数。新增测试直接覆盖 schema 契约,已有 monitor validation 测试继续覆盖运行时保护。Evidence (Before & After)
修改前:两个字段声明为
number。修改后:两个字段声明为integer。运行时行为本来就由Number.isInteger(...)保护,因此非法小数的可见行为不变,仍然会被拒绝。Tested on
Windows 和 WSL Ubuntu-22.04 已测试;macOS 未本地测试。Windows 验证命令为
cd packages/core && npx vitest run src/tools/monitor.test.ts、npm run build --workspace=packages/core、git diff --check,结果均通过。WSL Ubuntu-22.04 验证命令为cd packages/core && npx vitest run src/tools/monitor.test.ts --pool forks --poolOptions.forks.singleFork=true、npm run build --workspace=packages/core,结果均通过。Risk & Scope
主要风险很低:这只让 schema metadata 和已有 validation 对齐。唯一变化是 schema-aware 的调用链可能更早识别小数输入非法,而不是等到 monitor validator 再拒绝。没有支持的整数用法会改变,小数值此前也已经不是有效输入。
Linked Issues
N/A — 这是本地 tool-schema review 中发现的小一致性修复,没有关联 issue。