Conversation
Contributor
posthog-python Compliance ReportDate: 2026-09-16 10:04:21 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
|
Contributor
Prompt To Fix All With AI### Issue 1
posthog/mcp/_instrument_fastmcp.py:337-349
**Duplicated prompt-back policy**
This helper repeats the same mint/delivery logic in `_instrument_lowlevel.py` and `_instrument_v2.py`, recreating the risk of adapters drifting apart. It violates the repository’s “says everything once and only once” requirement, which must be satisfied before merging. Move the shared policy into one helper and leave only adapter-specific result wrapping in each adapter.
### Issue 2
posthog/test/mcp/test_units.py:318-320
**Tool-name loop tests identical inputs**
Each iteration calls `resolve_conversation_id(True, {})` with identical inputs; `name` only changes the assertion message. This suggests tool-specific coverage without exercising different behavior and violates the repository’s “no superfluous parts” requirement, which must be satisfied before merging. Replace the loop with one assertion and keep tool-name coverage at the lifecycle or adapter level, where names are actually consumed.
```suggestion
cid, minted = resolve_conversation_id(True, {})
assert minted is True and cid
```
---
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): anchor the virtual tools to t..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+337
to
+349
| def _conversation_prompt_back(lifecycle: Any) -> Tuple[Any, bool]: | ||
| """``(prompt_back_block_or_None, delivered)`` for a virtual tool's reply. | ||
|
|
||
| A minted handle rides a prompt-back block so the agent can echo it on its | ||
| next call and keep the whole exchange in one session; an echoed one needs no | ||
| delivery. ``delivered`` gates stamping the handle on the event, so a handle | ||
| the agent never received is never recorded.""" | ||
| if lifecycle.conversation_id and lifecycle.minted_conversation_id: | ||
| block = mcp_types.TextContent( | ||
| type="text", text=build_prompt_back(lifecycle.conversation_id)["text"] | ||
| ) | ||
| return block, True | ||
| return None, bool(lifecycle.conversation_id) |
Contributor
There was a problem hiding this comment.
This helper repeats the same mint/delivery logic in _instrument_lowlevel.py and _instrument_v2.py, recreating the risk of adapters drifting apart. It violates the repository’s “says everything once and only once” requirement, which must be satisfied before merging. Move the shared policy into one helper and leave only adapter-specific result wrapping in each adapter.
Context Used: Be direct and concise: state the issue, its impact, and the fix, with no preamble or praise. Do not comment on alphabetical sorting, trailing commas, or formatting. Linters catch these. Judge code by four simplicity rules: it passes all the tests, ex... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/mcp/_instrument_fastmcp.py
Line: 337-349
Comment:
**Duplicated prompt-back policy**
This helper repeats the same mint/delivery logic in `_instrument_lowlevel.py` and `_instrument_v2.py`, recreating the risk of adapters drifting apart. It violates the repository’s “says everything once and only once” requirement, which must be satisfied before merging. Move the shared policy into one helper and leave only adapter-specific result wrapping in each adapter.
**Context Used:** Be direct and concise: state the issue, its impact, and the fix, with no preamble or praise. Do not comment on alphabetical sorting, trailing commas, or formatting. Linters catch these. Judge code by four simplicity rules: it passes all the tests, ex... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Comment on lines
+318
to
+320
| for name in ("get_more_tools", "send_feedback", "echo"): | ||
| cid, minted = resolve_conversation_id(True, {}) | ||
| assert minted is True and cid, name |
Contributor
There was a problem hiding this comment.
Each iteration calls resolve_conversation_id(True, {}) with identical inputs; name only changes the assertion message. This suggests tool-specific coverage without exercising different behavior and violates the repository’s “no superfluous parts” requirement, which must be satisfied before merging. Replace the loop with one assertion and keep tool-name coverage at the lifecycle or adapter level, where names are actually consumed.
Suggested change
| for name in ("get_more_tools", "send_feedback", "echo"): | |
| cid, minted = resolve_conversation_id(True, {}) | |
| assert minted is True and cid, name | |
| cid, minted = resolve_conversation_id(True, {}) | |
| assert minted is True and cid |
Context Used: Be direct and concise: state the issue, its impact, and the fix, with no preamble or praise. Do not comment on alphabetical sorting, trailing commas, or formatting. Linters catch these. Judge code by four simplicity rules: it passes all the tests, ex... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/test/mcp/test_units.py
Line: 318-320
Comment:
**Tool-name loop tests identical inputs**
Each iteration calls `resolve_conversation_id(True, {})` with identical inputs; `name` only changes the assertion message. This suggests tool-specific coverage without exercising different behavior and violates the repository’s “no superfluous parts” requirement, which must be satisfied before merging. Replace the loop with one assertion and keep tool-name coverage at the lifecycle or adapter level, where names are actually consumed.
```suggestion
cid, minted = resolve_conversation_id(True, {})
assert minted is True and cid
```
**Context Used:** Be direct and concise: state the issue, its impact, and the fix, with no preamble or praise. Do not comment on alphabetical sorting, trailing commas, or formatting. Linters catch these. Judge code by four simplicity rules: it passes all the tests, ex... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
gesh
force-pushed
the
posthog/mcp-virtual-tool-conversation-id
branch
2 times, most recently
from
September 16, 2026 09:23
3bea84f to
31d5a92
Compare
gesh
force-pushed
the
posthog/mcp-virtual-tool-first-page
branch
from
September 16, 2026 09:29
8daec48 to
3aac206
Compare
gesh
force-pushed
the
posthog/mcp-virtual-tool-conversation-id
branch
from
September 16, 2026 09:29
31d5a92 to
24b200b
Compare
gesh
force-pushed
the
posthog/mcp-virtual-tool-conversation-id
branch
from
September 16, 2026 09:54
24b200b to
af09f57
Compare
Capture intent for tools the host owns; skip it only for PostHog's own. The code asked that question two different ways and they disagreed. Interception asked a per-request probe, which was right. Intent and the conversation exemption asked `virtual_tool_collisions`, which only a served tools/list writes. On a process that had served none -- the ordinary multi-pod case the probe exists for -- a host tool named `get_more_tools` dispatched correctly but its $mcp_tool_call carried `$mcp_intent=None`. Permanent on FastMCP and v2 MCPServer; on raw low-level it self-healed after one call, because the MCP SDK's own `req is None` cache pass happens to refresh the state during dispatch. The intent guard turns out to be dead code for its stated purpose: PostHog's virtual tools are intercepted before dispatch and captured by record_missing_capability / record_feedback, so they never reach record_tool_call. Verified by spying on it. Everything that gets there is host-dispatched by construction, so the guard could only ever fire on a host tool sharing the name. Removed. The same applies to the conversation exemption: the virtual tools' capture paths discard the resolved handle, so exempting by name only ever cost the host's tool its handle. start_tool_call_lifecycle now reads `enabled_virtual_tool_names` instead of `injectable_`, so the call path stops reading listing state altogether. That also closes the cross-client report: the collision set is per-server and rewritten by whichever listing ran last, so reading it on the call path let one caller's catalogue decide another caller's call. Note on the reports: the missing conversation id they also cite is not name-specific -- an ordinary tool shows the same in that harness, so it is not a regression here. The new tests assert parity with an ordinary tool instead, which is the rule being fixed. The changeset claimed a real `get_more_tools` "keeps its $mcp_intent", which was only true after a listing on the same instance. Corrected. Reported by QA Swarm and veria-ai on #962. Generated-By: PostHog Desktop Task-Id: 989fd424-fd24-4ae4-8682-548b87f761f6
With enable_conversation_id on, get_more_tools and send_feedback now advertise conversation_id, echo a minted handle back over prompt-back, and stamp $mcp_conversation_id -- under any configured name. They were exempt, in three places that had to agree: the schema pass skipped the argument, resolve_conversation_id short-circuited on their names, and the lifecycle passed None when preparing their session. So a $mcp_feedback or $mcp_missing_capability event was filed under a $session_id of its own. An agent's complaint about a tool landed in a different session than the call it was complaining about, which is most of the value of the report, and the fallback session emitted a second spurious $mcp_initialize. resolve_conversation_id loses its two tool-name parameters: with no tool exempt there is nothing to compare against, so the exemption cannot come back by accident. The virtual tools still never get the injected `context` argument -- they state their intent through their own -- so `is_sdk_virtual_tool` now gates only that. Two tests are inverted on purpose, with the reasoning in their docstrings: test_feedback_never_mints_conversation_id becomes test_feedback_joins_the_conversation, and the renamed-tool schema case now expects the argument to track the option. Stacked on #962. Generated-By: PostHog Desktop Task-Id: 989fd424-fd24-4ae4-8682-548b87f761f6
The per-request ownership fix landed here rather than in #962, so its user-facing effects belong in this changeset, not that one. Generated-By: PostHog Desktop Task-Id: 989fd424-fd24-4ae4-8682-548b87f761f6
gesh
force-pushed
the
posthog/mcp-virtual-tool-conversation-id
branch
from
September 16, 2026 09:59
af09f57 to
dc9873c
Compare
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
1 participant
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.
Stacked on #962 — review that one first.
Problem
With
enable_conversation_id=True, the SDK's virtual tools were exempt from conversation anchoring. So a$mcp_feedbackor$mcp_missing_capabilityevent was filed under a$session_idof its own.An agent that calls
echo, hits friction, and reports it lands like this:$mcp_tool_call(echo)ses_57a5f376…$mcp_initializeses_01a0a522…← spurious second one$mcp_feedbackses_01a0a522…$mcp_missing_capabilityses_01a0a522…The complaint about
echois not in the session containingecho. You get an orphan session holding only the report, which is most of the value of the feature gone — the point of a report is being reachable from the calls that prompted it. After: one session for all four.Changes
get_more_toolsandsend_feedbacknow take part like any other tool, under any configured name: they advertiseconversation_id, echo a minted handle back over prompt-back, and stamp$mcp_conversation_id.The exemption lived in three places that had to agree, which is why it drifted:
is_sdk_virtual_toolgated bothcontextandconversation_id; it now gates onlycontext),resolve_conversation_idshort-circuited on the two names,Nonewhen preparing their session, so even a resolved handle was dropped.resolve_conversation_idloses its two tool-name parameters entirely — with no tool exempt there is nothing to compare against, so the exemption can't come back by accident.The virtual tools still never get the injected
contextargument; they state their intent through their own (contextforget_more_tools,summary/detailsforsend_feedback). A handle the agent never received is still never stamped, same as for ordinary tools.PostHogMCPis unaffected — the custom-dispatcher path has no conversation-id handling at all.Also here: per-request tool ownership
Two reviewer findings on #962 turned out to share one root cause, and the fix belongs with this PR rather than that one, since both are about attribution rather than where the tools are advertised.
The SDK asked "is this name mine?" two ways that disagreed. Interception asked a per-request probe, which was right. Intent and the conversation exemption asked
virtual_tool_collisions, which only a servedtools/listwrites — so on a process that had served none (the ordinary multi-pod case the probe exists for), a host tool namedget_more_toolsdispatched correctly but its$mcp_tool_callcarried$mcp_intent=None. Permanent on FastMCP and v2MCPServer.The intent guard turned out to be dead code for its stated purpose: PostHog's virtual tools are intercepted before dispatch and captured by
record_missing_capability/record_feedback, so they never reachrecord_tool_call(verified by spying on it). Everything arriving there is host-dispatched by construction, so the guard could only ever fire on a host tool sharing the name. Removed.start_tool_call_lifecyclenow reads options-only names, so the call path consults no listing state at all — which also closes the second finding, where one caller's listing could suppress another caller's virtual-tool handling on a server with caller-specific catalogues.One correction to the reports: the missing conversation id they also cite is not name-specific — an ordinary tool shows the same in that harness — so the new tests assert parity with an ordinary tool instead.
Two inverted tests
Both deliberate, with the reasoning in their docstrings so the next reader doesn't revert them:
test_feedback_never_mints_conversation_id→test_feedback_joins_the_conversation.conversation_idto track the option rather than always be absent.Plus new tests asserting the report and the call it is about share a
$session_id, for both the default and a renamed tool.Note on
@posthog/mcp@posthog/mcpcurrently excludes the virtual tools too, by a different route — its virtual descriptors never getconversation_idinjected, andresolveConversationIdis gated on schema-derived ownership, soownership.conversationIdis false for them. This PR therefore puts Python ahead of TypeScript, and a matching change is worth raising there so the two don't drift. (Checked against a local feature branch rather thanmain, so worth confirming.)Verification
pytest posthog/test/mcpon both MCP SDK majors: 512 passed on v1, 443 passed / 19 skipped on v2.posthog/test/ai/hitting a403 product_access_denied.ruff format --check,ruff check, filteredmypy(clean),check_public_api.py.$mcp_initializedisappears.Created with PostHog Desktop