feat: accept view_range as alias for offset/length in read_file#517
feat: accept view_range as alias for offset/length in read_file#517wonderwhy-er wants to merge 1 commit into
Conversation
LLM clients frequently call read_file with a view_range: [start, end] argument (the text-editor convention) instead of offset/length. That key was silently dropped, so the schema defaults took over and the tool read 1000 lines from the start of the file. Accept view_range ([startLine, endLine], 1-based inclusive) as a first-class optional argument and normalize it to offset/length in the handler. endLine of -1 reads to end of file. offset/length still work unchanged. Tool description updated to document view_range alongside offset/length.
📝 WalkthroughWalkthroughAdds an optional Changesview_range line-range reading for read_file
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/handlers/filesystem-handlers.ts`:
- Around line 98-100: The condition checking for the special case in the
view_range calculation is too permissive. Change the condition from `end < 0` to
`end === -1` in the effectiveLength assignment, as the documented API specifies
only the value -1 to mean "to end of file". Using a strict equality check
ensures that only the documented special case is handled, and any other negative
values will not silently succeed with undocumented behavior.
- Around line 94-95: The effectiveLength assignment on line 94 uses
parsed.length as the primary value with defaultLimit as a fallback, but since
parsed.length has a schema default value of 1000, it is never undefined and the
configured defaultLimit is never applied. To fix this, modify the logic to check
whether the caller explicitly provided a length value (rather than relying on
the schema default), ensuring that when length is omitted, the configured
fileReadLineLimit setting is properly used instead of being bypassed by the
schema default.
In `@src/tools/schemas.ts`:
- Around line 53-56: The view_range field definition in the schema only
validates the shape (array of 2 numbers) but lacks contract-level validation for
the actual constraints. Replace the current
z.array(z.number()).length(2).optional() definition with a more comprehensive
validation that enforces the following rules: startLine must be >= 1, both
values must be integers, and endLine must either be -1 or be >= startLine. Use
Zod's refine method or int() helper combined with additional validation logic to
ensure these constraints are checked at schema level rather than relying on
later coercion, preventing malformed inputs from being silently accepted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1c733bf3-0a98-482f-af3b-ce5179b7fc59
📒 Files selected for processing (3)
src/handlers/filesystem-handlers.tssrc/server.tssrc/tools/schemas.ts
| let effectiveLength = parsed.length ?? defaultLimit; | ||
| if (parsed.view_range) { |
There was a problem hiding this comment.
Configured fileReadLineLimit is bypassed for normal reads.
Line 94 falls back to defaultLimit, but parsed.length is pre-filled by schema default (1000), so config won’t apply when callers omit length. This makes the runtime behavior diverge from the documented configurable default.
Proposed fix (preserve config default semantics)
- let effectiveOffset = parsed.offset ?? 0;
- let effectiveLength = parsed.length ?? defaultLimit;
+ const rawArgs = args as Record<string, unknown>;
+ const hasOffset = Object.prototype.hasOwnProperty.call(rawArgs, 'offset');
+ const hasLength = Object.prototype.hasOwnProperty.call(rawArgs, 'length');
+
+ let effectiveOffset = hasOffset ? (parsed.offset ?? 0) : 0;
+ let effectiveLength = hasLength ? (parsed.length ?? defaultLimit) : defaultLimit;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/handlers/filesystem-handlers.ts` around lines 94 - 95, The
effectiveLength assignment on line 94 uses parsed.length as the primary value
with defaultLimit as a fallback, but since parsed.length has a schema default
value of 1000, it is never undefined and the configured defaultLimit is never
applied. To fix this, modify the logic to check whether the caller explicitly
provided a length value (rather than relying on the schema default), ensuring
that when length is omitted, the configured fileReadLineLimit setting is
properly used instead of being bypassed by the schema default.
| effectiveLength = end < 0 | ||
| ? defaultLimit // endLine -1 => "to end", capped by config | ||
| : Math.max(1, Math.floor(end) - Math.floor(start) + 1); |
There was a problem hiding this comment.
view_range special case is too broad (end < 0 instead of end === -1).
On Line 98, any negative end is interpreted as “to end of file,” but the documented API defines only -1 for that behavior. This creates undocumented inputs that silently succeed.
Proposed fix
- effectiveLength = end < 0
+ effectiveLength = end === -1
? defaultLimit
: Math.max(1, Math.floor(end) - Math.floor(start) + 1);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| effectiveLength = end < 0 | |
| ? defaultLimit // endLine -1 => "to end", capped by config | |
| : Math.max(1, Math.floor(end) - Math.floor(start) + 1); | |
| effectiveLength = end === -1 | |
| ? defaultLimit // endLine -1 => "to end", capped by config | |
| : Math.max(1, Math.floor(end) - Math.floor(start) + 1); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/handlers/filesystem-handlers.ts` around lines 98 - 100, The condition
checking for the special case in the view_range calculation is too permissive.
Change the condition from `end < 0` to `end === -1` in the effectiveLength
assignment, as the documented API specifies only the value -1 to mean "to end of
file". Using a strict equality check ensures that only the documented special
case is handled, and any other negative values will not silently succeed with
undocumented behavior.
| // Optional [startLine, endLine] convenience alias, 1-based and inclusive | ||
| // (matches the text-editor view_range convention). Normalized to offset/length | ||
| // in the handler. endLine of -1 means "to end of file". | ||
| view_range: z.array(z.number()).length(2).optional(), |
There was a problem hiding this comment.
view_range needs contract-level validation, not just shape validation.
On Line 56, the schema enforces only “2 numbers,” but the feature contract is stricter (startLine >= 1, integer lines, and endLine === -1 or endLine >= startLine). Right now malformed inputs are accepted and later coerced, which can return unexpected slices silently.
Proposed schema hardening
- view_range: z.array(z.number()).length(2).optional(),
+ view_range: z
+ .tuple([z.number().int().min(1), z.number().int()])
+ .refine(([start, end]) => end === -1 || end >= start, {
+ message: "view_range must be [startLine, endLine] with endLine = -1 or endLine >= startLine",
+ })
+ .optional(),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Optional [startLine, endLine] convenience alias, 1-based and inclusive | |
| // (matches the text-editor view_range convention). Normalized to offset/length | |
| // in the handler. endLine of -1 means "to end of file". | |
| view_range: z.array(z.number()).length(2).optional(), | |
| // Optional [startLine, endLine] convenience alias, 1-based and inclusive | |
| // (matches the text-editor view_range convention). Normalized to offset/length | |
| // in the handler. endLine of -1 means "to end of file". | |
| view_range: z | |
| .tuple([z.number().int().min(1), z.number().int()]) | |
| .refine(([start, end]) => end === -1 || end >= start, { | |
| message: "view_range must be [startLine, endLine] with endLine = -1 or endLine >= startLine", | |
| }) | |
| .optional(), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/tools/schemas.ts` around lines 53 - 56, The view_range field definition
in the schema only validates the shape (array of 2 numbers) but lacks
contract-level validation for the actual constraints. Replace the current
z.array(z.number()).length(2).optional() definition with a more comprehensive
validation that enforces the following rules: startLine must be >= 1, both
values must be integers, and endLine must either be -1 or be >= startLine. Use
Zod's refine method or int() helper combined with additional validation logic to
ensure these constraints are checked at schema level rather than relying on
later coercion, preventing malformed inputs from being silently accepted.
1 participant
Problem
LLM clients frequently call
read_filewith aview_range: [start, end]argument (the text-editor convention) instead ofoffset/length. That key wasn't in the schema, so it was silently dropped — theoffset/lengthdefaults took over and the tool returned 1000 lines from the start of the file. Looked like the read tool "ignoring the range", but it was just an unrecognized parameter.Change
view_range: [startLine, endLine](1-based, inclusive) as a first-class optional arg onread_file.offset/lengthin the handler:offset = start - 1,length = end - start + 1.endLineof-1reads fromstartLineto end of file (capped byfileReadLineLimit).offset/lengthcontinue to work exactly as before.view_rangealongsideoffset/length. No warnings added — both styles are valid.Verification
Against a 200-line file:
view_range: [50, 55]→ lines 50–55 (6 lines), identical tooffset: 49, length: 6.view_range: [198, -1]→ lines 198–200 (to end).npm run buildpasses.Summary by CodeRabbit
Release Notes
view_rangeparameter for intuitive file access by specifying start and end line numbers (1-based, inclusive).-1as the end line value.