Skip to content

Commit f2b9bd3

Browse files
jshtkclaude
andauthored
fix: respect an explicitly provided Accept header (#656)
* fix: respect an explicitly provided Accept header Header parameters are copied into the request headers, but the acceptType configured in endpoints.json then overwrote the entry unconditionally. An Accept header passed by the caller never reached Graph. This made the alternate representation of a resource unreachable even when Graph explicitly asked the client to retry with a different Accept value. For get-meeting-transcript-content (acceptType text/vtt), a tenant with speaker attribution disabled returns 403 SpeakerAttributionNotAllowed and names application/vnd.microsoft.graph.transcript+text as the format to retry with. Retrying returned the identical 403, because text/vtt was put back in place of the requested format. Treat acceptType as a default instead of an override, so callers that send nothing keep the previous behaviour. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: expose Accept so the acceptType default can be overridden The generated client declares no Accept header on any endpoint, so guarding the configured acceptType against an existing headers['Accept'] was unreachable on its own: nothing could ever put a value there. Add a synthetic, optional Accept param to every tool whose endpoint declares acceptType — the same shape the existing fetchAllPages/timezone synthetic params use — and forward it in executeGraphTool, where it lands in the fallback chain because the generated client has no parameter definition for it. describeToolSchema mirrors the param so --discovery mode does not drift, and the tool-schema tests assert it against the real generated client rather than a mock, so a future regression to dead code fails the suite. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test: cover the Accept override end-to-end against the real generated client The mocked accept-header test originally passed only because the mock invented an Accept header param the generated client never declares. These two tests register the real endpoints and assert both acceptType tools (get-meeting-transcript-content, get-mail-message-mime) apply their configured default and forward an explicit Accept all the way into graphRequest — so a regression back to dead code cannot hide behind a mock again. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent a224f11 commit f2b9bd3

6 files changed

Lines changed: 216 additions & 1 deletion

File tree

src/graph-tools.ts

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@ import {
5252
CONFIRM_PARAM_DESCRIPTION,
5353
TIMEZONE_PARAM_DESCRIPTION,
5454
EXPAND_EXTENDED_PROPERTIES_PARAM_DESCRIPTION,
55+
getAcceptParamDescription,
5556
getAccountParamDescription,
5657
getFetchAllPagesParamDescription,
5758
} from './lib/param-descriptions.js';
@@ -1524,6 +1525,11 @@ async function executeGraphTool(
15241525
.replace(`{${camelCaseParamName}}`, encodedValue)
15251526
.replace(`:${camelCaseParamName}`, encodedValue);
15261527
logger.info(`Path param fallback: replaced :${camelCaseParamName} with encoded value`);
1528+
} else if (paramName.toLowerCase() === 'accept' && config?.acceptType) {
1529+
// The synthetic Accept param added for acceptType endpoints. It has no entry in
1530+
// the generated client's parameter list, so it lands here rather than in the
1531+
// 'Header' case above.
1532+
headers['Accept'] = `${paramValue}`;
15271533
} else if (isOdataParam) {
15281534
// Fallback: OData param recognised by name but absent from generated client's parameter
15291535
// list — forward it as a query param rather than silently dropping it.
@@ -1656,7 +1662,7 @@ async function executeGraphTool(
16561662
logger.info(`Setting custom Content-Type: ${config.contentType}`);
16571663
}
16581664

1659-
if (config?.acceptType) {
1665+
if (config?.acceptType && !headers['Accept']) {
16601666
headers['Accept'] = config.acceptType;
16611667
logger.info(`Setting custom Accept: ${config.acceptType}`);
16621668
}
@@ -2028,6 +2034,17 @@ export function registerGraphTools(
20282034
}
20292035
}
20302036

2037+
// Endpoints with a configured acceptType get a synthetic, optional `Accept` param.
2038+
// The generated client declares no Accept header anywhere, so without this the
2039+
// caller has no way to reach the alternate representation of the resource — the
2040+
// configured default would be the only value the server can ever send.
2041+
if (endpointConfig?.acceptType && paramSchema['Accept'] === undefined) {
2042+
paramSchema['Accept'] = z
2043+
.string()
2044+
.describe(getAcceptParamDescription(endpointConfig.acceptType))
2045+
.optional();
2046+
}
2047+
20312048
if (isFetchAllPagesApplicable(tool)) {
20322049
const maxPages = getMaxPages();
20332050
paramSchema['fetchAllPages'] = z

src/lib/param-descriptions.ts

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,14 @@ export const TIMEZONE_PARAM_DESCRIPTION =
108108
export const EXPAND_EXTENDED_PROPERTIES_PARAM_DESCRIPTION =
109109
'When true, expands singleValueExtendedProperties on each event. Use this to retrieve custom extended properties (e.g., sync metadata) stored on calendar events.';
110110

111+
export function getAcceptParamDescription(acceptType: string): string {
112+
return (
113+
`Accept header for the response representation. Defaults to "${acceptType}". ` +
114+
'Only set this when Graph asks for a different format — e.g. a 403 SpeakerAttributionNotAllowed ' +
115+
'on transcript content names the media type to retry with.'
116+
);
117+
}
118+
111119
/**
112120
* Layer 2 of multi-account support: account names are surfaced in the description
113121
* (not as a strict enum) so the LLM sees available accounts upfront without a

src/lib/tool-schema.ts

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import {
1212
CONFIRM_PARAM_DESCRIPTION,
1313
TIMEZONE_PARAM_DESCRIPTION,
1414
EXPAND_EXTENDED_PROPERTIES_PARAM_DESCRIPTION,
15+
getAcceptParamDescription,
1516
} from './param-descriptions.js';
1617

1718
type ToolEndpoint = (typeof api.endpoints)[number];
@@ -26,6 +27,7 @@ export interface ToolSchemaConfig extends DestructiveCheckConfig {
2627
descriptionOverride?: string;
2728
supportsTimezone?: boolean;
2829
supportsExpandExtendedProperties?: boolean;
30+
acceptType?: string;
2931
}
3032

3133
/**
@@ -167,6 +169,19 @@ export function describeToolSchema(
167169
});
168170
}
169171

172+
// Mirrors registerGraphTools: endpoints with a configured acceptType get a
173+
// synthetic, optional `Accept` header param so the configured default can be
174+
// overridden when Graph asks for a different representation.
175+
if (config?.acceptType && !params.some((p) => p.name.toLowerCase() === 'accept')) {
176+
params.push({
177+
name: 'Accept',
178+
in: 'Header',
179+
required: false,
180+
description: getAcceptParamDescription(config.acceptType),
181+
schema: { type: 'string' },
182+
});
183+
}
184+
170185
const llmTip = config?.llmTip;
171186
return {
172187
name: tool.alias,
Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,102 @@
1+
import { beforeEach, describe, expect, it, vi } from 'vitest';
2+
import { z } from 'zod';
3+
import { registerGraphTools } from '../src/graph-tools.js';
4+
import type { GraphClient } from '../src/graph-client.js';
5+
6+
vi.mock('../src/logger.js', () => ({
7+
default: {
8+
info: vi.fn(),
9+
error: vi.fn(),
10+
warn: vi.fn(),
11+
},
12+
}));
13+
14+
vi.mock('../src/generated/client-beta.js', () => ({ api: { endpoints: [] } }));
15+
vi.mock('../src/generated/client.js', () => ({
16+
api: {
17+
endpoints: [
18+
{
19+
// endpoints.json configures acceptType 'text/vtt' for this tool
20+
alias: 'get-meeting-transcript-content',
21+
method: 'get',
22+
path: '/me/onlineMeetings/:onlineMeetingId/transcripts/:callTranscriptId/content',
23+
description: 'Transcript content.',
24+
parameters: [
25+
{ name: 'onlineMeetingId', type: 'Path', schema: z.string() },
26+
{ name: 'callTranscriptId', type: 'Path', schema: z.string() },
27+
],
28+
},
29+
],
30+
},
31+
}));
32+
33+
describe('explicit Accept header', () => {
34+
let mockServer: { tool: ReturnType<typeof vi.fn>; registerTool: ReturnType<typeof vi.fn> };
35+
let mockGraphClient: GraphClient;
36+
37+
beforeEach(() => {
38+
vi.clearAllMocks();
39+
mockServer = { tool: vi.fn(), registerTool: vi.fn() };
40+
mockGraphClient = {
41+
graphRequest: vi.fn().mockResolvedValue({
42+
content: [{ type: 'text', text: 'WEBVTT' }],
43+
}),
44+
} as unknown as GraphClient;
45+
});
46+
47+
function getRegistration(toolName: string) {
48+
// transcript tools are work-scoped only, so they need org mode to register
49+
registerGraphTools(mockServer, mockGraphClient, false, undefined, true);
50+
const call = mockServer.registerTool.mock.calls.find((c: unknown[]) => c[0] === toolName);
51+
expect(call).toBeDefined();
52+
return call!;
53+
}
54+
55+
function getToolHandler(toolName: string) {
56+
const call = getRegistration(toolName);
57+
return call[call.length - 1] as (params: Record<string, unknown>) => Promise<unknown>;
58+
}
59+
60+
function getParamSchema(toolName: string) {
61+
const call = getRegistration(toolName);
62+
const { inputSchema } = call[1] as { inputSchema: z.ZodObject<z.ZodRawShape> };
63+
return inputSchema.shape;
64+
}
65+
66+
function sentHeaders() {
67+
const call = (mockGraphClient.graphRequest as ReturnType<typeof vi.fn>).mock.calls[0];
68+
return (call[1] as { headers: Record<string, string> }).headers;
69+
}
70+
71+
it('exposes an Accept param for tools with a configured acceptType', () => {
72+
// The generated client declares no Accept header on any endpoint, so without the
73+
// synthetic param the caller has no way to send one and the override is unreachable.
74+
const schema = getParamSchema('get-meeting-transcript-content');
75+
76+
expect(schema['Accept']).toBeDefined();
77+
expect(schema['Accept'].isOptional()).toBe(true);
78+
});
79+
80+
it('falls back to the configured acceptType when the caller sends none', async () => {
81+
const handler = getToolHandler('get-meeting-transcript-content');
82+
83+
await handler({ onlineMeetingId: 'meeting-1', callTranscriptId: 'transcript-1' });
84+
85+
expect(sentHeaders()['Accept']).toBe('text/vtt');
86+
});
87+
88+
it('keeps an explicitly provided Accept header instead of overwriting it', async () => {
89+
const handler = getToolHandler('get-meeting-transcript-content');
90+
91+
// Graph asks for this format when speaker-attributed transcripts are
92+
// disabled for the tenant (403 SpeakerAttributionNotAllowed). Overwriting
93+
// it with the configured acceptType made that retry impossible.
94+
await handler({
95+
onlineMeetingId: 'meeting-1',
96+
callTranscriptId: 'transcript-1',
97+
Accept: 'application/vnd.microsoft.graph.transcript+text',
98+
});
99+
100+
expect(sentHeaders()['Accept']).toBe('application/vnd.microsoft.graph.transcript+text');
101+
});
102+
});
Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
import { describe, expect, it, vi } from 'vitest';
2+
import { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js';
3+
import { registerGraphTools } from '../src/graph-tools.js';
4+
import type { GraphClient } from '../src/graph-client.js';
5+
6+
// End-to-end smoke against the REAL generated client (no endpoint mocks):
7+
// both acceptType endpoints must expose Accept and forward it to graphRequest.
8+
describe('Accept override, real generated client', () => {
9+
function setup() {
10+
const server = new McpServer({ name: 't', version: '1' });
11+
const calls: Array<[string, { headers: Record<string, string> }]> = [];
12+
const graphClient = {
13+
graphRequest: vi.fn(async (path: string, opts: { headers: Record<string, string> }) => {
14+
calls.push([path, opts]);
15+
return { content: [{ type: 'text', text: 'ok' }] };
16+
}),
17+
} as unknown as GraphClient;
18+
const handlers = new Map<string, (p: Record<string, unknown>) => Promise<unknown>>();
19+
vi.spyOn(server, 'registerTool').mockImplementation(((
20+
name: string,
21+
_cfg: unknown,
22+
h: never
23+
) => {
24+
handlers.set(name, h);
25+
// eslint-disable-next-line @typescript-eslint/no-explicit-any -- registerTool() has many overloads
26+
}) as any);
27+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
28+
vi.spyOn(server, 'tool').mockImplementation((() => {}) as any);
29+
registerGraphTools(server, graphClient, false, undefined, true);
30+
return { handlers, calls };
31+
}
32+
33+
it('transcript content: default text/vtt, override survives', async () => {
34+
const { handlers, calls } = setup();
35+
const h = handlers.get('get-meeting-transcript-content')!;
36+
expect(h).toBeDefined();
37+
await h({ onlineMeetingId: 'm1', callTranscriptId: 't1' });
38+
expect(calls[0][1].headers['Accept']).toBe('text/vtt');
39+
await h({
40+
onlineMeetingId: 'm1',
41+
callTranscriptId: 't1',
42+
Accept: 'application/vnd.microsoft.graph.transcript+text',
43+
});
44+
expect(calls[1][1].headers['Accept']).toBe('application/vnd.microsoft.graph.transcript+text');
45+
});
46+
47+
it('mail mime: default text/plain, override survives', async () => {
48+
const { handlers, calls } = setup();
49+
const h = handlers.get('get-mail-message-mime')!;
50+
expect(h).toBeDefined();
51+
await h({ messageId: 'abc' });
52+
expect(calls[0][1].headers['Accept']).toBe('text/plain');
53+
await h({ messageId: 'abc', Accept: 'application/octet-stream' });
54+
expect(calls[1][1].headers['Accept']).toBe('application/octet-stream');
55+
});
56+
});

test/tool-schema.test.ts

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -218,6 +218,23 @@ describe('describeToolSchema parity with registerGraphTools (discovery-mode drif
218218
expect(discovery?.description).toBe(expected);
219219
});
220220

221+
it('matches Accept description exactly for an endpoint with a configured acceptType', () => {
222+
const entry = registry.get('get-meeting-transcript-content');
223+
if (!entry) throw new Error('registry missing get-meeting-transcript-content');
224+
const s = describeToolSchema(entry.tool, entry.config);
225+
const discovery = s.parameters.find((p) => p.name === 'Accept');
226+
const expected = registeredDescription('get-meeting-transcript-content', 'Accept');
227+
expect(expected).toBeDefined();
228+
expect(discovery?.in).toBe('Header');
229+
expect(discovery?.description).toBe(expected);
230+
});
231+
232+
it('adds no Accept param to endpoints without a configured acceptType', () => {
233+
const s = schemaFor('list-mail-messages');
234+
expect(s.parameters.find((p) => p.name === 'Accept')).toBeUndefined();
235+
expect(registered.get('list-mail-messages')).not.toHaveProperty('Accept');
236+
});
237+
221238
it('matches confirm description exactly (already shared logic, guarded against future drift)', () => {
222239
const entry = registry.get('delete-mail-message');
223240
if (!entry) throw new Error('registry missing delete-mail-message');

0 commit comments

Comments
 (0)