feat(mcp): opt-in captureModel — agent-self-reported model id on MCP events - #4633
Open
DanielVisca wants to merge 3 commits into
Open
feat(mcp): opt-in captureModel — agent-self-reported model id on MCP events#4633DanielVisca wants to merge 3 commits into
DanielVisca wants to merge 3 commits into
Conversation
…el id
Injects a required `llm_model` parameter into every tool (mirroring the
`context` -> `$mcp_intent` mechanism) so the calling agent self-reports the
model it runs as, captured as `$mcp_llm_model` with
`$mcp_llm_model_source = "self_reported"`.
The MCP wire deliberately carries no model identity, so agent self-report is
the only capture path: harnesses inject the model id into the agent's system
prompt, and agents restate it accurately. Reasoning effort is intentionally
not collected - agents don't reliably know it.
- Off by default; `captureModel: true` or `{ description }` to enable
- Argument is stripped before the tool handler runs
- A customer-declared `llm_model` parameter is never stolen or captured
- An honest "unknown" from the agent is dropped rather than recorded
Generated-By: PostHog Desktop
Task-Id: efe2cfaa-b580-4392-bdb4-a18eb2cd325b
DanielVisca
marked this pull request as ready for review
August 24, 2026 22:29
Contributor
|
Reviews (1): Last reviewed commit: "feat(mcp): add opt-in captureModel optio..." | Re-trigger Greptile |
Contributor
Contributor
|
Size Change: +11.1 kB (+0.05%) Total Size: 20.2 MB 📦 View Changed
ℹ️ View Unchanged
|
marandaneto
reviewed
Aug 25, 2026
marandaneto
left a comment
marandaneto
left a comment
Member
There was a problem hiding this comment.
Automated advisory code review.
| tools = addContextParameterToTools(tools, getContextDescription(data.options.context), data.logger) | ||
| } | ||
| if (data && isCaptureModelEnabled(data.options.captureModel)) { | ||
| tools = addModelParameterToTools(tools, getModelDescription(data.options.captureModel), data.logger) |
Member
There was a problem hiding this comment.
blocking: Inject the model parameter into the virtual tool — model injection runs before reportMissing appends get_more_tools, so with both options enabled the advertised virtual tool lacks llm_model and its missing-capability calls cannot self-report the model, contrary to the documented every-tool behavior.
| * handling. Skip rules match `addContextParameterToTool`: a tool that already | ||
| * declares `llm_model` owns it, and complex schemas can't safely gain keys. | ||
| */ | ||
| export function addModelParameterToTool<TTool extends ModelInjectableTool>( |
Member
There was a problem hiding this comment.
suggestion: Reuse the existing parameter-injection implementation — this function and its batch wrapper duplicate almost all of addContextParameterToTool; a shared analytics-parameter helper would keep schema ownership, cloning, and required-field behavior from drifting.
Contributor
🦔 ReviewHog reviewed this pull requestFound 0 must fix, 2 should fix, 0 consider. Published 2 findings (view the review). |
Contributor
|
ReviewHog Alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
| '@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. |
Contributor
There was a problem hiding this comment.
Document the ownership limit for stateless server instances
Why we think it's a valid issue
- Checked: the ownership resolution path for
llm_model—getActiveAnalyticsParameterOwnershipinpackages/mcp/src/extensions/instrumentation.ts, the strip helperstripOwnedAnalyticsArgumentsinpackages/mcp/src/extensions/analytics-parameters.ts, the capture call site, and the README section that already documents the same limit forcontext. - Found:
packages/mcp/src/extensions/instrumentation.ts:183-189resolves ownership asoverride ?? listed, wherelistedcomes fromdata.toolAnalyticsParameterOwnership— a map that onlytools/listfills (cacheToolAnalyticsParameterOwnership, same file, line 659). When neither source answers,llmModelisfalse. - Found: both effects the release note promises hang on that flag.
packages/mcp/src/extensions/instrumentation.ts:278-279callssetEventModelonlyif (ownership.llmModel), andstripOwnedAnalyticsArgumentsremovesllm_modelonly 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-277states 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 thecontextdegradation is documented (packages/mcp/README.md:125-133). - Found: this PR touches no documentation except the changeset.
git difflists 12 files andpackages/mcp/README.mdis not among them. A repo-wide grep findscaptureModelonly in the changeset and in thetypes.tsJSDoc, and neither text mentions the ownership condition. - Impact: on a host where ownership never resolves, an operator who enables
captureModelpays the extra token on every tool call and records no$mcp_llm_modelat 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-269passesparameterOwnershipbuilt from_registeredTools[toolName]— soinstrument(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>
Comment on lines
+132
to
+145
| /** | ||
| * Inject a required `llm_model` parameter on 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"`. Off by default. | ||
| * | ||
| * The MCP wire deliberately carries no model identity, so self-report is the | ||
| * only capture path — harnesses inject the model id into the agent's system | ||
| * prompt, and agents restate it accurately. Like `clientInfo` in the MCP | ||
| * spec, the value is unverified: use it for per-model quality analytics, not | ||
| * billing or security. An honest `"unknown"` from the agent is dropped | ||
| * rather than captured. Reasoning effort is intentionally not collected — | ||
| * agents don't reliably know it, so it would only ever be noise. | ||
| */ | ||
| captureModel?: boolean | MCPAnalyticsModelOptions |
Contributor
There was a problem hiding this comment.
Add captureModel to the user documentation and event catalog
Why we think it's a valid issue
- Checked: every documentation artifact in the package —
packages/mcp/README.mdin full,packages/mcp/docs/ARCHITECTURE.mdsection by section, and thedocs/adr/set — against the new public surface the PR adds (captureModelonMCPAnalyticsOptions, and theLlmModel/LlmModelSourcewire keys written inpackages/mcp/src/extensions/posthog-events.ts:173-180). - Found: the finding names the wrong file.
packages/mcp/README.mdholds no options table, no event catalog, and no property catalog. It is 190 lines of deployment gotchas plus a local-development guide, and lines 3-5 state that usage examples live in the official docs athttps://posthog.com/docs/mcp-analytics. So the claim that JavaScript users cannot discover the option "from the main package documentation" misreads what that README is for. - Found: the three artifacts the finding describes do exist, in
packages/mcp/docs/ARCHITECTURE.md— §5 Event catalog (line 99), §6 Property catalog (line 117), §7 Customer extension points (line 163). The PR changes 12 files and none of them is a documentation file, so all three go stale on merge. - Found: those catalogs are actively maintained, not stale boilerplate. §6 opens with "All wire keys live in
PostHogMCPAnalyticsProperty" and then enumerates every one, including the newest additions$mcp_client_user_agent,$mcp_vendor_client, and$mcp_protocol_version. Adding$mcp_llm_modeland$mcp_llm_model_sourcewithout a row makes that opening sentence false. - Found: the closest precedent is already documented in exactly the shape the finding asks for. The
ConversationIdrow in §6 reads "Optional; set whenenableConversationId: trueand the SDK owns the tool's injectedconversation_idparameter", and §7 listsenableConversationIdwith itsfalsedefault.captureModelis the same kind of opt-in, ownership-gated injected parameter and has neither entry. - Impact: confirmed drift in the package's own reference documentation. An analyst who sees
$mcp_llm_modelin PostHog finds no entry for it, and a reader of the options table sees nocaptureModel. The fix is mechanical: one row in §7 and two in §6. - Priority: lowered to
consider. The verified impact is internal reference drift with no behavioral consequence, and the user-facing half of the stated impact does not hold — the package README deliberately defers the user manual to posthog.com, which lives outside this repo. Anyone acting on this should editpackages/mcp/docs/ARCHITECTURE.md, not the README.
Issue description
The new public option appears only in TypeScript API comments and the changeset. The README has an options table, an event catalog, and a property catalog. None describe captureModel, $mcp_llm_model, or $mcp_llm_model_source. JavaScript users and operators cannot discover the option or understand the emitted data from the main package documentation.
Suggested fix
Add captureModel to the README options table with its default and object form. Add both properties to the event and property catalogs. Explain that the value is self-reported, unverified, optional, and unavailable when ownership cannot be confirmed.
Prompt to fix with AI (copy-paste)
## Context
@packages/mcp/src/types.ts#L132-145
<issue_description>
The new public option appears only in TypeScript API comments and the changeset. The README has an options table, an event catalog, and a property catalog. None describe `captureModel`, `$mcp_llm_model`, or `$mcp_llm_model_source`. JavaScript users and operators cannot discover the option or understand the emitted data from the main package documentation.
</issue_description>
<issue_validation>
- **Checked:** every documentation artifact in the package — `packages/mcp/README.md` in full, `packages/mcp/docs/ARCHITECTURE.md` section by section, and the `docs/adr/` set — against the new public surface the PR adds (`captureModel` on `MCPAnalyticsOptions`, and the `LlmModel` / `LlmModelSource` wire keys written in `packages/mcp/src/extensions/posthog-events.ts:173-180`).
- **Found:** the finding names the wrong file. `packages/mcp/README.md` holds no options table, no event catalog, and no property catalog. It is 190 lines of deployment gotchas plus a local-development guide, and lines 3-5 state that usage examples live in the official docs at `https://posthog.com/docs/mcp-analytics`. So the claim that JavaScript users cannot discover the option "from the main package documentation" misreads what that README is for.
- **Found:** the three artifacts the finding describes do exist, in `packages/mcp/docs/ARCHITECTURE.md` — §5 Event catalog (line 99), §6 Property catalog (line 117), §7 Customer extension points (line 163). The PR changes 12 files and none of them is a documentation file, so all three go stale on merge.
- **Found:** those catalogs are actively maintained, not stale boilerplate. §6 opens with "All wire keys live in `PostHogMCPAnalyticsProperty`" and then enumerates every one, including the newest additions `$mcp_client_user_agent`, `$mcp_vendor_client`, and `$mcp_protocol_version`. Adding `$mcp_llm_model` and `$mcp_llm_model_source` without a row makes that opening sentence false.
- **Found:** the closest precedent is already documented in exactly the shape the finding asks for. The `ConversationId` row in §6 reads "Optional; set when `enableConversationId: true` and the SDK owns the tool's injected `conversation_id` parameter", and §7 lists `enableConversationId` with its `false` default. `captureModel` is the same kind of opt-in, ownership-gated injected parameter and has neither entry.
- **Impact:** confirmed drift in the package's own reference documentation. An analyst who sees `$mcp_llm_model` in PostHog finds no entry for it, and a reader of the options table sees no `captureModel`. The fix is mechanical: one row in §7 and two in §6.
- **Priority:** lowered to `consider`. The verified impact is internal reference drift with no behavioral consequence, and the user-facing half of the stated impact does not hold — the package README deliberately defers the user manual to posthog.com, which lives outside this repo. Anyone acting on this should edit `packages/mcp/docs/ARCHITECTURE.md`, not the README.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Add `captureModel` to the README options table with its default and object form. Add both properties to the event and property catalogs. Explain that the value is self-reported, unverified, optional, and unavailable when ownership cannot be confirmed.
</potential_solution>
… injector, document captureModel Addresses review feedback on #4633. Blocking: model injection ran before reportMissing appended get_more_tools, so with both options on the advertised virtual tool had no llm_model and its missing-capability reports could never self-report a model — the one call type silently missing the property. Injection now runs after the append. Ownership is still cached from the un-injected descriptor, and context stays excluded because the virtual tool declares its own. Suggestion: addModelParameterToTool duplicated addContextParameterToTool almost verbatim. Both now delegate to a shared addAnalyticsParameterToTool in analytics-parameters.ts, so schema ownership, cloning, and required-field handling cannot drift. Docs: README gains a captureModel section covering the ownership limit (it degrades to silence, unlike context, and the high-level path resolves ownership per request from the registry); ARCHITECTURE gains the §5/§6/§7 entries for captureModel, $mcp_llm_model, and $mcp_llm_model_source; the changeset no longer promises unconditional stripping and capture. Generated-By: PostHog Desktop Task-Id: 44c7be3e-4938-4f9a-bb80-89fd9d74db6d
Contributor
Author
|
Both review comments and both ReviewHog findings were valid. Fixed in a960d92. Virtual tool missing Duplicated injection logic (suggestion). Undocumented ownership limit. Worth flagging that Stale reference docs. Added the On verification: the package suites were initially failing to load in my environment because the workspace deps were unbuilt, so I took a stashed baseline first and confirmed the failures were identical with and without my changes before fixing the environment. Full suite is 46 suites and 667 tests passing, lint clean. I mutation tested the new regression test rather than trusting it: reverting only the reorder fails the schema assertion, restoring it passes. The second new test covers the missing capability capture path but passes either way, since capture keys off cached ownership rather than the advertised schema, so the schema assertion is the actual guard. One deliberate omission: both markdown files were already non conforming to Prettier at HEAD and the package lint script only covers |
dustinbyrne
reviewed
Aug 25, 2026
dustinbyrne
left a comment
dustinbyrne
left a comment
Contributor
There was a problem hiding this comment.
Agent-led review, human-reviewed before posting.
| 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'), |
Contributor
There was a problem hiding this comment.
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.
Contributor
Author
There was a problem hiding this comment.
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.
…list cache The missing-capability branch called captureToolCall with no parameterOwnership, so ownership fell back to the per-instance tools/list cache. On a per-request server instance (the topology in ADR-0011) that cache is empty, so a get_more_tools call handled by an instance that never served a listing neither captured $mcp_llm_model nor stripped the injected llm_model argument. That branch is only entered when the application does not advertise a tool by this name, so the descriptor is the SDK's own and what it declares is known statically — the fail-closed rationale for registered tools (the value might be the application's own argument) cannot apply. Resolve ownership from the descriptor instead, via a shared helper used by both the high-level and low-level adapters, which had the same gap. conversation_id deliberately still reads from the cache: resolving it statically would start minting a handle, and appending its prompt-back block, on instances that today mint none — a change to session anchoring (ADR-0004) rather than to model capture. Tests cover both adapters with separate advertising and calling instances, and the README no longer implies the live registry covers the virtual tool. Generated-By: PostHog Desktop Task-Id: 44c7be3e-4938-4f9a-bb80-89fd9d74db6d
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
MCP server owners want to know which LLM is driving their server so they can tell whether tool-call quality degrades on specific models (raised by a customer via the MCP analytics team). The MCP wire deliberately carries no model identity — we validated with a raw-logging MCP server that traffic from the same client is byte-identical across model and effort settings, and no spec revision (through 2026-07-28) changes that. The one place the information exists is the agent itself: harnesses inject the model id into the agent's system prompt, and in testing agents restated it exactly (5/5 exact matches, including full dated model ids) via an injected tool parameter.
Changes
Adds an opt-in
captureModeloption to@posthog/mcp, reusing thecontext→$mcp_intentinjection machinery:llm_modelstring parameter into every tool attools/list; strips it before the handler runs.$mcp_llm_modelwith$mcp_llm_model_source = "self_reported"— honestly labeled as unverified, matching how the MCP spec treatsclientInfo.llm_modelkeeps it — never stolen, never captured."unknown"from the agent is dropped rather than recorded; value is truncated like other metadata fields.Release info Sub-libraries affected
Libraries affected
Checklist
If releasing new changes
.changeset/model-capture-option.md)🤖 Agent context
Autonomy: Human-driven (agent-assisted)
$mcp_intentfill rates across major harnesses support cross-harness viability of injected parameters.context, which is on by default) because the parameter adds tokens to every tool call and the value is self-reported.Created with PostHog Desktop