Thanks for the PR!

Template looks good ✓

Problem: This is input-validation hardening without a live reproduction — no linked issue or before/after showing a real tool call that emitted fractional offset/limit. That said, the validation gap is directly observable in the code: the current params.offset < 0 / params.limit <= 0 checks let 1.5 through, and the display string at line 87 (lines ${offset + 1}-${offset + limit}) would produce nonsense like "lines 2.5–3.5". So the bug is real even if nobody has tripped it yet.

Direction: Clearly aligned — tightening tool-boundary validation for a core tool is housekeeping, not feature creep. The schema change from number to integer is the main load-bearing fix (the LLM gets the JSON Schema and the framework enforces it at parse time); the runtime Number.isInteger() guard is defense-in-depth. No CHANGELOG precedent needed for this kind of fix.

Approach: Scope is tight — schema type change, runtime guard, updated error messages, and tests covering fractional offset, fractional limit, and valid zero-offset. Nothing extra. This is the minimal change.

Moving on to code review. 🔍

— Qwen Code · qwen3.7-max

Code Review

My independent proposal before reading the diff: change the JSON Schema type from number to integer for both fields, add Number.isInteger() runtime guards, update error messages, and add tests for fractional inputs.

The PR matches this exactly — same files, same approach, same test coverage. No simpler path was missed.

Correctness: Clean. The Number.isInteger() check correctly rejects fractional values while allowing 0 and positive integers. The combined conditions (!Number.isInteger(x) || x < 0 for offset, !Number.isInteger(x) || x <= 0 for limit) handle all edge cases including NaN and Infinity (both rejected by Number.isInteger()). Error messages updated from "number" to "integer" to match.

Tests: 65 → 70 tests, all passing. New coverage: fractional offset (1.5), fractional limit (0.5, 1.5), zero offset (valid), zero/negative limit (invalid). Existing offset-negative and limit-zero tests updated with new messages. Good coverage of the boundary conditions.

Reuse: No new abstractions or helpers — the validation is inline where it belongs. No issues.

No blockers found.

Real-Scenario Testing

Integer pagination with offset=4, limit=11 on a 50-line file — verifying the PR doesn't regress normal behavior.

Before (installed build)

$ qwen -p 'Read the file /tmp/triage-test-file.txt using offset 4 and limit 11' --max-session-turns 2 -y -o text
Warning: running headless with --yolo / approval-mode=yolo and no sandbox.
Lines 5–15 of the 51-line file — all containing `Lorem ipsum dolor sit amet`.
The offset/limit worked correctly: 0-based offset 4 starts at line 5, and 11 lines were returned.

After (this PR via dev build)

$ npm run dev -- -p 'Read the file /tmp/triage-test-file.txt using offset 4 and limit 11' --max-session-turns 2 -y -o text
Warning: running headless with --yolo / approval-mode=yolo and no sandbox.
Lines 5–15 of `/tmp/triage-test-file.txt` — all contain `Lorem ipsum dolor sit amet`.
The file has 51 total lines.

Both produce identical pagination results (lines 5–15, 11 lines from offset 4). Normal integer pagination is unaffected by the schema change.

Unit Tests

# Before (main): 65 passed
# After (this PR): 70 passed — 5 new tests for fractional/integer validation
# Core package typecheck: clean

— Qwen Code · qwen3.7-max

This is a clean, minimal input-validation fix. The schema change from number to integer is the load-bearing improvement — it tells both the LLM and the framework that fractional pagination params are invalid, catching them at the JSON Schema validation layer before they reach runtime. The Number.isInteger() guard is defense-in-depth.

The PR matches what I would have written independently: same files, same approach, same test coverage. No scope creep, no unnecessary abstractions, no drive-by refactors. The diff is exactly what the goal needs and nothing more.

Real-scenario testing confirms integer pagination works identically before and after. Unit tests go from 65 → 70, all passing, with good boundary coverage (fractional offset/limit, zero offset, negative limit).

Approving. ✅

— Qwen Code · qwen3.7-max

✅ Maintainer verification — real end-to-end run

I verified this PR locally by driving the real compiled ReadFileTool end-to-end (not just the unit tests), plus A/B toggle experiments that isolate exactly which change enforces what. Verdict: correct and safe to merge. One non-blocking observation below about the runtime guard being redundant/untested.

Environment: Linux (Debian 13, kernel 6.12), Node v22.22.2, fresh npm ci in a dedicated git worktree at PR head fd339c558 (parent 5c8af1a1f). True PR scope confirmed = the 2 files in the diff (50+/12−); the GitHub "files changed" count is accurate.

Methodology

1. Compiled-artifact E2E — imported the built packages/core/dist/src/tools/read-file.js and called tool.build(params) against a real temp file, capturing the actual thrown message or, on success, the actual getDescription() output. Ran it against both the PR build and a base-main build (same file reverted to pre-PR, core rebuilt).
2. A/B toggles — reverted each half of the fix independently and re-ran the PR's own read-file.test.ts (70 tests) to prove causation.
3. Standard gates: read-file.test.ts, typecheck --workspace=packages/core, git diff --check.

Compiled-artifact E2E — real ReadFileTool.build()

The base column reproduces the exact bug described in the PR: fractional pagination is accepted and renders nonsensical ranges like lines 2.5-3.5. And because JS Array.slice() truncates fractional indexes (['l1','l2','l3'].slice(1.5) → ['l2','l3']), the displayed range (from line 2.5) and the actual content read (from line 2) diverge — the ambiguous semantics the PR set out to remove. Valid integer inputs (and integer-valued numbers, which JSON/JS treat as integers) are unaffected; offset: 0 still works.

A/B toggle proofs (vitest, read-file.test.ts, 70 tests)

Why this happens (control flow)

BaseDeclarativeTool.validateToolParams() runs AJV schema validation first and returns on the first error, then calls validateToolParamValues(). So:

• Fractional inputs are rejected by the schema (type: 'integer') → params/… must be integer. The four-pass coercion in SchemaValidator only fixes numeric strings, so a fractional number stays rejected.
• The new runtime !Number.isInteger() clauses only ever run for values that already passed the schema (i.e. real integers), so they never fire for fractional inputs on the build() path.

Non-blocking observations (for the merge decision)

1. The one-line schema change (number→integer) is the real fix. The runtime !Number.isInteger() additions are harmless defense-in-depth (they'd only matter if the schema-then-runtime order ever changed), but Experiment 2 shows they are fully untested — deleting them keeps all 70 green. Consider either adding a direct validateToolParamValues unit test to lock the behavior in, dropping the clause, or keeping it with a // defense-in-depth comment.
2. Two message styles for closely related failures: fractional → params/offset must be integer (JSON-pointer/AJV style); negative/zero → Offset must be a non-negative integer (friendly style). This split pre-exists for negatives and isn't a regression — just noting the model/user sees two shapes.

Gate results

• read-file.test.ts: 70 passed ✅
• typecheck --workspace=packages/core: passed (tsc --noEmit, rc=0) ✅
• git diff --check: clean ✅ (the only warning is a pre-existing CRLF note on an unrelated NOTICES.txt)

Recommendation: 👍 merge. The fix does what it claims, valid callers are unaffected, and the reproduction/regression are both confirmed. Optionally address observation #1 (untested runtime guard) either now or in a follow-up.