[CouchRiv]

Reading an export is the first thing an agent does with a workbook, and the engine is fast at it, but the MCP reply wasn't usable at size. read_range without range returned the whole used range in one reply (up to the 1,000,000-cell limit), and every tool result was pretty-printed, one cell value per line. On a synthetic 15,000 × 30 CSV (generated data), read_range {} was one text block of 6,582,075 characters on 480,038 lines, with nothing telling the agent how to ask for less. MCP clients cap tool output, so in practice the agent got a truncated or spilled-to-disk result.

What changed

Two independent commits: pagination first, compact tool-result JSON second. Pagination can land without compact JSON if indented output should be retained, but must include the follow-up commit that rejects invalid maxCells values.

1. Paging (MCP: read_range returns long ranges in pages)

• sheet.read takes maxCells, a positive integer; zero, negative and non-integer values return an error, including on an empty sheet. It reads whole rows up to that many cells (at least one row, so a row wider than maxCells still comes back whole). When the range is longer it adds truncated: true, totalRows (rows of the whole range) and next, the remaining rows as a sheet-qualified range ('My Data'!A334:AD15001). Passing next back as range reads on from there whatever the active sheet is. Without maxCells the command behaves exactly as before, so the control channel and CLI are unchanged.
• The MCP read_range tool exposes maxCells and passes 5,000 when the caller gives none. The 1,000,000-cell limit still applies to what is read.
• params text, the tool description, docs/mcp.md and docs/control-protocol.md describe it.

2. Compact JSON (MCP: tool results as compact JSON)

• ToolResult::json uses serde_json::to_string instead of to_string_pretty. Agents parse the JSON either way; indented, 100 rows × 30 columns took 44,368 characters on 3,238 lines, compact 22,132 on one. docs/mcp.md said "pretty JSON" and now says compact.

Not included: an ISO date option for read_range (dates come back as serials unless formatted, where numbers become text too), and documenting in write_range that "'0010" or a Text format keeps a code with leading zeros as text.

Verification (Linux x86_64, original baseline fc45b7f)

• cargo test -p gridcraft-engine --lib tests::reading_a_range_in_pages: 10 rows × 3 columns on a sheet named My Data with another sheet active. maxCells: 7 gives A1:C2, totalRows 10 and next 'My Data'!A3:C10; reading next returns the 8 remaining rows with no truncated; a positive maxCells smaller than a row gives one row; no maxCells gives the whole range. On main: left: "A1:C10", right: "A1:C2". Passes here.
• cargo test -p gridcraft-mcp tests::read_range_pages_long_ranges: 2,000 rows × 3 through write_range, then read_range {} gives A1:C1666, truncated, totalRows 2000, next Sheet1!A1667:C2000; the next call reads the rest; maxCells: 6000 reads everything. On main: left: ("A1:C2000", Null, Null). Passes here.
• cargo test -p gridcraft-mcp tests::tool_results_are_compact_json: the text of a read_range result has no line break. Fails on main, passes here.
• gridcraft-cli mcp on the synthetic 15,000 × 30 CSV:

call	main (fc45b7f)	this branch
read_range {}	6,582,075 chars, 480,038 lines, A1:AD15001	36,398 chars, 1 line, A1:AD166, truncated, totalRows 15001, next large!A167:AD15001
read_range {range: "A1:AD101"}	44,368 chars, 3,238 lines	22,132 chars, 1 line

• cargo fmt --all -- --check, cargo clippy -p gridcraft-engine -p gridcraft-mcp --all-targets -- -D warnings, cargo test -p gridcraft-engine -p gridcraft-mcp -p gridcraft-cli

Limits and questions

• The default of 5,000 cells is a guess at what fits a client's output limit with room to spare: about 36,000 characters on that export, more with long text. Happy to change it, or to count characters instead of cells.
• An agent that relied on read_range {} returning the whole used range now gets the first page with truncated: true, and needs maxCells or next to read the rest.

Independent verification against updated main (84acf62, 2026-10-13)

Before the invalid-maxCells follow-up, the independent review reran cargo fmt --all -- --check, cargo clippy -p gridcraft-engine -p gridcraft-mcp --all-targets -- -D warnings and cargo test -p gridcraft-engine -p gridcraft-mcp on the branch and its clean merge with 84acf62; all exited 0. The added regression tests failed with production code from 84acf62 and passed on both branch and merged trees. These checks are separate from the historical fc45b7f measurements above.

Follow-up verification: invalid maxCells (2026-10-13)

• sheet_read_rejects_invalid_max_cells fails without the fix, including on an empty sheet, and passes with it.
• read_range_rejects_invalid_max_cells writes 2,000 rows by 3 columns and confirms that the default page contains 1,666 rows. Without the fix, maxCells: -1 returns all 2,000 rows instead of an error. With the fix, negative integers and zero fail; non-integer MCP arguments fail as JSON-RPC invalid params. maxCells: 1 still returns one complete row.
• cargo fmt --all -- --check, cargo clippy -p gridcraft-engine -p gridcraft-mcp --all-targets -- -D warnings, and cargo test -p gridcraft-engine -p gridcraft-mcp pass: 245 engine tests and 22 MCP tests, no failures. These checks ran on the branch after the follow-up, separately from the earlier review of its merged tree.

Conflicts

Against upstream/main at 84acf62, git merge-tree is clean. #884 (my open conditional-format PR) conflicts only where the engine tests are inserted in tests.rs; retain both tests. #870, #480, #885 and #1064 merge cleanly. The review covered 100 listed open PRs plus #480, not every older PR.

Summary by CodeRabbit

• New Features
  • Sheet reads can now be limited by a positive maxCells value and return whole-row pages. When more rows remain, results include the total row count and the range to read next.
  • The range-reading tool defaults to a 5,000-cell limit; callers can set a different limit.
• Improvements
  • Tool results now use compact, single-line JSON.
• Documentation
  • Updated the protocol and tool guides with pagination, limit validation, and response details.

[coderabbitai]

No actionable comments were generated in the recent review.

Recent review info

Run configuration

• Source mode: Auto
• Repository: storytold/gridcraft
• Branch: main
• File: .coderabbit.yaml
• Commit: 146161b
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 0dd3c93e-aeb2-4a7d-a1fd-234adac249da

Commits

Reviewing files that changed from the base of the PR and between 146161b and e231b0f.

Files selected for processing: 6

• crates/engine/src/cmd/inspect.rs
• crates/engine/src/tests.rs
• crates/mcp/src/tests.rs
• crates/mcp/src/tools.rs
• docs/control-protocol.md
• docs/mcp.md

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

Walkthrough

sheet.read now supports whole-row pagination through an optional maxCells limit. The MCP read_range tool defaults that limit to 5,000 and returns compact, single-line JSON.

Changes: Sheet read pagination

Layer / File(s)	Summary
Engine pagination and validation crates/engine/src/cmd/inspect.rs, crates/engine/src/tests.rs, docs/control-protocol.md	sheet.read validates maxCells as a positive integer and returns whole-row pages. If rows remain, the response includes truncated, the original totalRows, and a sheet-qualified next range. Tests cover validation, page boundaries, and full-range reads.
MCP tool integration and result format crates/mcp/src/tools.rs, crates/mcp/src/tests.rs, docs/mcp.md	read_range accepts and forwards maxCells, using 5,000 when omitted. Tool results use compact JSON. Tests cover pagination, validation, and the JSON format.

Priority: Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Feature

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant read_range
  participant sheet.read
  participant ToolResult.json
  Caller->>read_range: Request range and optional maxCells
  read_range->>sheet.read: Forward range and limit, defaulting to 5000
  sheet.read-->>read_range: Return values and continuation metadata
  read_range->>ToolResult.json: Serialize tool result
  ToolResult.json-->>Caller: Return compact JSON

Loading

Suggested reviewers: echelon

Merge Risk: Minimal · up to e231b

No merge-blocking issue was identified in the paging or compact JSON changes.

Security architecture risk: Minimal · up to e231b

The inspected changes preserve workbook targeting and read-size enforcement while making large reads paginated by default. No introduced or materially worsened security risk was identified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• observed — The changed read path remains scoped through existing workbook targeting and engine sheet/range selection. Callers can request values, formulas, or displayed text as before; the new argument controls page shape rather than granting access to additional workbooks or sheets.

Trust Boundaries and Controls

• observed — Caller-controlled maxCells passes through the existing MCP argument-validation and backend-dispatch boundary. The engine independently rejects zero, negative, non-integer, and other invalid values before any empty-sheet early return, preserving enforcement for direct engine callers.

Resilience and Maintainability Implications

• observed — Validation and oversized-page errors occur before result-row allocation in the read handler. Since paging only constructs a response, these failures leave no partially advanced paging state requiring rollback or recovery.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely summarizes the two main changes: paginated long-range reads and compact JSON tool results.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 4 functions across 4 files. (2 skipped: 2 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[FredApps]

ReviewerA — source-read COMMENT (not an approval)

I read the diff: the MCP server's sheet.read gets an optional maxCells (a positive integer, else an error) that reads whole rows up to that many cells (at least one row) and, when more remain, says so in the reply (truncated: true, totalRows, and next, the range where the rest starts, with the sheet name quoted by quote_sheet), so a caller can page through a long range instead of getting it in one reply; the second part of the title, compact JSON for tool results, is in a file I did not read. Paging by whole rows keeps each page well formed, and next is an address a caller can pass back, which is the right shape for an agent. The test reading_a_range_in_pages makes a sheet with a space in its name, writes ten rows of three cells and reads them in pages; a sheet name that needs quotes is a good case to include. Points: maxCells has no upper bound, so a caller can still ask for the whole sheet: say whether a server-side cap applies; a default page size for a client that does not pass it would protect the model's context; and the u64 to u32 cast in n as u32 - 1 is safe only because n < total_rows, which is a u32, so say so in a comment. I have not run it, and no exact-head CI result exists for this head, so this is a source-read comment and not an approval. Nothing was run.

I ran nothing and have no exact-head CI result for this PR, so this is not an approval. Where a diff was long I read its code hunks rather than every generated or documentation line.

Status: waiting_author.