Skip to content

feat: accept view_range as alias for offset/length in read_file #517

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wonderwhy-er wants to merge 1 commit into from
Closed

feat: accept view_range as alias for offset/length in read_file #517

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions src/handlers/filesystem-handlers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,19 @@ export async function handleReadFile(args: unknown): Promise<ServerResult> {

const defaultLimit = config.fileReadLineLimit ?? 1000;

// Normalize view_range ([startLine, endLine], 1-based inclusive) into the
// offset/length the readers use (offset is 0-based). This lets clients that
// reach for the text-editor style view_range "just work".
let effectiveOffset = parsed.offset ?? 0;
let effectiveLength = parsed.length ?? defaultLimit;
if (parsed.view_range) {
Comment on lines +94 to +95

@coderabbitai coderabbitai Bot Jun 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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.

const [start, end] = parsed.view_range;
effectiveOffset = Math.max(0, Math.floor(start) - 1);
effectiveLength = end < 0
? defaultLimit // endLine -1 => "to end", capped by config
: Math.max(1, Math.floor(end) - Math.floor(start) + 1);
Comment on lines +98 to +100

@coderabbitai coderabbitai Bot Jun 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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.

Suggested change
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.

}

// Convert sheet parameter: numeric strings become numbers for Excel index access
let sheetParam: string | number | undefined = parsed.sheet;
if (parsed.sheet !== undefined && /^\d+$/.test(parsed.sheet)) {
Expand All @@ -95,8 +108,8 @@ export async function handleReadFile(args: unknown): Promise<ServerResult> {

const options: ReadOptions = {
isUrl: parsed.isUrl,
offset: parsed.offset ?? 0,
length: parsed.length ?? defaultLimit,
offset: effectiveOffset,
length: effectiveLength,
sheet: sheetParam,
range: parsed.range
};
Expand Down
5 changes: 5 additions & 0 deletions src/server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,12 +313,17 @@ server.setRequestHandler(ListToolsRequestSchema, async () => {
- 'length' (max lines to read, default: configurable via 'fileReadLineLimit' setting, initially 1000)
* Used with positive offsets for range reading
* Ignored when offset is negative (reads all requested tail lines)
- 'view_range' ([startLine, endLine], 1-based and inclusive) — convenient
alternative to offset/length for reading a specific span of lines.
Use endLine -1 to read from startLine to the end of the file.

Examples:
- offset: 0, length: 10 → First 10 lines
- offset: 100, length: 5 → Lines 100-104
- offset: -20 → Last 20 lines
- offset: -5, length: 10 → Last 5 lines (length ignored)
- view_range: [100, 150] → Lines 100-150 (1-based, inclusive)
- view_range: [100, -1] → Line 100 to end of file

Performance optimizations:
- Large files with negative offsets use reverse reading for efficiency
Expand Down
4 changes: 4 additions & 0 deletions src/tools/schemas.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,10 @@ export const ReadFileArgsSchema = z.object({
isUrl: z.boolean().optional().default(false),
offset: z.number().optional().default(0),
length: z.number().optional().default(1000),
// 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(),
Comment on lines +53 to +56

@coderabbitai coderabbitai Bot Jun 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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.

Suggested change
// 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.

sheet: z.string().optional(), // String only for MCP client compatibility (Cursor doesn't support union types in JSON Schema)
range: z.string().optional(),
options: z.record(z.any()).optional()
Expand Down
Loading