fix(mcp): plug feedback-tool-shadow and error-leak gaps from review

- _matches_extra_schema: a declared "integer" extra accepted any float,
  including fractional ones (3.5), since Python's numeric tower conflates
  int and float; now requires the value to be whole.
- handle_feedback: an on_feedback exception was logged with str(error)
  verbatim, letting agent-controlled report text (PII, credentials,
  log-forging newlines) an error message echoes reach host logs; now logs
  only the exception type, matching the report log beside it.
- start_tool_call_lifecycle: conversation-id resolution skipped every call
  named like the feedback tool regardless of the listing-derived shadow
  flag, so a real tool that collided with the configured feedback name
  never got a conversation id even once ownership was known; now mirrors
  ToolCallLifecycle.is_feedback's fail-open guard.

Addresses greptile-apps findings on PR #939.

Generated-By: PostHog Desktop
Task-Id: 0b5063cb-6fcf-4364-be5e-de945b1448f0

Author: gesh

posthog/mcp/_instrumentation.py

@@ -553,7 +553,11 @@ def start_tool_call_lifecycle(
     feedback_options = resolve_collect_feedback_options(data.options.collect_feedback)
     feedback_name = (
         resolve_send_feedback_tool_name(feedback_options)
-        if feedback_options is not None
+        # Mirrors `ToolCallLifecycle.is_feedback`'s fail-open guard below: once a
+        # real application tool is known to own this name, conversation-id
+        # resolution must treat calls to it like any other tool too, not skip
+        # them as if they were the (shadowed) virtual feedback tool.
+        if feedback_options is not None and not data.feedback_tool_shadowed
         else None
     )
     conversation_id, minted = resolve_conversation_id(

posthog/mcp/feedback.py

@@ -207,7 +207,15 @@ def _matches_extra_schema(value: Any, schema: Dict[str, Any]) -> bool:
     else:
         actual = "object"
     declared = schema.get("type")
-    if declared != actual and not (declared == "integer" and actual == "number"):
+    if declared == "integer" and actual == "number":
+        # A JSON Schema `integer` also accepts a whole-valued float (`3.0`), but
+        # not a fractional one (`3.5`) - Python's numeric tower conflates int and
+        # float here, so the type name alone can't tell them apart.
+        if not (
+            isinstance(value, int) or (isinstance(value, float) and value.is_integer())
+        ):
+            return False
+    elif declared != actual:
         return False
     enum = schema.get("enum")
     return not isinstance(enum, list) or value in enum
@@ -352,8 +360,13 @@ async def handle_feedback(
             if isinstance(reply, str) and reply.strip():
                 return reply
         except Exception as error:  # noqa: BLE001 - never break the agent's turn
+            # Only the exception's type, matching the report log above: a
+            # handler can echo the unsanitized report (PII, credentials,
+            # log-forging newlines, unbounded length) into its error message,
+            # and that agent-controlled text does not belong in host logs
+            # any more than `report.summary` does.
             log(
-                "Warning: on_feedback handler threw; returning the default "
-                f"acknowledgement - {error}"
+                "Warning: on_feedback handler threw "
+                f"({type(error).__name__}); returning the default acknowledgement"
             )
     return _SEND_FEEDBACK_RESULT_TEXT

posthog/test/mcp/test_feedback.py

@@ -304,6 +304,28 @@ def test_extras_must_match_declared_type_and_enum():
     assert conforming.extras == {"score": 9, "channel": "web"}
 
 
+def test_extras_declared_integer_rejects_fractional_float():
+    # `isinstance(value, (int, float))` alone can't tell `3` from `3.5` - both
+    # are Python `number`s - so a declared `integer` extra must additionally
+    # check the value has no fractional part before it's trusted downstream.
+    options = CollectFeedbackOptions(extra_properties={"score": {"type": "integer"}})
+
+    fractional = parse_feedback_report(
+        {"feedback_type": "praise", "summary": "s", "score": 3.5}, options
+    )
+    assert fractional.extras == {}
+
+    whole_float = parse_feedback_report(
+        {"feedback_type": "praise", "summary": "s", "score": 3.0}, options
+    )
+    assert whole_float.extras == {"score": 3.0}
+
+    whole_int = parse_feedback_report(
+        {"feedback_type": "praise", "summary": "s", "score": 3}, options
+    )
+    assert whole_int.extras == {"score": 3}
+
+
 def test_tool_name_gets_pii_redaction():
     report = parse_feedback_report(
         {"feedback_type": "issue", "summary": "s", "tool_name": "ask jane@example.com"}
@@ -588,6 +610,42 @@ def on_feedback(report):
     assert _events(client, "$mcp_feedback")
 
 
+async def test_on_feedback_raise_does_not_log_agent_text():
+    # A raising backend can echo the unsanitized report (PII, credentials,
+    # forged newlines) into its exception message; the warning log must not
+    # repeat it, mirroring the report log just above it that logs only the type.
+    from posthog.mcp import set_logger
+
+    server = make_fastmcp()
+    client = FakeClient()
+    secret_summary = "credit card 4242-4242-4242-4242 jane@example.com"
+
+    def on_feedback(report):
+        raise RuntimeError(f"backend rejected: {report.summary}")
+
+    instrument(
+        server,
+        client,
+        MCPAnalyticsOptions(
+            collect_feedback=CollectFeedbackOptions(on_feedback=on_feedback)
+        ),
+    )
+
+    messages = []
+    set_logger(messages.append)
+    try:
+        canned = await server._tool_manager.call_tool(
+            "send_feedback", {**dict(_REPORT_ARGS), "summary": secret_summary}
+        )
+        await _flush()
+    finally:
+        set_logger(None)
+
+    assert canned[0].text == send_feedback_result_text()
+    assert any("on_feedback handler threw" in message for message in messages)
+    assert not any(secret_summary in message for message in messages)
+
+
 # --- instrument(): low-level v1 ----------------------------------------------------
 
 

posthog/test/mcp/test_v2_mcpserver.py

@@ -429,6 +429,33 @@ def send_feedback(note: str) -> str:
     assert _events(client, "$mcp_tool_call")
 
 
+async def test_collect_feedback_collision_keeps_conversation_id():
+    # The name collision must fail open for every feature keyed off the
+    # feedback tool name, not just dispatch - conversation-id resolution used
+    # to keep skipping the real tool because it checked the configured name
+    # alone, ignoring the listing-derived shadow flag.
+    server = make_server()
+
+    @server.tool()
+    def send_feedback(note: str) -> str:
+        return f"real tool got {note}"
+
+    client = FakeClient()
+    instrument(
+        server,
+        client,
+        MCPAnalyticsOptions(collect_feedback=True, enable_conversation_id=True),
+    )
+
+    await _list_tools(server)
+    await _call_tool(server, "send_feedback", {"note": "hi", "context": "real tool"})
+    await _flush()
+
+    calls = _events(client, "$mcp_tool_call")
+    assert len(calls) == 1
+    assert calls[0]["properties"].get("$mcp_conversation_id")
+
+
 async def test_instrument_is_idempotent():
     server = make_server()
     client = FakeClient()