Skip to content

Commit 3899fa7

Browse files
jirispilkaclaudeCopilot
authored
fix: Fill unit-test gaps for MCP task functionality (#688)
* test: Fill unit-test gaps for MCP task functionality Task behavior is covered end-to-end at the integration level but the small pure helpers were uncovered at the unit level. Adds: - isTaskCancelled (src/mcp/utils.ts): return values for each task status and not-found case, plus that taskId/mcpSessionId propagate to getTask. - ProgressTracker: related-task _meta is included when taskId is supplied and absent otherwise. - createProgressTracker factory: null/instance branches for each combination of progressToken, sendNotification, and onStatusMessage. - taskSupport contract across tool categories: only default-mode call-actor declares execution.taskSupport, value is in ALLOWED_TASK_TOOL_EXECUTION_MODES, and no openai-mode tool declares it. https://claude.ai/code/session_01SBaWhEW1vE35bTTivv1JNs * refactor: Extract RELATED_TASK_META_KEY constant; trim createProgressTracker tests Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: handle progressToken=0 in progress tracking Agent-Logs-Url: https://github.com/apify/apify-mcp-server/sessions/aed4b4c5-420c-469d-a96d-0bf1cf8915b1 Co-authored-by: jirispilka <19406805+jirispilka@users.noreply.github.com> --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
1 parent 0966a03 commit 3899fa7

6 files changed

Lines changed: 149 additions & 8 deletions

File tree

src/const.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -208,3 +208,7 @@ export const HTTP_NOT_FOUND = 404;
208208

209209
// Modes that allow long running task tool executions
210210
export const ALLOWED_TASK_TOOL_EXECUTION_MODES = ['optional', 'required'] as const;
211+
212+
// MCP _meta key for associating messages with a task.
213+
// TODO: replace with RELATED_TASK_META_KEY from @modelcontextprotocol/sdk once the installed SDK exports it.
214+
export const RELATED_TASK_META_KEY = 'io.modelcontextprotocol/related-task';

src/mcp/server.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -871,7 +871,7 @@ export class ActorsMcpServer {
871871
}
872872

873873
// Only set up notification handlers if progressToken is provided by the client
874-
if (progressToken) {
874+
if (progressToken !== undefined && progressToken !== null) {
875875
// Set up notification handlers for the client
876876
for (const schema of ServerNotificationSchema.options) {
877877
const method = schema.shape.method.value;

src/utils/progress.ts

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import type { ProgressNotification } from '@modelcontextprotocol/sdk/types.js';
22

33
import type { ApifyClient } from '../apify_client.js';
4-
import { PROGRESS_NOTIFICATION_INTERVAL_MS } from '../const.js';
4+
import { PROGRESS_NOTIFICATION_INTERVAL_MS, RELATED_TASK_META_KEY } from '../const.js';
55

66
export class ProgressTracker {
77
private progressToken?: string | number;
@@ -27,7 +27,7 @@ export class ProgressTracker {
2727
this.currentProgress += 1;
2828

2929
// Send progress notification only if progressToken and sendNotification are available
30-
if (this.progressToken && this.sendNotification) {
30+
if (this.progressToken !== undefined && this.progressToken !== null && this.sendNotification) {
3131
try {
3232
const notification: ProgressNotification = {
3333
method: 'notifications/progress' as const,
@@ -39,7 +39,7 @@ export class ProgressTracker {
3939
// Per MCP spec: progress notifications during task execution should include related-task metadata
4040
...(this.taskId && {
4141
_meta: {
42-
'io.modelcontextprotocol/related-task': {
42+
[RELATED_TASK_META_KEY]: {
4343
taskId: this.taskId,
4444
},
4545
},
@@ -111,7 +111,8 @@ export function createProgressTracker(
111111
onStatusMessage?: (message: string) => Promise<void>,
112112
): ProgressTracker | null {
113113
// Create tracker if we have either progress notification support or a status message callback
114-
if ((!progressToken || !sendNotification) && !onStatusMessage) {
114+
const hasProgressNotificationSupport = progressToken !== undefined && progressToken !== null && !!sendNotification;
115+
if (!hasProgressNotificationSupport && !onStatusMessage) {
115116
return null;
116117
}
117118

tests/unit/mcp.utils.test.ts

Lines changed: 36 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,8 @@
1+
import type { TaskStore } from '@modelcontextprotocol/sdk/experimental/tasks/interfaces.js';
12
import { describe, expect, it, vi } from 'vitest';
23

34
import { SKYFIRE_README_CONTENT } from '../../src/const.js';
4-
import { parseInputParamsFromUrl } from '../../src/mcp/utils.js';
5+
import { isTaskCancelled, parseInputParamsFromUrl } from '../../src/mcp/utils.js';
56
import { resolvePaymentProvider } from '../../src/payments/index.js';
67
import { createResourceService } from '../../src/resources/resource_service.js';
78
import type { AvailableWidget } from '../../src/resources/widgets.js';
@@ -61,6 +62,40 @@ describe('parseInputParamsFromUrl', () => {
6162
});
6263
});
6364

65+
describe('isTaskCancelled', () => {
66+
const makeTaskStore = (getTaskReturn: unknown) => ({
67+
getTask: vi.fn().mockResolvedValue(getTaskReturn),
68+
} as unknown as TaskStore);
69+
70+
it('should return true when task status is cancelled', async () => {
71+
const taskStore = makeTaskStore({ status: 'cancelled' });
72+
const result = await isTaskCancelled('task-1', 'session-1', taskStore);
73+
74+
expect(result).toBe(true);
75+
});
76+
77+
it('should return false when task status is not cancelled', async () => {
78+
const taskStore = makeTaskStore({ status: 'working' });
79+
const result = await isTaskCancelled('task-1', 'session-1', taskStore);
80+
81+
expect(result).toBe(false);
82+
});
83+
84+
it('should return false when task is not found (getTask returns undefined)', async () => {
85+
const taskStore = makeTaskStore(undefined);
86+
const result = await isTaskCancelled('task-1', 'session-1', taskStore);
87+
88+
expect(result).toBe(false);
89+
});
90+
91+
it('should pass taskId and mcpSessionId through to taskStore.getTask', async () => {
92+
const taskStore = makeTaskStore({ status: 'working' });
93+
await isTaskCancelled('task-42', 'session-xyz', taskStore);
94+
95+
expect(taskStore.getTask).toHaveBeenCalledWith('task-42', 'session-xyz');
96+
});
97+
});
98+
6499
describe('MCP resources', () => {
65100
const buildAvailableWidget = (uri: string, exists: boolean): AvailableWidget => ({
66101
...WIDGET_REGISTRY[uri],

tests/unit/tools.mode_contract.test.ts

Lines changed: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99
*/
1010
import { describe, expect, it } from 'vitest';
1111

12-
import { HelperTools } from '../../src/const.js';
12+
import { ALLOWED_TASK_TOOL_EXECUTION_MODES, HelperTools } from '../../src/const.js';
1313
import { searchApifyDocsTool } from '../../src/tools/common/search_apify_docs.js';
1414
import { CATEGORY_NAMES, getCategoryTools } from '../../src/tools/index.js';
1515
import type { ToolBase, ToolEntry } from '../../src/types.js';
@@ -174,6 +174,40 @@ describe('getCategoryTools mode contract (tool-mode separation)', () => {
174174
});
175175
});
176176

177+
describe('taskSupport contract across tool categories', () => {
178+
it('should declare taskSupport only on call-actor in default mode, with an allowed value', () => {
179+
const defaultCategories = getCategoryTools('default');
180+
const toolsWithTaskSupport: { name: string; value: unknown }[] = [];
181+
182+
for (const categoryName of CATEGORY_NAMES) {
183+
for (const tool of defaultCategories[categoryName]) {
184+
if (tool.execution?.taskSupport !== undefined) {
185+
toolsWithTaskSupport.push({ name: tool.name, value: tool.execution.taskSupport });
186+
}
187+
}
188+
}
189+
190+
// Only default-mode call-actor is expected to declare taskSupport among static internal tools.
191+
// (Dynamically-created Actor tools from actor_tools_factory also declare it, but those are not
192+
// returned by getCategoryTools.)
193+
expect(toolsWithTaskSupport.map((t) => t.name)).toEqual([HelperTools.ACTOR_CALL]);
194+
195+
for (const { value } of toolsWithTaskSupport) {
196+
expect(ALLOWED_TASK_TOOL_EXECUTION_MODES).toContain(value);
197+
}
198+
});
199+
200+
it('should not declare taskSupport on any tool in openai mode', () => {
201+
const openaiCategories = getCategoryTools('openai');
202+
203+
for (const categoryName of CATEGORY_NAMES) {
204+
for (const tool of openaiCategories[categoryName]) {
205+
expect(tool.execution?.taskSupport).toBeUndefined();
206+
}
207+
}
208+
});
209+
});
210+
177211
describe('getToolPublicFieldOnly _meta filtering', () => {
178212
const toolWithOpenAiMeta = {
179213
name: 'test-tool',

tests/unit/utils.progress.test.ts

Lines changed: 68 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { describe, expect, it, vi } from 'vitest';
22

3-
import { ProgressTracker } from '../../src/utils/progress.js';
3+
import { RELATED_TASK_META_KEY } from '../../src/const.js';
4+
import { createProgressTracker, ProgressTracker } from '../../src/utils/progress.js';
45

56
describe('ProgressTracker', () => {
67
it('should send progress notifications correctly', async () => {
@@ -83,4 +84,70 @@ describe('ProgressTracker', () => {
8384
await expect(tracker.updateProgress('Test')).resolves.toBeUndefined();
8485
expect(mockOnStatusMessage).toHaveBeenCalledWith('Test');
8586
});
87+
88+
it('should include related-task metadata with taskId in progress notifications', async () => {
89+
const mockSendNotification = vi.fn();
90+
const tracker = new ProgressTracker({
91+
progressToken: 'tok',
92+
sendNotification: mockSendNotification,
93+
taskId: 'task-abc',
94+
});
95+
96+
await tracker.updateProgress('running');
97+
98+
expect(mockSendNotification).toHaveBeenCalledWith({
99+
method: 'notifications/progress',
100+
params: {
101+
progressToken: 'tok',
102+
progress: 1,
103+
message: 'running',
104+
},
105+
_meta: {
106+
[RELATED_TASK_META_KEY]: {
107+
taskId: 'task-abc',
108+
},
109+
},
110+
});
111+
});
112+
113+
it('should not include _meta when taskId is not provided', async () => {
114+
const mockSendNotification = vi.fn();
115+
const tracker = new ProgressTracker({
116+
progressToken: 'tok',
117+
sendNotification: mockSendNotification,
118+
});
119+
120+
await tracker.updateProgress('running');
121+
122+
const notification = mockSendNotification.mock.calls[0][0];
123+
expect(notification).not.toHaveProperty('_meta');
124+
});
125+
});
126+
127+
describe('createProgressTracker', () => {
128+
it('should return null when no progressToken, no sendNotification, and no onStatusMessage', () => {
129+
expect(createProgressTracker(undefined, undefined)).toBeNull();
130+
});
131+
132+
it('should return ProgressTracker when only onStatusMessage is provided', () => {
133+
const tracker = createProgressTracker(undefined, undefined, undefined, vi.fn());
134+
expect(tracker).toBeInstanceOf(ProgressTracker);
135+
});
136+
137+
it('should return ProgressTracker and send notifications for progressToken = 0', async () => {
138+
const mockSendNotification = vi.fn();
139+
const tracker = createProgressTracker(0, mockSendNotification);
140+
141+
expect(tracker).toBeInstanceOf(ProgressTracker);
142+
await tracker?.updateProgress('Started');
143+
144+
expect(mockSendNotification).toHaveBeenCalledWith({
145+
method: 'notifications/progress',
146+
params: {
147+
progressToken: 0,
148+
progress: 1,
149+
message: 'Started',
150+
},
151+
});
152+
});
86153
});

0 commit comments

Comments
 (0)