Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 6 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,12 @@ jobs:
if: "${{ needs.classify_pr.outputs.skip_ci != 'true' && steps.ci_profile.outputs.ci_profile == 'full' }}"
run: 'npm run check:desktop-isolation'

- name: 'Run desktop session-tools-core tests'
if: "${{ needs.classify_pr.outputs.skip_ci != 'true' && steps.ci_profile.outputs.ci_profile == 'full' }}"
run: |-
npm exec --yes bun@1.3.14 -- test \
packages/desktop/packages/session-tools-core/src/handlers/list-sessions.test.ts
Comment on lines +285 to +289

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] The CI step is named "Run desktop session-tools-core tests" but hardcodes only list-sessions.test.ts. The tool-defs-filtering.test.ts change made in this same PR (the type === 'integer' schema assertion) is not covered by any CI step — Concrete cost: if a future change reverts z.number().int() to z.number(), the schema test catches it locally but CI stays green, and the regression ships undetected.

Suggested change
- name: 'Run desktop session-tools-core tests'
if: "${{ needs.classify_pr.outputs.skip_ci != 'true' && steps.ci_profile.outputs.ci_profile == 'full' }}"
run: |-
npm exec --yes bun@1.3.14 -- test \
packages/desktop/packages/session-tools-core/src/handlers/list-sessions.test.ts
- name: 'Run desktop session-tools-core tests'
if: "${{ needs.classify_pr.outputs.skip_ci != 'true' && steps.ci_profile.outputs.ci_profile == 'full' }}"
run: |-
npm exec --yes bun@1.3.14 -- test \
packages/desktop/packages/session-tools-core/src/handlers/list-sessions.test.ts \
packages/desktop/packages/session-tools-core/src/tool-defs-filtering.test.ts
中文说明

CI 步骤名为 "Run desktop session-tools-core tests",但只硬编码了 list-sessions.test.ts。本 PR 同时修改的 tool-defs-filtering.test.ts(schema integer 类型断言)没有被任何 CI 步骤覆盖。具体代价:如果未来某个变更将 z.number().int() 回退为 z.number(),schema 测试在本地能捕获,但 CI 仍为绿色,回归会不被发现地合入。

— qwen3.8-max-preview via Qwen Code /review


- name: 'Install linters'
if: "${{ needs.classify_pr.outputs.skip_ci != 'true' && steps.ci_profile.outputs.ci_profile == 'full' }}"
run: 'node scripts/lint.js --setup'
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
import { describe, expect, it } from 'bun:test';
import { handleListSessions } from './list-sessions.ts';
Comment on lines +1 to +2

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] This test file (and the pre-existing tool-defs-filtering.test.ts) is outside every npm workspace and no CI workflow runs bun test in the desktop workspace — these tests never execute in CI — Concrete cost: a future change that weakens the validation guards would merge undetected.

Consider adding a CI step that runs bun test in the desktop workspace (e.g., in ci.yml gated on a full profile), or moving the critical validation tests into a root-workspace test file that npm run test:ci collects.

— qwen3.7-max via Qwen Code /review

@VectorPeak VectorPeak Jul 19, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks, addressed. I added a full-profile CI step that runs the focused desktop regression test with npm exec --yes bun@1.3.14 -- test packages/desktop/packages/session-tools-core/src/handlers/list-sessions.test.ts. I kept the CI command scoped to the list_sessions handler coverage from this PR; the broader session-tools-core suite still passes under WSL/Linux locally, and the latest remote Test (ubuntu-latest, Node 22.x) run is green.

import type { ListSessionsOptions, SessionToolContext } from '../context.ts';

function createCtx(
onListSessions: (options?: ListSessionsOptions) => void,
): SessionToolContext {
return {
listSessions: (options?: ListSessionsOptions) => {
onListSessions(options);
return {
total: 1,
returned: 1,
sessions: [
{
id: 'session-123',
name: 'Example',
labels: [],
status: 'todo',
createdAt: 1,
},
],
};
},
} as unknown as SessionToolContext;
}

describe('handleListSessions', () => {
it('rejects malformed pagination values before listing sessions', async () => {
const cases: Array<{ args: ListSessionsOptions; message: string }> = [
{ args: { limit: 0 }, message: 'limit must be a positive integer.' },
{ args: { limit: -1 }, message: 'limit must be a positive integer.' },
{ args: { limit: 1.5 }, message: 'limit must be a positive integer.' },
Comment on lines +32 to +33

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] Missing boundary tests (limit: 0, limit: 1, offset: 0) for off-by-one regression coverage — Concrete cost: if < 1 were accidentally changed to <= 1, limit: 1 (a natural first-page pagination value) would be incorrectly rejected with no existing test detecting it.

Suggested change
{ args: { limit: -1 }, message: 'limit must be a positive integer.' },
{ args: { limit: 1.5 }, message: 'limit must be a positive integer.' },
{ args: { limit: 0 }, message: 'limit must be a positive integer.' },
{ args: { limit: -1 }, message: 'limit must be a positive integer.' },
{ args: { limit: 1.5 }, message: 'limit must be a positive integer.' },

Also consider adding a valid-boundary test:

it('accepts minimum valid pagination boundaries', async () => {
  const calls: Array<ListSessionsOptions | undefined> = [];
  const ctx = createCtx((options) => calls.push(options));
  const result = await handleListSessions(ctx, { limit: 1, offset: 0 });
  expect(result.isError).toBe(false);
  expect(calls).toEqual([{ limit: 1, offset: 0 }]);
});

— qwen3.7-max via Qwen Code /review

@VectorPeak VectorPeak Jul 19, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks, addressed. I added the invalid limit: 0 case and a valid minimum-boundary test for { limit: 1, offset: 0 }, so the regression suite now covers both sides of the off-by-one boundary. The latest remote Test (ubuntu-latest, Node 22.x) run is green.

{
args: { offset: -1 },
message: 'offset must be a non-negative integer.',
},
{
args: { offset: 1.5 },
message: 'offset must be a non-negative integer.',
},
];

for (const { args, message } of cases) {
const calls: Array<ListSessionsOptions | undefined> = [];
const ctx = createCtx((options) => calls.push(options));

const result = await handleListSessions(ctx, args);

expect(result.isError).toBe(true);
expect(result.content[0]?.text).toContain(message);
expect(calls).toEqual([]);
}
});

it('accepts minimum valid pagination boundaries', async () => {
const calls: Array<ListSessionsOptions | undefined> = [];
const ctx = createCtx((options) => calls.push(options));

const result = await handleListSessions(ctx, { limit: 1, offset: 0 });

expect(result.isError).toBe(false);
expect(calls).toEqual([{ limit: 1, offset: 0 }]);
});

it('passes valid pagination values through to the session lister', async () => {
const calls: Array<ListSessionsOptions | undefined> = [];
const ctx = createCtx((options) => calls.push(options));

const result = await handleListSessions(ctx, {
status: 'todo',
limit: 2,
offset: 1,
});

expect(result.isError).toBe(false);
expect(calls).toEqual([{ status: 'todo', limit: 2, offset: 1 }]);
});

it('preserves high integer limits for the backend clamp', async () => {
const calls: Array<ListSessionsOptions | undefined> = [];
const ctx = createCtx((options) => calls.push(options));

const result = await handleListSessions(ctx, { limit: 101 });

expect(result.isError).toBe(false);
expect(calls).toEqual([{ limit: 101 }]);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,26 @@ export interface ListSessionsArgs {

export async function handleListSessions(
ctx: SessionToolContext,
args: ListSessionsArgs
args: ListSessionsArgs,
): Promise<ToolResult> {
if (!ctx.listSessions) {
return errorResponse('list_sessions is not available in this context.');
}

if (
args.limit !== undefined &&
(!Number.isInteger(args.limit) || args.limit < 1)
) {
return errorResponse('limit must be a positive integer.');
}

if (
args.offset !== undefined &&
(!Number.isInteger(args.offset) || args.offset < 0)
) {
return errorResponse('offset must be a non-negative integer.');
}

try {
const result = ctx.listSessions({
status: args.status,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,19 @@ describe('session tool filtering helpers', () => {
expect(names.includes('send_developer_feedback')).toBe(false);
});

it('json schema exposes list_sessions pagination controls as integers', () => {
const defs = getToolDefsAsJsonSchema({ includeDeveloperFeedback: false });
const listSessions = defs.find(d => d.name === 'list_sessions');
const properties = (
listSessions?.inputSchema as
| { properties?: Record<string, { type?: string }> }
| undefined
)?.properties;

expect(properties?.limit?.type).toBe('integer');
expect(properties?.offset?.type).toBe('integer');
Comment on lines +55 to +56

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] The schema test verifies only type === 'integer' (the .int() half) but not the .min(1) / .min(0) constraints this same diff introduces — Concrete cost: if .min(1)/.min(0) is later dropped from the schema, this test still passes and the schema metadata advertised to MCP clients silently regresses (runtime handling is independently protected by the handler validation, so the impact is bounded to client-side schema display).

Suggested change
expect(properties?.limit?.type).toBe('integer');
expect(properties?.offset?.type).toBe('integer');
expect(properties?.limit?.type).toBe('integer');
expect(properties?.offset?.type).toBe('integer');
expect((properties?.limit as Record<string, unknown>)?.minimum).toBe(1);
expect((properties?.offset as Record<string, unknown>)?.minimum).toBe(0);
中文说明

Schema 测试仅验证了 type === 'integer'(即 .int() 部分),但未断言本 diff 同时引入的 .min(1) / .min(0) 约束。具体代价:如果未来 .min(1)/.min(0) 被移除,此测试仍会通过,MCP 客户端看到的 schema 元数据会静默回归(运行时处理由 handler 层独立保护,因此影响仅限于客户端 schema 展示)。

— qwen3.8-max-preview via Qwen Code /review

});

it('all canonical session tools declare safeMode metadata', () => {
for (const def of SESSION_TOOL_DEFS) {
expect(def.safeMode === 'allow' || def.safeMode === 'block').toBe(true);
Expand Down
4 changes: 2 additions & 2 deletions packages/desktop/packages/session-tools-core/src/tool-defs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -199,8 +199,8 @@ export const ListSessionsSchema = z.object({
label: z.string().optional().describe('Filter by label'),
search: z.string().optional().describe('Substring match on session name'),
sortBy: z.enum(['recent', 'name', 'status']).optional().describe('Sort order (default: recent)'),
limit: z.number().optional().describe('Max sessions to return (default 20, max 100)'),
offset: z.number().optional().describe('Skip first N results (for pagination)'),
limit: z.number().int().min(1).optional().describe('Max sessions to return (default 20, max 100)'),
offset: z.number().int().min(0).optional().describe('Skip first N results (for pagination)'),
});

// Inter-session messaging
Expand Down
Loading