feat(session-manager): ✨ add offset pagination to session_read tool#2114
feat(session-manager): ✨ add offset pagination to session_read tool#2114Lynricsy wants to merge 1 commit intocode-yeongyu:devfrom
Conversation
Add 1-indexed offset parameter for session_read, enabling pagination similar to the file Read tool. Includes validation for out-of-range offsets, pagination headers in output, and 7 new test cases. Co-authored-by: Wine Fox <fox@ling.plus>
There was a problem hiding this comment.
Pull request overview
Adds 1-indexed offset support to the session_read tool so callers can paginate through session messages starting at an arbitrary message number, with updated formatting and tests.
Changes:
- Extend
SessionReadArgsandsession_readtool schema to accept anoffsetparameter. - Implement offset/limit slicing and conditional pagination context passed into the session message formatter.
- Update session message formatting to include a pagination header and message numbering; add tests for offset/limit behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
src/tools/session-manager/types.ts |
Adds offset?: number to SessionReadArgs to support pagination. |
src/tools/session-manager/tools.ts |
Adds offset validation and applies offset/limit slicing; passes pagination context to formatter. |
src/tools/session-manager/session-formatter.ts |
Adds pagination header and message numbering to formatted output. |
src/tools/session-manager/constants.ts |
Updates session_read tool description/docs to mention offset and pagination example. |
src/tools/session-manager/tools.test.ts |
Adds new tests intended to cover offset validation and pagination formatting. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| beforeAll(() => { | ||
| mock.module("./storage", () => ({ | ||
| sessionExists: () => Promise.resolve(true), | ||
| readSessionMessages: () => Promise.resolve([...mockMessages]), | ||
| readSessionTodos: () => Promise.resolve([]), |
There was a problem hiding this comment.
mock.module("./storage", ...) is set up in this hook, but session_read is instantiated earlier at module load time. With ESM imports, mocking after the tool is created may not replace the already-imported ./storage bindings, so these tests can unintentionally hit the real filesystem. Apply the mock before importing/creating the tools (e.g., dynamic import after mocking or create a fresh tool instance inside the setup).
| const result = await session_read.execute({ session_id: "ses_mock", offset: 2, limit: 2 }, mockContext) | ||
|
|
||
| expect(result).not.toContain("Error") | ||
| expect(typeof result).toBe("string") |
There was a problem hiding this comment.
This test claims to "return correct slice" but only asserts that the result is a string and doesn’t contain "Error". Add assertions that the returned output includes the expected messages (and excludes messages outside the requested offset/limit), otherwise the new pagination logic isn’t actually verified.
| expect(typeof result).toBe("string") | |
| expect(typeof result).toBe("string") | |
| // Expect pagination header for the slice corresponding to offset=2, limit=2 on 5 messages. | |
| expect(result).toContain("Showing messages 2-3 of 5") | |
| // Ensure headers for other ranges are not present. | |
| expect(result).not.toContain("Showing messages 1-2 of 5") | |
| expect(result).not.toContain("Showing messages 3-5 of 5") |
| if (args.offset !== undefined) { | ||
| if (args.offset < 1) { | ||
| return `Error: offset must be >= 1, got ${args.offset}` | ||
| } | ||
| if (args.offset > totalMessages) { |
There was a problem hiding this comment.
offset accepts any number (including NaN/Infinity or non-integers). With the current checks, values like 1.5 or NaN can pass validation and then Array.slice() will coerce them, producing an incorrect start position. Validate that offset is a finite integer before using it (and return a clear error otherwise).
| return `Error: offset ${args.offset} exceeds total message count (${totalMessages})` | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
limit is also treated as a generic number; fractional/NaN values will be coerced by slice() (and negative values silently disable limiting). Since limit is described as a message count, it should be validated as a finite integer (and probably >= 1 when provided) to avoid surprising pagination behavior.
| if (args.limit !== undefined) { | |
| if (!Number.isFinite(args.limit) || !Number.isInteger(args.limit)) { | |
| return `Error: limit must be a finite integer, got ${args.limit}` | |
| } | |
| if (args.limit < 1) { | |
| return `Error: limit must be >= 1, got ${args.limit}` | |
| } | |
| } |
| if (pagination) { | ||
| const endIndex = pagination.offset + messages.length - 1 | ||
| lines.push(`Showing messages ${pagination.offset}-${endIndex} of ${pagination.totalMessages}`) | ||
| lines.push("") |
There was a problem hiding this comment.
lines.push("") adds a blank line after the pagination header, but each message line is also prefixed with \n later. When pagination is set this results in extra blank lines (e.g., three consecutive newlines before the first message). Consider removing the empty-string push and/or removing the leading newline from message lines so spacing is consistent.
| lines.push("") |
| const timestamp = msg.time?.created ? new Date(msg.time.created).toISOString() : "Unknown time" | ||
| const agent = msg.agent ? ` (${msg.agent})` : "" | ||
| lines.push(`\n[${msg.role}${agent}] ${timestamp}`) | ||
| lines.push(`\n[Message ${messageNumber}] ${msg.role}${agent} ${timestamp}`) |
There was a problem hiding this comment.
The message header string begins with \n, which creates a leading blank line in the output and compounds with the pagination header spacing. If you want a blank line between messages, it’s usually clearer to add separators explicitly (e.g., push an empty line between messages) rather than embedding newlines in the content lines.
| Example output: | ||
| Session: ses_abc123 | ||
| Messages: 45 | ||
| Messages: 3-4 of 45 | ||
| Date Range: 2025-12-20 to 2025-12-24 | ||
|
|
There was a problem hiding this comment.
The SESSION_READ_DESCRIPTION example output includes Session: ..., Messages: ..., and Date Range: ..., but session_read currently returns only formatSessionMessages(...) output (no session/date-range header). This makes the tool docs misleading; update the example to match the real output or update session_read to actually include those header lines.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 5 files
Confidence score: 4/5
- This PR looks safe to merge with minimal risk; the only issue is a moderate-strength test weakness rather than a functional bug.
src/tools/session-manager/tools.test.tsclaims to verify the correct slice but doesn’t assert slice contents, so it may miss regressions.- Pay close attention to
src/tools/session-manager/tools.test.ts- strengthen assertions to validate slice contents.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/tools/session-manager/tools.test.ts">
<violation number="1" location="src/tools/session-manager/tools.test.ts:201">
P2: Test claims to verify 'correct slice' but has weak assertions that don't check slice contents</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| }) | ||
| }) | ||
|
|
||
| describe("#when session_read is called with offset=2 and limit=2", () => { |
There was a problem hiding this comment.
P2: Test claims to verify 'correct slice' but has weak assertions that don't check slice contents
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/tools/session-manager/tools.test.ts, line 201:
<comment>Test claims to verify 'correct slice' but has weak assertions that don't check slice contents</comment>
<file context>
@@ -134,3 +137,103 @@ describe("session-manager tools", () => {
+ })
+ })
+
+ describe("#when session_read is called with offset=2 and limit=2", () => {
+ test("#then executes without error and returns correct slice", async () => {
+ const result = await session_read.execute({ session_id: "ses_mock", offset: 2, limit: 2 }, mockContext)
</file context>
Summary
offsetparameter tosession_readtool for message pagination, similar to how the fileReadtool worksChanges
types.tsoffset?: numbertoSessionReadArgsconstants.tstools.tssession-formatter.tstools.test.tsMotivation
The existing
session_readtool only supportslimit(read N messages from the beginning), making it inconvenient to read recent messages in long sessions. This change addsoffsetsupport so users can jump to any position in the message history — useful since the most relevant information is often near the end of a session.Testing
tsc --noEmit)Summary by cubic
Added 1-indexed offset pagination to session_read so you can start reading from any message. Output now shows the message range and numbers each message, with clear errors for invalid offsets.
Written for commit e5d660b. Summary will update on new commits.