Key-fingerprint-based analytics identity - #440
Conversation
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAnalytics now uses validated API-key fingerprints for selected Mixpanel identities, preserving heuristic fallbacks. Fingerprints flow through direct, resource-read, and community telemetry paths, with updated tests, documentation, and version metadata. ChangesAnalytics identity propagation
Sequence Diagram(s)sequenceDiagram
participant ToolDecorator
participant Analytics
participant Mixpanel
ToolDecorator->>Analytics: pass auth_origin and api_key_fingerprint
Analytics->>Analytics: choose fingerprint or heuristic distinct_id
Analytics->>Mixpanel: send event with derived distinct_id
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- Correct the disjointness rationale in _build_fingerprint_distinct_id: the "|" separators, not the "key:" tag, are the structural guarantee (a composite CAN start with "key:" via a spoofed x-forwarded-for); pin that adversarial case in the domain-separation test. - Pin the documented defensive row on the direct path: unknown/"none" origin with a fingerprint present still selects the legacy basis. - SPEC.md: record the accepted analytics-integrity risk that a forged community report can direct events into any chosen fingerprint-derived identity. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Collapse the three community-path fingerprint-basis tests into one parametrized test over auth_origin (client/server/None), matching the parametrize idiom already used for the direct-path rows. - Move the duplicated reset_mode_and_client fixture into tests/conftest.py as a shared, opt-in reset_analytics_state fixture (module-wide via pytestmark); drop the post-yield _mp_client reset, which monkeypatch teardown made a no-op. - Trim the UUIDv5 derivation detail from the api_key_fingerprint wire-schema description in models.py — the recipe stays documented in SPEC.md; the public description keeps only what a reporting instance needs to know. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Extract the duplicated DummyRequest/DummyCtx test doubles from tests/test_analytics.py and tests/test_analytics_identity.py into a shared tests/analytics_ctx_helpers.py module (same pattern as pro_api_key_helpers.py) and list it in the AGENTS.md tree. - Widen the test_analytics_identity.py module docstring: the module pins the basis-selection matrix for the direct path and the PageView carve-out, not only the community-reporting path. - Pin the community-path degenerate row auth_origin="none" with a present fingerprint: well-behaved reporters never send it, but the wire accepts it and the "any present fingerprint" rule must hold there too. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
AGENTS.md (1)
367-367: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep implementation mechanics out of
AGENTS.md.The distinct_id derivation algorithm is implementation-specific module documentation. Keep this entry at a high level and link readers to
SPEC.mdoranalytics.pyfor the identity rules.As per coding guidelines,
AGENTS.mdshould remain a search hint rather than implementation-specific module documentation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@AGENTS.md` at line 367, Update the AGENTS.md entry describing anonymous distinct_id generation to a high-level search hint, removing the key-fingerprint and IP/name/version algorithm details. Direct readers to SPEC.md or analytics.py for the identity rules while preserving the entry’s purpose and scope.Source: Coding guidelines
SPEC.md (1)
946-946: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep wire-contract details out of
SPEC.md.This paragraph mixes architectural rationale with validator/coercion and wire-format behavior. Keep the accepted analytics-integrity risk and pepper decision here, but defer exact contract details to
API.mdand the implementation.As per coding guidelines,
SPEC.mdshould focus on architecture and decisions rather than exhaustive technical behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@SPEC.md` at line 946, Condense the “Authorization context” section in SPEC.md to retain only the architectural rationale, accepted analytics-integrity risk, and server-side HMAC pepper decision. Remove exact field names, enum/coercion behavior, fingerprint wire-format constraints, and validator-specific details, directing contract specifics to API.md and implementation validators.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@SPEC.md`:
- Around line 929-930: Revise the distinct_id collision rationale in SPEC.md to
describe the actual input-shape separation: key-derived inputs are “key:”
followed by a 64-character hexadecimal digest, while legacy composite inputs
contain “|”. Remove the claim that the key: prefix alone prevents collisions,
and state the guarantee based on these distinct shapes.
---
Nitpick comments:
In `@AGENTS.md`:
- Line 367: Update the AGENTS.md entry describing anonymous distinct_id
generation to a high-level search hint, removing the key-fingerprint and
IP/name/version algorithm details. Direct readers to SPEC.md or analytics.py for
the identity rules while preserving the entry’s purpose and scope.
In `@SPEC.md`:
- Line 946: Condense the “Authorization context” section in SPEC.md to retain
only the architectural rationale, accepted analytics-integrity risk, and
server-side HMAC pepper decision. Remove exact field names, enum/coercion
behavior, fingerprint wire-format constraints, and validator-specific details,
directing contract specifics to API.md and implementation validators.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2bc00615-76ae-4de8-91a9-c27b2f88663c
📒 Files selected for processing (20)
AGENTS.mdSPEC.mdblockscout_mcp_server/__init__.pyblockscout_mcp_server/analytics.pyblockscout_mcp_server/models.pyblockscout_mcp_server/observability.pyblockscout_mcp_server/pro_api_key_context.pyblockscout_mcp_server/telemetry.pyblockscout_mcp_server/tools/decorators.pypyproject.tomlserver.jsontests/analytics_ctx_helpers.pytests/api/test_routes.pytests/conftest.pytests/test_analytics.pytests/test_analytics_helpers.pytests/test_analytics_identity.pytests/test_observability.pytests/test_tool_usage_report.pytests/tools/test_decorators.py
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Summary
Makes the Mixpanel
distinct_idderive from the PRO API key fingerprint instead of the IP/client-name/client-version composite, so a user keeps a stable analytics identity across client upgrades, IP churn, and transport changes.Closes #437.
What changed
_build_fingerprint_distinct_id(fingerprint)inblockscout_mcp_server/analytics.py—uuid5(NAMESPACE_URL, "https://mcp.blockscout.com/mcp" + "key:" + fingerprint). Thekey:domain tag keeps the two bases' input spaces from ever colliding.track_tool_invocation/track_resource_read): fingerprint basis only whenauth_origin == "client"— a server-key fingerprint there identifies the deployment, not a user.track_community_usage): fingerprint basis whenever one is present, regardless of origin — for a self-hosted report, a server-key fingerprint identifies the installation and a client-key fingerprint identifies that installation's individual users, so either is meaningful.tools/decorators.pyandobservability.pynow passapi_key_fingerprintthrough to analytics.distinct_idand is never added to the event properties.telemetry.py,models.py,pro_api_key_context.py; SPEC.md and AGENTS.md updated; version bumped to0.18.0.dev0.Testing
tests/test_analytics_identity.pycovers the full basis-selection matrix for both paths, plus the relocated fingerprint-leak test.analytics.py/tools/decorators.py/observability.pyconfirmed exercised./skill/SKILL.mdresource read, andPOST /v1/report_tool_usageproduced 4 events all carrying the same fingerprint-deriveddistinct_id, independently recomputed from the raw key. Zero occurrences of the raw key and zero 64-hex digests anywhere in the captured payloads.ruff check .andruff format --check .clean across 215 files.Reviewer notes
track_community_usagedocstring for the reasoning.distinct_ids change for authenticated users. Historical events keyed on the old composite will not merge with the new identity.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
distinct_idderived from API key fingerprints when applicable.Bug Fixes
Documentation
Chores
0.18.0.dev0.