feat(mcp): capture model identifiers - #927
Conversation
Capture model identifiers from recognized client metadata, with an SDK-owned self-report fallback for other clients. Preserve source provenance and fail closed when the application owns the field.\n\nVerify both MCP Python SDK 1.x and 2.x, including a 2026-07-28 wire-level call.
posthog-python Compliance ReportDate: 2026-09-08 18:45:26 UTC ✅ All Tests Passed!111/111 tests passed Capture_V1 Tests✅ 94/94 tests passed View Details
Feature_Flags Tests✅ 17/17 tests passed View Details
|
Prompt To Fix All With AI### Issue 1
posthog/mcp/_model_parameters.py:68-69
**Strict schemas become permissive**
When model capture is enabled, this removes `additionalProperties: false` from otherwise strict input schemas even though `llm_model` has already been added to `properties`. Raw low-level servers use the advertised schema for validation and do not strip other keys, so clients can submit undeclared arguments that were previously rejected. Preserve the strictness constraint while adding the declared analytics property.
### Issue 2
posthog/mcp/posthog_mcp.py:319-322
**Injected field loses ownership**
For object-based tools, `prepare_tool_list()` mutates the original object by adding `llm_model`. If that same object is then passed as `original_tool`, as the README recommends, this check mistakes the SDK-injected field for an application-owned field. As a result, `prepare_tool_call()` neither captures nor strips the self-reported model, and dispatch can receive an unexpected `llm_model` argument. Record ownership before mutating object schemas or avoid inferring it from an already-mutated object.
### Issue 3
posthog/mcp/_instrumentation.py:690-694
**Low-level model field optional**
Model requiredness is tied to the adapter-specific `context_required` flag. Both low-level adapters pass this flag as false, so compatible tool schemas advertise `llm_model` as optional even though the documented feature contract says the self-report field is required. Schema-driven clients may therefore omit it, causing model capture to silently produce no value. Give model injection its own requiredness decision or explicitly exclude these adapters from the documented contract.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "feat(mcp): capture model identifiers" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if schema.get("additionalProperties") is False: | ||
| schema.pop("additionalProperties", None) |
There was a problem hiding this comment.
When model capture is enabled, this removes additionalProperties: false from otherwise strict input schemas even though llm_model has already been added to properties. Raw low-level servers use the advertised schema for validation and do not strip other keys, so clients can submit undeclared arguments that were previously rejected. Preserve the strictness constraint while adding the declared analytics property.
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/mcp/_model_parameters.py
Line: 68-69
Comment:
**Strict schemas become permissive**
When model capture is enabled, this removes `additionalProperties: false` from otherwise strict input schemas even though `llm_model` has already been added to `properties`. Raw low-level servers use the advertised schema for validation and do not strip other keys, so clients can submit undeclared arguments that were previously rejected. Preserve the strictness constraint while adding the declared analytics property.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Fixed in 11c48e9. Model injection now preserves additionalProperties: false. Context and conversation-ID injection also preserve it, so later stages cannot relax the constraint. Regression coverage verifies that declared analytics fields remain valid and undeclared arguments are rejected, including after a low-level tool-cache rebuild.
| if original_tool is not None: | ||
| analytics_owns_model = can_inject_model_parameter( | ||
| _tool_schema(original_tool) | ||
| ) |
There was a problem hiding this comment.
For object-based tools, prepare_tool_list() mutates the original object by adding llm_model. If that same object is then passed as original_tool, as the README recommends, this check mistakes the SDK-injected field for an application-owned field. As a result, prepare_tool_call() neither captures nor strips the self-reported model, and dispatch can receive an unexpected llm_model argument. Record ownership before mutating object schemas or avoid inferring it from an already-mutated object.
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/mcp/posthog_mcp.py
Line: 319-322
Comment:
**Injected field loses ownership**
For object-based tools, `prepare_tool_list()` mutates the original object by adding `llm_model`. If that same object is then passed as `original_tool`, as the README recommends, this check mistakes the SDK-injected field for an application-owned field. As a result, `prepare_tool_call()` neither captures nor strips the self-reported model, and dispatch can receive an unexpected `llm_model` argument. Record ownership before mutating object schemas or avoid inferring it from an already-mutated object.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Fixed in 11c48e9. Model injection now copies object tools before changing their schemas, leaving the original application tool available for ownership checks. Regression cases cover repeated listings with and without original_tool, using SDK tool objects and custom descriptors across both MCP SDK majors. Self-reported values are captured and stripped before dispatch.
| schema = add_model_parameter_to_schema( | ||
| schema, | ||
| tool.name, | ||
| get_model_description(data.options.capture_model), | ||
| required=context_required, |
There was a problem hiding this comment.
Model requiredness is tied to the adapter-specific context_required flag. Both low-level adapters pass this flag as false, so compatible tool schemas advertise llm_model as optional even though the documented feature contract says the self-report field is required. Schema-driven clients may therefore omit it, causing model capture to silently produce no value. Give model injection its own requiredness decision or explicitly exclude these adapters from the documented contract.
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/mcp/_instrumentation.py
Line: 690-694
Comment:
**Low-level model field optional**
Model requiredness is tied to the adapter-specific `context_required` flag. Both low-level adapters pass this flag as false, so compatible tool schemas advertise `llm_model` as optional even though the documented feature contract says the self-report field is required. Schema-driven clients may therefore omit it, causing model capture to silently produce no value. Give model injection its own requiredness decision or explicitly exclude these adapters from the documented contract.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Addressed through the documented-contract option in 11c48e9. The README now distinguishes required fields on custom dispatchers and official high-level adapters from optional fields on raw low-level servers and standalone FastMCP. The standalone adapter strips injected fields before input validation, so making llm_model required there would reject calls after stripping. Requiredness is unchanged; the low-level regression also verifies that a call omitting the field succeeds.
gesh
left a comment
gesh
left a comment
There was a problem hiding this comment.
Note
🤖 Automated comment by QA Swarm — not written by a human
QA Swarm review complete. See inline comments.
| ) | ||
| else: | ||
| analytics_owns_model = self._model_parameter_injected.get(name, False) | ||
| llm_model, llm_model_source = resolve_model( |
There was a problem hiding this comment.
Note
🤖 Automated comment by QA Swarm — not written by a human
[convergent: router + validation] 🟠 HIGH
resolve_model() is called outside the capture_model gate. Line 318 gates only analytics_owns_model, and resolve_model() checks Codex x-codex-turn-metadata before it consults allow_self_reported (_model_parameters.py:123-127). So the client-metadata branch runs even when capture_model is left at its default of False.
Reproduced at this head: a PostHogMCP built without capture_model and called as prepare_tool_call("search", {"q": "hello"}, request_meta={"x-codex-turn-metadata": {"model": "..."}}) returns a populated llm_model with source client_metadata, where both should be None.
There is no downstream gate to catch it. _apply_model() writes the fields onto the event whenever they are non-empty, and capture_tool_call() / capture_missing_capability() take them as plain parameters without re-checking the option. A host that plumbs PreparedToolCall through therefore emits the model identifier with no opt-in anywhere in the chain.
This is the documented custom-dispatcher path, and it contradicts the PR's own "off by default" claim. The instrument() adapters get this right — both resolve_model() call sites in _instrumentation.py sit inside if is_capture_model_enabled(...).
Fix: gate the call itself, matching the adapters.
llm_model, llm_model_source = None, None
if is_capture_model_enabled(self._capture_model):
llm_model, llm_model_source = resolve_model(
request_meta, args, allow_self_reported=analytics_owns_model
)Worth a test too: every capture_model test in the suite constructs the client with capture_model=True, so the default-off case is unpinned.
There was a problem hiding this comment.
Confirmed and fixed in 11c48e9. The resolve_model() call itself now sits inside the capture_model gate, with both prepared fields initialized to None. The capture test now covers enabled, explicit False, and default settings with Codex metadata present, checking both prepared values and emitted event properties.
| pass | ||
| return tool | ||
|
|
||
| def _inject_models(self, tools: List[Any]) -> List[Any]: |
There was a problem hiding this comment.
Note
🤖 Automated comment by QA Swarm — not written by a human
[router] 🟢 LOW
_inject_models() clears self._model_parameter_injected and then rebuilds it, while prepare_tool_call() reads the same dict at line 324. Between the clear() and the update(), a reader sees an empty map, concludes the SDK does not own llm_model for that tool, and skips stripping it from prepared_args — so the injected argument reaches the real tool.
Reachability is limited: there is no await between the two statements, so asyncio alone cannot interleave here. It needs genuine OS threads sharing one PostHogMCP and re-listing tools while a call is in flight. That is a narrow window, but the class is explicitly built for reuse across concurrent requests, and context never had a comparable shared ownership cache.
Cheapest fix is to swap rather than mutate: build the new dict locally and assign it in one statement, so a concurrent reader sees either the old map or the new one, never an empty one. If you would rather not spend anything here, a comment stating that prepare_tool_list() must not run concurrently with prepare_tool_call() would at least make the assumption explicit.
There was a problem hiding this comment.
Fixed in 11c48e9. Ownership is now built in a local dictionary, including injection failures, and assigned to the instance only after preparation completes. The existing map remains available during rebuilding; the clear()/update() window is gone.
| return schema | ||
|
|
||
|
|
||
| def can_inject_model_parameter(input_schema: Any) -> bool: |
There was a problem hiding this comment.
Note
🤖 Automated comment by QA Swarm — not written by a human
[router] 🟢 LOW
can_inject_model_parameter() re-derives the same three conditions that add_model_parameter_to_schema() already implements at lines 32-49: does the schema own llm_model, and is it a $ref / oneOf / allOf / anyOf schema. The two are kept in sync by hand.
Since those conditions are exactly what makes the feature fail closed, an edit to one copy that misses the other reintroduces the ownership bug this design exists to prevent. Consider extracting one predicate and having add_model_parameter_to_schema() call it.
There was a problem hiding this comment.
Updated in 11c48e9. add_model_parameter_to_schema() now calls can_inject_model_parameter() rather than duplicating the ownership and complex-schema checks. Existing coverage for application-owned fields and complex schemas passes.
| request_meta: Optional[JsonRecord] = None, | ||
| original_tool: Any = None, | ||
| ) -> PreparedToolCall: | ||
| """Pull the agent's intent off the injected ``context`` argument, strip |
There was a problem hiding this comment.
Note
🤖 Automated comment by QA Swarm — not written by a human
[router] ⚪ NIT
The docstring still describes only the context behavior. This method now also resolves the model, records the source, and strips llm_model from the arguments — worth a line, since this is the documented entry point for the new feature on the custom-dispatcher path.
There was a problem hiding this comment.
Updated in 11c48e9. The prepare_tool_call() docstring now describes opt-in model resolution, source attribution, and stripping the SDK-owned llm_model argument. The list helper and README also explain object copying and original-tool ownership.
Gate custom-dispatcher model resolution behind capture_model and copy object tools before model injection so repeated listings preserve ownership. Publish the completed ownership map without an intermediate empty state. Keep additionalProperties constraints in all analytics schema injectors. Document adapter-specific requiredness rather than requiring fields that standalone FastMCP strips before input validation. Share the eligibility predicate and update prepare helper documentation and the changeset. Verification: - MCP v1 suite: 267 passed. - MCP v2 suite: 245 passed, 13 expected skips. - Ruff check and format check passed repository-wide. - Repository mypy/baseline check passed (229 source files). - Public API snapshot and git diff --check passed. - Regression cases failed before the fix for disabled capture, object ownership and strict validation through a low-level tool-cache rebuild. Full non-MCP test suite not rerun locally; CI covers the broader matrix.
2 participants
💡 Motivation and Context
Python MCP server owners can compare tool behavior by calling model, matching posthog-js#4829.
$mcp_llm_model_sourcedistinguishesclient_metadatafromself_reportedvalues.Public API snapshot updates and the shared injection predicate are mechanical.
💚 How did you test it?
📝 Checklist
If releasing new changes
sampo addto generate a changeset file🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Codex authored the change and review fixes with terminal and GitHub CLI tools. Skills: debugging MCP analytics, writing tests, writing dataclasses, and writing PR descriptions.
Model resolution respects opt-in on custom dispatchers. Optional fields remain optional where stripping them before validation would otherwise reject calls.