Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
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
5 changes: 5 additions & 0 deletions .changeset/model-capture-option.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@posthog/mcp': minor
---

Add opt-in `captureModel` option: injects a required `llm_model` parameter into every tool so the calling agent self-reports the model it runs as, captured as `$mcp_llm_model` with `$mcp_llm_model_source = "self_reported"`. The argument is stripped before the tool handler runs, a customer-declared `llm_model` parameter is never stolen or captured, and an honest `"unknown"` from the agent is dropped rather than recorded.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Document the ownership limit for stateless server instances

should_fix

Why we think it's a valid issue
  • Checked: the ownership resolution path for llm_modelgetActiveAnalyticsParameterOwnership in packages/mcp/src/extensions/instrumentation.ts, the strip helper stripOwnedAnalyticsArguments in packages/mcp/src/extensions/analytics-parameters.ts, the capture call site, and the README section that already documents the same limit for context.
  • Found: packages/mcp/src/extensions/instrumentation.ts:183-189 resolves ownership as override ?? listed, where listed comes from data.toolAnalyticsParameterOwnership — a map that only tools/list fills (cacheToolAnalyticsParameterOwnership, same file, line 659). When neither source answers, llmModel is false.
  • Found: both effects the release note promises hang on that flag. packages/mcp/src/extensions/instrumentation.ts:278-279 calls setEventModel only if (ownership.llmModel), and stripOwnedAnalyticsArguments removes llm_model only under the same flag. So the premise holds: with ownership unresolved, the SDK does not capture the model and does not strip the argument.
  • Found: the code comment at packages/mcp/src/extensions/instrumentation.ts:274-277 states this choice is deliberate, and contrasts it with intent. Intent still captures when ownership is unresolved (ownership.context || !ownership.contextOwnershipKnown, line 126). The two options therefore degrade differently, and only the context degradation is documented (packages/mcp/README.md:125-133).
  • Found: this PR touches no documentation except the changeset. git diff lists 12 files and packages/mcp/README.md is not among them. A repo-wide grep finds captureModel only in the changeset and in the types.ts JSDoc, and neither text mentions the ownership condition.
  • Impact: on a host where ownership never resolves, an operator who enables captureModel pays the extra token on every tool call and records no $mcp_llm_model at all. The failure is silent and fail-safe — no wrong value, no stolen parameter — but nothing tells the operator why the property is empty.
  • Priority: lowered to consider. Three facts reduce the weight. First, the target file is a changeset, which is a one-paragraph release note; caveats of this depth normally belong in the README, and the finding's own suggestion points there. Second, the high-level path already resolves ownership per request from the live registry — packages/mcp/src/extensions/instrument-highlevel.ts:267-269 passes parameterOwnership built from _registeredTools[toolName] — so instrument(McpServer) strips and captures even on a fresh instance, and the affected surface is narrower than the finding states. Third, nothing breaks: the consequence is missing data, not wrong data or a failed call.
Issue description

The release note says the SDK strips and captures llm_model. This is not always true. A new server instance can handle tools/call without the ownership data from tools/list. The implementation then keeps the argument and does not capture the model. The current text can give operators false coverage expectations.

Suggested fix

State that stripping and capture require confirmed SDK ownership. Document the per-request instance limit, as the README already does for context. Explain which server configurations preserve ownership between tools/list and tools/call.

Prompt to fix with AI (copy-paste)
## Context
@.changeset/model-capture-option.md#L5

<issue_description>
The release note says the SDK strips and captures `llm_model`. This is not always true. A new server instance can handle `tools/call` without the ownership data from `tools/list`. The implementation then keeps the argument and does not capture the model. The current text can give operators false coverage expectations.
</issue_description>

<issue_validation>
- **Checked:** the ownership resolution path for `llm_model` — `getActiveAnalyticsParameterOwnership` in `packages/mcp/src/extensions/instrumentation.ts`, the strip helper `stripOwnedAnalyticsArguments` in `packages/mcp/src/extensions/analytics-parameters.ts`, the capture call site, and the README section that already documents the same limit for `context`.
- **Found:** `packages/mcp/src/extensions/instrumentation.ts:183-189` resolves ownership as `override ?? listed`, where `listed` comes from `data.toolAnalyticsParameterOwnership` — a map that only `tools/list` fills (`cacheToolAnalyticsParameterOwnership`, same file, line 659). When neither source answers, `llmModel` is `false`.
- **Found:** both effects the release note promises hang on that flag. `packages/mcp/src/extensions/instrumentation.ts:278-279` calls `setEventModel` only `if (ownership.llmModel)`, and `stripOwnedAnalyticsArguments` removes `llm_model` only under the same flag. So the premise holds: with ownership unresolved, the SDK does not capture the model and does not strip the argument.
- **Found:** the code comment at `packages/mcp/src/extensions/instrumentation.ts:274-277` states this choice is deliberate, and contrasts it with intent. Intent still captures when ownership is unresolved (`ownership.context || !ownership.contextOwnershipKnown`, line 126). The two options therefore degrade differently, and only the `context` degradation is documented (`packages/mcp/README.md:125-133`).
- **Found:** this PR touches no documentation except the changeset. `git diff` lists 12 files and `packages/mcp/README.md` is not among them. A repo-wide grep finds `captureModel` only in the changeset and in the `types.ts` JSDoc, and neither text mentions the ownership condition.
- **Impact:** on a host where ownership never resolves, an operator who enables `captureModel` pays the extra token on every tool call and records no `$mcp_llm_model` at all. The failure is silent and fail-safe — no wrong value, no stolen parameter — but nothing tells the operator why the property is empty.
- **Priority:** lowered to `consider`. Three facts reduce the weight. First, the target file is a changeset, which is a one-paragraph release note; caveats of this depth normally belong in the README, and the finding's own suggestion points there. Second, the high-level path already resolves ownership per request from the live registry — `packages/mcp/src/extensions/instrument-highlevel.ts:267-269` passes `parameterOwnership` built from `_registeredTools[toolName]` — so `instrument(McpServer)` strips and captures even on a fresh instance, and the affected surface is narrower than the finding states. Third, nothing breaks: the consequence is missing data, not wrong data or a failed call.
</issue_validation>

## Task
Investigate the issue and solve it

<potential_solution>
State that stripping and capture require confirmed SDK ownership. Document the per-request instance limit, as the README already does for `context`. Explain which server configurations preserve ownership between `tools/list` and `tools/call`.
</potential_solution>

311 changes: 311 additions & 0 deletions packages/mcp/src/__tests__/model-parameters.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,311 @@
import { Client } from '@modelcontextprotocol/sdk/client/index.js'
import { InMemoryTransport } from '@modelcontextprotocol/sdk/inMemory.js'
import { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js'
import { CallToolResultSchema, ListToolsResultSchema } from '@modelcontextprotocol/sdk/types.js'
import { z } from 'zod'
import { instrument } from '../index'
import { DEFAULT_MODEL_PARAMETER_DESCRIPTION } from '../extensions/constants'
import { addModelParameterToTool, addModelParameterToTools } from '../extensions/model-parameters'
import { log } from '../extensions/logger'
import { EventCapture, fakePostHog } from './test-utils'
import { resetTodos, setupTestServerAndClient } from './test-utils/client-server-factory'

jest.mock('../extensions/logger', () => ({
createLogger: (logger?: (message: string) => void) => logger ?? (() => undefined),
log: jest.fn(),
setLogger: jest.fn(),
}))

const mockedLog = jest.mocked(log)

beforeEach(() => {
mockedLog.mockClear()
})

afterEach(() => {
jest.restoreAllMocks()
})

/**
* --- Unit tests: `addModelParameterToTool` / `addModelParameterToTools` ---
*
* Same pure-function contract as the `context` injection: mutate a JSON-Schema
* tool descriptor to add a required `llm_model` string parameter.
*/
describe('addModelParameterToTool', () => {
it.each([
['no inputSchema', { name: 'tool' }],
['empty inputSchema {}', { name: 'tool', inputSchema: {} }],
[
'inputSchema with existing properties + required',
{
name: 'tool',
inputSchema: {
type: 'object',
properties: { text: { type: 'string' } },
required: ['text'],
},
},
],
])('injects a string llm_model param + marks it required (%s)', (_, tool) => {
const result = addModelParameterToTool(tool as Parameters<typeof addModelParameterToTool>[0])

expect(result.inputSchema?.properties?.llm_model).toEqual({
type: 'string',
description: DEFAULT_MODEL_PARAMETER_DESCRIPTION,
})
expect(result.inputSchema?.required).toContain('llm_model')
})

it('preserves existing required fields when adding llm_model', () => {
const result = addModelParameterToTool({
name: 'tool',
inputSchema: { type: 'object', properties: { text: { type: 'string' } }, required: ['text'] },
})

expect(result.inputSchema?.required).toEqual(expect.arrayContaining(['text', 'llm_model']))
})

it('removes additionalProperties:false (would otherwise reject the injected param)', () => {
const result = addModelParameterToTool({
name: 'strict-tool',
inputSchema: {
type: 'object',
properties: { name: { type: 'string' } },
additionalProperties: false,
},
})

expect(result.inputSchema?.properties?.llm_model).toBeDefined()
expect(result.inputSchema?.additionalProperties).toBeUndefined()
})

it.each([
[
'tool already has an llm_model property',
{
name: 'has-model',
inputSchema: { type: 'object', properties: { llm_model: { type: 'number', description: 'existing' } } },
},
"already has 'llm_model' parameter",
],
['schema uses a root $ref', { name: 'referenced-tool', inputSchema: { $ref: '#/$defs/Input' } }, 'complex schema'],
[
'schema uses oneOf',
{ name: 'union-tool', inputSchema: { oneOf: [{ type: 'object', properties: {} }] } },
'complex schema',
],
])('skips + warns when %s', (_, tool, expectedWarningSubstring) => {
const before = JSON.parse(JSON.stringify(tool))
const result = addModelParameterToTool(tool as Parameters<typeof addModelParameterToTool>[0])

expect(result.inputSchema).toEqual(before.inputSchema)
expect(mockedLog).toHaveBeenCalledWith(expect.stringContaining(expectedWarningSubstring))
})

it('uses a custom description when provided', () => {
const customDescription = 'State your model identifier'
const result = addModelParameterToTool({ name: 'tool' }, customDescription)

expect(result.inputSchema?.properties?.llm_model?.description).toBe(customDescription)
})
})

describe('addModelParameterToTools (batch)', () => {
it('applies the right outcome per tool in a mixed batch', () => {
const result = addModelParameterToTools([
{ name: 'plain', inputSchema: { type: 'object', properties: {} } },
{ name: 'complex', inputSchema: { oneOf: [{ type: 'string' }] } },
{ name: 'collision', inputSchema: { type: 'object', properties: { llm_model: { type: 'number' } } } },
])

expect(result[0].inputSchema?.properties?.llm_model).toBeDefined()
expect(result[1].inputSchema?.properties).toBeUndefined()
expect(result[2].inputSchema?.properties?.llm_model?.type).toBe('number')
expect(mockedLog).toHaveBeenCalledTimes(2)
})
})

/**
* --- Integration tests: captureModel against a real MCP server ---
*/
describe('Model capture — integration with an instrumented server', () => {
let server: any
let client: any
let cleanup: () => Promise<void>

beforeEach(async () => {
resetTodos()
const setup = await setupTestServerAndClient()
server = setup.server
client = setup.client
cleanup = setup.cleanup
})

afterEach(async () => {
await cleanup()
})

it('does not inject llm_model when captureModel is off (default)', async () => {
instrument(server, fakePostHog(), {})

const toolsResponse = await client.request({ method: 'tools/list', params: {} }, ListToolsResultSchema)
for (const tool of toolsResponse.tools) {
expect(tool.inputSchema.properties?.llm_model).toBeUndefined()
}
})

it.each([
['default', true, DEFAULT_MODEL_PARAMETER_DESCRIPTION],
['custom', { description: 'Which model are you?' }, 'Which model are you?'],
])('injects the llm_model parameter on every tool in tools/list (%s description)', async (_, option, expected) => {
instrument(server, fakePostHog(), { captureModel: option as any })

const toolsResponse = await client.request({ method: 'tools/list', params: {} }, ListToolsResultSchema)
const userTools = toolsResponse.tools.filter((t: any) =>
['add_todo', 'list_todos', 'complete_todo'].includes(t.name)
)

expect(userTools).toHaveLength(3)
for (const tool of userTools) {
expect(tool.inputSchema.properties.llm_model).toBeDefined()
expect(tool.inputSchema.properties.llm_model.type).toBe('string')
expect(tool.inputSchema.properties.llm_model.description).toBe(expected)
}
})

it('captures llm_model as $mcp_llm_model with source=self_reported and strips it from the tool args', async () => {
const capture = new EventCapture()
await capture.start()
try {
instrument(server, fakePostHog(), { captureModel: true })

// Prime ownership the way a real client does: list tools first.
await client.request({ method: 'tools/list', params: {} }, ListToolsResultSchema)

const result = await client.request(
{
method: 'tools/call',
params: {
name: 'add_todo',
arguments: { text: 'buy milk', llm_model: 'claude-opus-4-8' },
},
},
CallToolResultSchema
)

// The tool ran normally — the injected argument never reached the handler.
expect(result.content[0].text).toContain('buy milk')

const toolCalls = capture.findCapturesByEvent('$mcp_tool_call')
expect(toolCalls).toHaveLength(1)
expect(toolCalls[0].properties.$mcp_llm_model).toBe('claude-opus-4-8')
expect(toolCalls[0].properties.$mcp_llm_model_source).toBe('self_reported')
// Stripped from the captured parameters too — it is analytics metadata, not an argument.
expect((toolCalls[0].properties.$mcp_parameters as any)?.llm_model).toBeUndefined()
} finally {
await capture.stop()
}
})

it('omits the property when the agent passes "unknown"', async () => {
const capture = new EventCapture()
await capture.start()
try {
instrument(server, fakePostHog(), { captureModel: true })
await client.request({ method: 'tools/list', params: {} }, ListToolsResultSchema)

await client.request(
{
method: 'tools/call',
params: { name: 'add_todo', arguments: { text: 'x', llm_model: 'unknown' } },
},
CallToolResultSchema
)

const toolCalls = capture.findCapturesByEvent('$mcp_tool_call')
expect(toolCalls).toHaveLength(1)
expect(toolCalls[0].properties.$mcp_llm_model).toBeUndefined()
expect(toolCalls[0].properties.$mcp_llm_model_source).toBeUndefined()
} finally {
await capture.stop()
}
})

it('does not capture or strip llm_model when captureModel is off', async () => {
const capture = new EventCapture()
await capture.start()
try {
instrument(server, fakePostHog(), {})
await client.request({ method: 'tools/list', params: {} }, ListToolsResultSchema)

await client.request(
{
method: 'tools/call',
params: { name: 'add_todo', arguments: { text: 'x' } },
},
CallToolResultSchema
)

const toolCalls = capture.findCapturesByEvent('$mcp_tool_call')
expect(toolCalls).toHaveLength(1)
expect(toolCalls[0].properties.$mcp_llm_model).toBeUndefined()
} finally {
await capture.stop()
}
})
})

/**
* --- Ownership: a customer-declared llm_model parameter is never stolen ---
*/
describe('llm_model ownership', () => {
it('passes a customer-declared llm_model argument through to the callback and does not capture it', async () => {
const server = new McpServer({ name: 'model-ownership', version: '1.0.0' })
let receivedArgs: Record<string, unknown> | undefined

server.registerTool(
'pick_model',
{
inputSchema: z.object({
llm_model: z.string(),
prompt: z.string(),
}),
},
async (args) => {
receivedArgs = { ...args }
return { content: [{ type: 'text', text: 'ok' }] }
}
)

const client = new Client({ name: 'model-ownership-client', version: '1.0.0' })
const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair()
await Promise.all([client.connect(clientTransport), server.server.connect(serverTransport)])

const capture = new EventCapture()
await capture.start()
try {
instrument(server, fakePostHog(), { captureModel: true })
await client.request({ method: 'tools/list', params: {} }, ListToolsResultSchema)

await client.request(
{
method: 'tools/call',
params: { name: 'pick_model', arguments: { llm_model: 'gpt-5', prompt: 'hi' } },
},
CallToolResultSchema
)

// The application owns this parameter: it must reach the handler untouched...
expect(receivedArgs).toEqual({ llm_model: 'gpt-5', prompt: 'hi' })
// ...and must not be recorded as the calling agent's model.
const toolCalls = capture.findCapturesByEvent('$mcp_tool_call')
expect(toolCalls).toHaveLength(1)
expect(toolCalls[0].properties.$mcp_llm_model).toBeUndefined()
} finally {
await capture.stop()
await clientTransport.close?.()
await serverTransport.close?.()
}
})
})
9 changes: 7 additions & 2 deletions packages/mcp/src/extensions/analytics-parameters.ts
Original file line number Diff line number Diff line change
Expand Up @@ -76,18 +76,19 @@ export function getAnalyticsParameterOwnership(
return {
context: analyticsOwnsParameter(inputSchema, 'context'),
conversationId: analyticsOwnsParameter(inputSchema, 'conversation_id'),
llmModel: analyticsOwnsParameter(inputSchema, 'llm_model'),
outputInstructions: canDeclareOutputInstructions(outputSchema),
}
}

/**
* Removes the arguments we injected, leaving the host's own. Input-side only, so
* it takes just those two flags rather than the whole ownership record — callers
* it takes just those flags rather than the whole ownership record — callers
* that have no output-schema context should not have to invent a value for it.
*/
export function stripOwnedAnalyticsArguments(
args: unknown,
ownership: Pick<AnalyticsParameterOwnership, 'context' | 'conversationId'>
ownership: Pick<AnalyticsParameterOwnership, 'context' | 'conversationId' | 'llmModel'>
): unknown {
let cleanedArgs = args
if (ownership.context && cleanedArgs && typeof cleanedArgs === 'object' && 'context' in cleanedArgs) {
Expand All @@ -98,5 +99,9 @@ export function stripOwnedAnalyticsArguments(
const { conversation_id: _conversationId, ...rest } = cleanedArgs
cleanedArgs = rest
}
if (ownership.llmModel && cleanedArgs && typeof cleanedArgs === 'object' && 'llm_model' in cleanedArgs) {
const { llm_model: _llmModel, ...rest } = cleanedArgs
cleanedArgs = rest
}
return cleanedArgs
}
4 changes: 4 additions & 0 deletions packages/mcp/src/extensions/capture.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,10 @@ export function captureEvent(
response: eventInput.response,
userIntent: eventInput.userIntent,
userIntentSource: eventInput.userIntentSource,
// Self-reported per request (`captureModel`), so like the header-borne
// fields above there is deliberately no sessionInfo fallback.
llmModel: eventInput.llmModel,
llmModelSource: eventInput.llmModelSource,
isError: eventInput.isError,
// Two consumers of the same failure: `error` is the full thrown value that
// fans out into the separate `$exception` event (stacktrace, error-tracking
Expand Down
4 changes: 4 additions & 0 deletions packages/mcp/src/extensions/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ export const INACTIVITY_TIMEOUT_IN_MINUTES = 30

export const DEFAULT_CONTEXT_PARAMETER_DESCRIPTION = `Explain why you are calling this tool and how it fits into the user's overall goal. This parameter is used for analytics and user intent tracking. YOU MUST provide 15-25 words (count carefully). NEVER use first person ('I', 'we', 'you') - maintain third-person perspective. NEVER include sensitive information such as credentials, passwords, or personal data. Example (20 words): "Searching across the organization's repositories to find all open issues related to performance complaints and latency issues for team prioritization."`

export const DEFAULT_MODEL_PARAMETER_DESCRIPTION = `The exact model identifier you (the assistant) are running as, taken from your system prompt or environment (e.g. "claude-opus-4-8", "gpt-5.2"). Used for analytics only. If you do not know your model identifier with certainty, pass "unknown" — never guess.`

export const DEFAULT_CONVERSATION_ID_DESCRIPTION =
"Echo the conversation_id from the server's previous response. The server provides it on the first call — never invent one, and do not issue parallel tool calls until you have it."

Expand Down Expand Up @@ -48,6 +50,8 @@ export const PostHogMCPAnalyticsProperty = {
Intent: '$mcp_intent',
IntentSource: '$mcp_intent_source',
ListedToolNames: '$mcp_listed_tool_names',
LlmModel: '$mcp_llm_model',
LlmModelSource: '$mcp_llm_model_source',
Parameters: '$mcp_parameters',
ProtocolVersion: '$mcp_protocol_version',
ResourceName: '$mcp_resource_name',
Expand Down
2 changes: 2 additions & 0 deletions packages/mcp/src/extensions/instrument-highlevel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
stripOwnedAnalyticsArguments,
} from './analytics-parameters'
import { isContextEnabled } from './context-parameters'
import { isCaptureModelEnabled } from './model-parameters'
import { MCPAnalyticsEventType } from './event-types'
import { getServerTrackingData } from './internal'
import type { LoggerFn } from './logger'
Expand Down Expand Up @@ -189,6 +190,7 @@ function addTracingToToolCallbackInternal(
const cleanedArgs = stripOwnedAnalyticsArguments(args, {
context: isContextEnabled(options?.context) && analyticsOwnsParameter(inputSchema, 'context'),
conversationId: options?.enableConversationId === true && analyticsOwnsParameter(inputSchema, 'conversation_id'),
llmModel: isCaptureModelEnabled(options?.captureModel) && analyticsOwnsParameter(inputSchema, 'llm_model'),

@dustinbyrne dustinbyrne Aug 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Question: Does instrument(McpServer) support applications that create a new high-level server instance for each request?

The README appears to say that it does. Conceptually, an application can use this pattern:

async function handleRequest(request) {
  const server = createAndInstrumentServer()
  return handleWithServer(server, request)
}

A tools/list request creates Instance A. The later get_more_tools call creates Instance B. The two instances do not share the ownership map.

The virtual-tool branch returns before the registry-based parameterOwnership logic. Also, isToolAdvertised() does not fill the ownership cache. This appears to make ownership.llmModel false on Instance B. The SDK would then neither capture nor remove llm_model.

If this lifecycle is supported, should this branch pass ownership from the virtual-tool descriptor? A test with separate list and call instances could verify the behavior.

This was the only potential gap found during our agent-assisted human review.

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.

Confirmed, and reproduced with the test you suggested: separate advertising and calling instances, $mcp_llm_model came back undefined. Fixed in 6c10c1b.

That branch only runs when isToolAdvertised() returns false, so the descriptor is the SDK's own getReportMissingToolDescriptor(). Ownership is knowable statically there and we simply weren't consulting it. The fail-closed rule for registered tools exists because an unresolved llm_model may be the application's own argument, which cannot be the case when the application has no tool by that name. So the fix resolves ownership from the descriptor rather than relaxing the policy.

The low-level adapter had the same gap, so both now go through one shared helper. Each adapter has a test with separate list and call instances, and I checked they fail without the fix.

Scoped to llm_model on purpose. conversation_id still resolves from the cache: resolving it statically starts minting handles and appending prompt-back blocks on instances that mint none today, which is a session anchoring change (ADR-0004) rather than this gap, and it broke the existing fresh-instance test. Worth its own PR. context is already forced off for this event type and outputInstructions reads the cache directly, so llm_model is the only value that moves.

Also corrected the README line that prompted the question. It credited the live registry, which never covered the virtual tool.

})
try {
if (cleanedArgs === undefined) {
Expand Down
Loading