Repository navigation
fix(mcp): stop insight-update from reporting success when nothing persists - #108314
posthog[bot] wants to merge 3 commits into
Conversation
…sists The insight-update schema drops keys it does not declare. A nested or misnamed field reached the API as an empty PATCH, which saved the row, changed only updated_at, and returned success. A hand-written override now rejects a call with no field to change, and it checks the saved insight against the requested name, description, favorited, tags, and dashboards before it returns success. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 52fe957e-c02e-4ed1-b620-cbebd01a7b05
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
🤖 CI report✅ Trunk lane — non-backend lane (
|
| File | Comment lines | Added lines |
|---|---|---|
services/mcp/src/tools/insights/update.ts |
17 | 91 |
services/mcp/tests/unit/insight-update-persistence.test.ts |
1 | 88 |
This check does not block merging. It updates on every push and clears when the share drops.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 52fe957e-c02e-4ed1-b620-cbebd01a7b05
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: PostHog/posthog/.coderabbit.yaml Review profile: QUIET Plan: Enterprise Run ID: 📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughThe MCP tool registry now uses a wrapper for Priority: ⚪ Not assessed Merge Risk: ⚪ Minimal · up to Dashboard-only updates are checked against the returned tiles before success is reported. No actionable merge-blocking risk remains after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change prevents empty insight updates from reporting success and retains the existing project-scoped update path. Its main remaining risk is that an error can be returned after a change has already been saved, making recovery important. Retained concerns
Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
services/mcp/tests/unit/insight-update-persistence.test.ts (1)
77-81: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the fields sent in the PATCH body.
The response mock returns matching values independently of the request. The test can pass if the PATCH body omits
name,tags,favorited, ordashboards. Assert these fields alongside the method and path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 72694a85-6548-4476-a0ff-4dd94274fe37
📒 Files selected for processing (4)
services/mcp/src/tools/index.tsservices/mcp/src/tools/insights/update.tsservices/mcp/tests/unit/insight-update-persistence.test.tsservices/mcp/tests/unit/tool-name-validation.test.ts
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 3 remain after this review.
| params.dashboards !== undefined && | ||
| Array.isArray(saved.dashboards) && | ||
| JSON.stringify(sortedIds(params.dashboards)) !== JSON.stringify(sortedIds(saved.dashboards)) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🔎 Supported by static analysis
🏁 Script executed:
sed -n '1,115p' services/mcp/src/tools/insights/update.ts
sed -n '385,470p' services/mcp/src/tools/generated/product_analytics.ts
rg -n 'include.*dashboard|dashboards.*include|session.*dashboard|insight.*dashboard' services/mcp/src/generated/product_analytics/api.ts | head -65Repository: PostHog/posthog
Length of output: 12292
🏁 Script executed:
sed -n '60,125p' services/mcp/src/generated/product_analytics/api.ts
sed -n '150,220p' services/mcp/src/generated/product_analytics/api.ts
sed -n '360,425p' services/mcp/src/generated/product_analytics/api.ts
rg -n -A18 -B8 'function omitResponseFields|const omitResponseFields|omitResponseFields' services/mcp/src
rg -n -A12 -B8 'include_dashboards' services/mcp/src/tools/generated services/mcp/src/generated | head -160Repository: PostHog/posthog
Length of output: 41746
🏁 Script executed:
rg -n -A20 -B12 'InsightsPartialUpdateParams|InsightUpdateSchema|include_dashboards|InsightRetrieveSchema' services/mcp/src/generated/product_analytics/api.ts services/mcp/src/tools/generated/product_analytics.ts
rg -n -A18 -B8 'insight-get|insightGet|include_dashboards' services/mcp/src/tools/generated/product_analytics.ts services/mcp/src/tools/insights services/mcp/src/generated/product_analyticsRepository: PostHog/posthog
Length of output: 41696
🏁 Script executed:
rg -n -A12 -B12 'include_dashboards|class .*Insight.*Serializer|tags.*Serializer|tags =|get_tags' posthog products api 2>/dev/null | head -240Repository: PostHog/posthog
Length of output: 18246
🏁 Script executed:
rg -l 'INSIGHT_DASHBOARDS_OPT_IN_ENFORCED|include_dashboards' posthog products | head -40
rg -n -A10 -B10 'INSIGHT_DASHBOARDS_OPT_IN_ENFORCED|include_dashboards' posthog/api posthog/insights products/analytics products/insights 2>/dev/null | head -240Repository: PostHog/posthog
Length of output: 486
🏁 Script executed:
rg -n -A18 -B18 'include_dashboards|dashboards|tags' products/product_analytics/backend/presentation/insight.py products/product_analytics/backend/tests/api/test_deprecated_dashboards_field.pyRepository: PostHog/posthog
Length of output: 42626
Read back dashboards before reporting success.
The session-authenticated PATCH response omits dashboards, and the generated insight-update request does not pass include_dashboards=true. A dashboard-only update can therefore skip the current comparison and report success without validating persistence. Read back the insight through insight-get with include_dashboards=true, then compare the returned dashboard IDs before returning success.
There was a problem hiding this comment.
Fixed in 0e3df84. The check reads dashboard ids from dashboard_tiles (non-deleted tiles). The response always includes this field, and the backend clears its tile cache after the update. So no second read is needed. It uses dashboards only when dashboard_tiles is not present. A new test case covers a dashboards mismatch with no dashboards field in the response. The success test now asserts the PATCH body.
The PATCH response can omit the deprecated `dashboards` field, so a dashboards-only update skipped the persistence check. The check now reads dashboard ids from `dashboard_tiles`, which the response always includes. The test also asserts the PATCH body. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 52fe957e-c02e-4ed1-b620-cbebd01a7b05
A new stamphog review started for this PR — the fresh verdict replaces this approval.
There was a problem hiding this comment.
Approved.
This is a contained MCP tool wrapper that follows the existing update-feature-flag override pattern. It sits outside risky territory, has focused tests, and can be reverted easily. The generated tool it wraps exists.
- Minor: the post-PATCH comparison could report an error for a change that did save, for example if the API filters dashboards the caller cannot access. This affects only the MCP tool's messaging, so it is not blocking.
- CodeRabbit nitpick, non-blocking: the success test already asserts the PATCH body fields.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 102L, 2F substantive, 205L/4F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1c-medium (205L, 4F, single-area, fix) |
| stamphog 2.3.0 | .stamphog/policy.yml @ 0e3df84 · reviewed head 0e3df84 |
|
Reply to the stamphog note about false errors on dashboards: no change is needed. |
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, please remove the |
|
This PR was closed due to lack of activity. Feel free to reopen if it's still relevant. |
Problem
insight-updatecan get a success response while no requested field changes. The agent cannot tell that the edit failed, so the task stays blocked.{"id": 7, "insight": {"name": ...}}) or a misnamed field (title) reaches the handler as{id}only.saved=True,is_sample=False) and returns it with onlyupdated_atchanged.Origin
Changes
insight-updatewraps the generated handler, the same wayupdate-feature-flagdoes.name,description,favorited,tags, anddashboards. It returns an error that names the fields that did not persist.dashboard_tiles, because the response can omit the deprecateddashboardsfield. It does not comparequery, because the API normalizes the stored query.Note
Nothing changes on the backend. An empty PATCH from a non-MCP caller still changes
updated_at. The MCP tool no longer sends one.How did you test this code?
services/mcp/tests/unit/insight-update-persistence.test.ts. It sends the input through the tool's own schema, so the tests see what zod strips:nameandtagsgets an error that names both fields.tool-name-validation.test.ts:insight-updateis now on the list of generated tools that the override shadows on purpose.services/mcpunit suite locally. The repo-wide typecheck failed only on missingreactand@posthog/quillpackages in the sandbox. None of those errors are in the changed files.Release status
Automatic notifications
Docs update
None.
🤖 Agent context
Autonomy: Fully autonomous
Agent: PostHog Desktop (Claude Code), Claude Opus 5.5 (
claude-opus-5-5)/writing-tests.gh pr list --search "insight-update mcp") found no open PR for this symptom.updated_atchanged, no error".Created with PostHog Desktop from this inbox report.
🤖 Generated with Claude Code