fix(mcp): make event-definition-create idempotent for taxonomy syncs - #108136
posthog[bot] wants to merge 3 commits into
Conversation
Cross-project event-definition syncs re-send definitions that already exist in the target project. The API rejected the duplicate name with a validation error, so the create tool counted the call as a failure even though the definition the agent wanted was already present. On a duplicate-name validation error, adopt the existing definition and apply the supplied metadata, then return it as a success. Genuine rejections (bad metadata, missing scope) still surface as failures. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 21d655ff-c4bc-47d8-b721-2246d1c07105
|
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 |
🦔 PostHog Review reviewed this pull requestFound 1 must fix, 2 should fix, 0 consider. Published 3 findings (view the review). Resolving comments: 1/3 · 1 fixed Safe fixes are committed to the branch; every settled thread gets a reply. This line updates as threads settle. |
posthog
Bot
added
the
skip-agent-review
🤖 CI report✅ Trunk lane — non-backend lane (
|
| File | Comment lines | Added lines |
|---|---|---|
services/mcp/src/api/client.ts |
6 | 32 |
services/mcp/src/tools/projects/createEventDefinition.ts |
3 | 3 |
services/mcp/tests/unit/api-client.test.ts |
2 | 115 |
This check does not block merging. It updates on every push and clears when the share drops.
![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. 📝 WalkthroughWalkthrough
Priority: ➖ Normal Merge Risk: 🔵 Low · up to Duplicate-name creation now updates the existing definition, and supplied tags replace its tag list. The tool annotation should be corrected to mark this as destructive. The risk is small and the fix is quick. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to Duplicate creates can now change an existing definition’s tags and visibility or verification settings. The recovery also needs read access in addition to write access. The requests remain within the selected project and use the caller’s existing credentials, but these contract and permission changes merit review. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
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.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
services/mcp/src/api/client.ts-876-885 (1)
876-885: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winScope duplicate adoption to the requesting team.
When two same-name team-scoped definitions exist in one project, the duplicate create can reach
by_name. That endpoint searches every team in the project. The non-EE lookup can fail withMultipleObjectsReturned. The EE lookup can select another team's definition with.first(). The fallback can therefore fail or update the wrong definition.Resolve the effective definition for the requesting team before applying the metadata. Use the same team/project scope for duplicate validation and lookup. Event names are stored and queried exactly, so normalization is not the issue.
🧹 Nitpick comments (1)
services/mcp/tests/unit/api-client.test.ts (1)
625-626: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the metadata sent by the fallback PATCH.
The test only checks that a PATCH occurs. The mock returns
updatedwithout inspecting the request body, so the test passes if the client omits the metadata. Supplydescription,tags,verified, andhidden, then assert the parsed PATCH body.Suggested test assertion
- data: { description: 'Synced from production', tags: ['acquisition'] }, + data: { + description: 'Synced from production', + tags: ['acquisition'], + verified: true, + hidden: false, + }, ... expect(patchCall).not.toBeUndefined() + expect(JSON.parse(String(patchCall?.[1]?.body))).toEqual({ + description: 'Synced from production', + tags: ['acquisition'], + verified: true, + hidden: false, + })
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 600f5e79-d901-49f0-aa1f-45f2f253cd11
📒 Files selected for processing (5)
services/mcp/schema/tool-definitions-all.jsonservices/mcp/schema/tool-definitions.jsonservices/mcp/src/api/client.tsservices/mcp/src/tools/projects/createEventDefinition.tsservices/mcp/tests/unit/api-client.test.ts
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 9 remain after this review.
|
PostHog Review alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
| return this.projects().updateEventDefinition({ | ||
| projectId, | ||
| eventName, | ||
| data: data ?? {}, | ||
| }) |
There was a problem hiding this comment.
Preserve retry and error handling in the duplicate fallback
Issue description
The new fallback calls updateEventDefinition, which uses raw fetch for its GET and PATCH. A failed GET gets no safe transport retry. A 429 or 5xx response becomes a generic error, so the tool loses the rate-limit hint and reports an internal failure.
Why we think it's a valid issue
- Checked: The fallback at
services/mcp/src/api/client.ts:881-885.updateEventDefinitionatclient.ts:888-938.fetchJsonatclient.ts:598-700.buildApiErroratclient.ts:530-596.handleToolErrorandfindRecoverableApiErrorinservices/mcp/src/lib/errors.ts:464-560. The error class hierarchy aterrors.ts:200,247,284. The create handler's comment on typed errors inservices/mcp/src/tools/projects/createEventDefinition.ts. - Found: The POST goes through
fetchJson(client.ts:862). That path retries 429s on Retry-After or backoff (client.ts:639-677), retries transport faults on GET (client.ts:612-636), and returns typedPostHogRateLimitError,PostHogValidationError, orPostHogApiError(client.ts:542-595). The fallback then callsupdateEventDefinition. That method uses rawthis.fetchfor the by-name GET (client.ts:907) and the PATCH (client.ts:925), with no 429 or transport retry. Each non-ok response becomes a plainnew Error(... statusText)(client.ts:917,931), so the API body,detail, and Retry-After are lost. - Found:
PostHogRateLimitError extends PostHogApiError(errors.ts:247).handleToolErrorreturns a typed 4xx, 429 included, to the agent as a recoverable message and skipscaptureException(errors.ts:522-538). A plainErrorhas no typed cause. It falls through tocaptureExceptionas an internal MCP tool fault, fingerprinted by tool name (errors.ts:565-585). The create handler wraps the result error ascauseexactly so that this classification works (createEventDefinition.ts, comment abovewrapError). The fallback results cannot use that path. - Impact: Concrete trigger: a taxonomy sync bursts duplicate creates. Each duplicate now costs three requests (POST, GET, PATCH), up from one. A 429 on the GET or the PATCH is not retried, even though the POST before it got a retry. The agent gets "Failed to find event definition: Too Many Requests" with no Retry-After hint. The tool counts it as an internal failure and creates error-tracking noise. A 4xx on the PATCH, for example a serializer rejection of the supplied metadata, loses its validation
detailin the same way and is also captured as an internal fault. This works against the PR's goal of lower failure counts on this tool. It also breaks the typed-error contract that the handler relies on. - Found: The raw-fetch weakness in
updateEventDefinitionpredates this PR and also affectsevent-definition-update. This PR is the change that puts it on the create path. The fix goes inupdateEventDefinition(move both calls tofetchJson), so it also fixes the update tool.
Suggested fix
Use fetchJson for the by-name GET and PATCH. Keep transport retries limited to the safe GET. Return typed API errors so the tool can report rate limits and server failures correctly.
Prompt to fix with AI (copy-paste)
## Context
@services/mcp/src/api/client.ts#L881-885
<issue_description>
The new fallback calls `updateEventDefinition`, which uses raw `fetch` for its GET and PATCH. A failed GET gets no safe transport retry. A 429 or 5xx response becomes a generic error, so the tool loses the rate-limit hint and reports an internal failure.
</issue_description>
<issue_validation>
- **Checked:** The fallback at `services/mcp/src/api/client.ts:881-885`. `updateEventDefinition` at `client.ts:888-938`. `fetchJson` at `client.ts:598-700`. `buildApiError` at `client.ts:530-596`. `handleToolError` and `findRecoverableApiError` in `services/mcp/src/lib/errors.ts:464-560`. The error class hierarchy at `errors.ts:200,247,284`. The create handler's comment on typed errors in `services/mcp/src/tools/projects/createEventDefinition.ts`.
- **Found:** The POST goes through `fetchJson` (`client.ts:862`). That path retries 429s on Retry-After or backoff (`client.ts:639-677`), retries transport faults on GET (`client.ts:612-636`), and returns typed `PostHogRateLimitError`, `PostHogValidationError`, or `PostHogApiError` (`client.ts:542-595`). The fallback then calls `updateEventDefinition`. That method uses raw `this.fetch` for the by-name GET (`client.ts:907`) and the PATCH (`client.ts:925`), with no 429 or transport retry. Each non-ok response becomes a plain `new Error(... statusText)` (`client.ts:917,931`), so the API body, `detail`, and Retry-After are lost.
- **Found:** `PostHogRateLimitError extends PostHogApiError` (`errors.ts:247`). `handleToolError` returns a typed 4xx, 429 included, to the agent as a recoverable message and skips `captureException` (`errors.ts:522-538`). A plain `Error` has no typed cause. It falls through to `captureException` as an internal MCP tool fault, fingerprinted by tool name (`errors.ts:565-585`). The create handler wraps the result error as `cause` exactly so that this classification works (`createEventDefinition.ts`, comment above `wrapError`). The fallback results cannot use that path.
- **Impact:** Concrete trigger: a taxonomy sync bursts duplicate creates. Each duplicate now costs three requests (POST, GET, PATCH), up from one. A 429 on the GET or the PATCH is not retried, even though the POST before it got a retry. The agent gets "Failed to find event definition: Too Many Requests" with no Retry-After hint. The tool counts it as an internal failure and creates error-tracking noise. A 4xx on the PATCH, for example a serializer rejection of the supplied metadata, loses its validation `detail` in the same way and is also captured as an internal fault. This works against the PR's goal of lower failure counts on this tool. It also breaks the typed-error contract that the handler relies on.
- **Found:** The raw-fetch weakness in `updateEventDefinition` predates this PR and also affects `event-definition-update`. This PR is the change that puts it on the create path. The fix goes in `updateEventDefinition` (move both calls to `fetchJson`), so it also fixes the update tool.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Use `fetchJson` for the by-name GET and PATCH. Keep transport retries limited to the safe GET. Return typed API errors so the tool can report rate limits and server failures correctly.
</potential_solution>
| const isDuplicateName = error instanceof PostHogValidationError && /already exists/i.test(error.detail) | ||
| if (!isDuplicateName) { | ||
| return createResult | ||
| } | ||
|
|
||
| return this.projects().updateEventDefinition({ | ||
| projectId, | ||
| eventName, | ||
| data: data ?? {}, | ||
| }) |
There was a problem hiding this comment.
Update the integration test before this behavior reaches the CI gate
Issue description
The existing test in services/mcp/tests/tools/projects.integration.test.ts expects a second create with the same name to throw. This branch now returns success. The MCP integration job runs that test and will fail when the PR leaves draft.
Why we think it's a valid issue
- Checked:
services/mcp/tests/tools/projects.integration.test.ts:83-123. The integration configservices/mcp/vitest.integration.config.mts. Theintegration-testsjob in.github/workflows/ci-mcp.yml:341-354. The backend duplicate checkEventDefinitionSerializer.validate_nameatposthog/api/event_definition.py:231-238. The new fallback atservices/mcp/src/api/client.ts:875-885. - Found: The test
should throw error when the event definition already exists(projects.integration.test.ts:116-122) createseventNameand then callscreateTool.handler(context, { eventName })again. It expectsrejects.toThrow(). Thedescribeblock is not skipped, and the config includestests/**/*.integration.test.ts. The PR changes five files: the two schema JSON files,client.ts,createEventDefinition.ts, and the unit testapi-client.test.ts. This integration file is not one of them. - Found: On the second call,
validate_namefinds the row that the first call created. The first call wroteteam_id=view.team_id, and the second lookup uses the same value. The check raisesEvent definition with name '...' already exists(event_definition.py:234-236). That message matches/already exists/i(client.ts:876), so the client runs the by-name GET and PATCH{}. Both return 200, and the handler returns success. Therejects.toThrow()assertion therefore fails. - Found:
ci-mcp.yml:347-354skips the integration job on drafts. It runs the job when the PR is not a draft and ontrunk-merge/*merge-queue branches. The draft PR's CI stays green now, but the job fails as soon as the PR is marked ready and again in the merge queue. The test config hasretry: 1, and a retry cannot fix a failure that happens every run. - Impact: This is a sure CI failure on the merge gate, so the PR cannot land without a test change. The test also asserts the old, non-idempotent contract, so it has to change to cover the new behavior (the second call returns the first definition's
id, with the supplied metadata applied).must_fixis the correct priority.
Suggested fix
Change the integration test to assert that the second call returns the first definition's ID. Also assert that supplied metadata appears in the result.
Prompt to fix with AI (copy-paste)
## Context
@services/mcp/src/api/client.ts#L876-885
<issue_description>
The existing test in `services/mcp/tests/tools/projects.integration.test.ts` expects a second create with the same name to throw. This branch now returns success. The MCP integration job runs that test and will fail when the PR leaves draft.
</issue_description>
<issue_validation>
- **Checked:** `services/mcp/tests/tools/projects.integration.test.ts:83-123`. The integration config `services/mcp/vitest.integration.config.mts`. The `integration-tests` job in `.github/workflows/ci-mcp.yml:341-354`. The backend duplicate check `EventDefinitionSerializer.validate_name` at `posthog/api/event_definition.py:231-238`. The new fallback at `services/mcp/src/api/client.ts:875-885`.
- **Found:** The test `should throw error when the event definition already exists` (`projects.integration.test.ts:116-122`) creates `eventName` and then calls `createTool.handler(context, { eventName })` again. It expects `rejects.toThrow()`. The `describe` block is not skipped, and the config includes `tests/**/*.integration.test.ts`. The PR changes five files: the two schema JSON files, `client.ts`, `createEventDefinition.ts`, and the unit test `api-client.test.ts`. This integration file is not one of them.
- **Found:** On the second call, `validate_name` finds the row that the first call created. The first call wrote `team_id` = `view.team_id`, and the second lookup uses the same value. The check raises `Event definition with name '...' already exists` (`event_definition.py:234-236`). That message matches `/already exists/i` (`client.ts:876`), so the client runs the by-name GET and PATCH `{}`. Both return 200, and the handler returns success. The `rejects.toThrow()` assertion therefore fails.
- **Found:** `ci-mcp.yml:347-354` skips the integration job on drafts. It runs the job when the PR is not a draft and on `trunk-merge/*` merge-queue branches. The draft PR's CI stays green now, but the job fails as soon as the PR is marked ready and again in the merge queue. The test config has `retry: 1`, and a retry cannot fix a failure that happens every run.
- **Impact:** This is a sure CI failure on the merge gate, so the PR cannot land without a test change. The test also asserts the old, non-idempotent contract, so it has to change to cover the new behavior (the second call returns the first definition's `id`, with the supplied metadata applied). `must_fix` is the correct priority.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Change the integration test to assert that the second call returns the first definition's ID. Also assert that supplied metadata appears in the result.
</potential_solution>
The idempotent create path PATCHes an existing definition, and the API replaces its full tag list with the supplied tags. State this in the create tool description and in the shared tags field, so callers know to include existing tags or omit the field to keep them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: cab66b9d-45db-4881-bc70-e68655c4d577
A new stamphog review started for this PR — the fresh verdict replaces this approval.
There was a problem hiding this comment.
Not approved — this change needs a human reviewer.
Re-add the stamphog label to request another review once you have addressed this.
A substantive, still-unresolved review finding is confirmed. The integration test that expects a duplicate create to throw now contradicts the new behavior and will fail CI. Two other concerns are also open: the fallback bypasses the client's retry and error typing, and the by-name lookup may hit the wrong team's definition.
- services/mcp/tests/tools/projects.integration.test.ts (~line 116) still asserts that a second create with the same name rejects. That is now false, so the MCP integration CI job will fail. Update it to assert idempotent success (existing id returned, metadata applied). Flagged as must-fix by @PostHog[bot].
- services/mcp/src/api/client.ts: the duplicate fallback goes through updateEventDefinition, which uses raw fetch. It loses the 429 retry and rate-limit hint and the safe GET retry that fetchJson provides, and it returns generic errors. Flagged by @PostHog[bot].
- CodeRabbit (unresolved): the by_name lookup searches every team in the project. With same-name team-scoped definitions it can raise MultipleObjectsReturned or update another team's definition. Scope the lookup to the requesting team or confirm it cannot happen.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 43L, 6F substantive, 116L/9F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1c-medium (116L, 9F, single-area, fix) |
| stamphog 2.3.0 | .stamphog/policy.yml @ c3bcd45 · reviewed head c3bcd45 |
The idempotent create fallback calls updateEventDefinition, which used raw fetch for the by-name lookup and the PATCH. A 429, a transport fault, or a validation rejection there became a plain Error: no retry, no Retry-After hint, no validation detail, and the tool counted it as an internal fault. Route both calls through fetchJson so they get the same rate-limit and safe-method transport retries as the POST, and return the typed API error. The explicit "not found" message for a missing definition is unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: cab66b9d-45db-4881-bc70-e68655c4d577
There was a problem hiding this comment.
Not approved — this change needs a human reviewer.
Re-add the stamphog label to request another review once you have addressed this.
The integration test that expects a duplicate create to throw was not updated, so it contradicts the new idempotent behavior and will fail the MCP integration job. The must-fix flagged by @PostHog[bot] is still unaddressed on this diff.
- services/mcp/tests/tools/projects.integration.test.ts:116-122 still expects a second create with the same name to reject, which contradicts the new behavior (unresolved must-fix from @PostHog[bot]).
- CodeRabbit flagged that the by_name lookup may not be scoped to the requesting team when same-name definitions exist in several teams of one project. Confirm this is a real case, or reply that it is out of scope.
- Optional: the unit test only checks that a PATCH happens, not the body it sends (description, tags, verified, hidden).
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 86L, 6F substantive, 219L/9F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1c-medium (219L, 9F, single-area, fix) |
| stamphog 2.3.0 | .stamphog/policy.yml @ 1b0e78f · reviewed head 1b0e78f |
|
|
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
services/mcp/schema/tool-definitions.json-451-451 (1)
451-451: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winThe duplicate-name path can replace an existing definition’s tag list, but both tool definitions still signal additive-only updates. MCP defines
destructiveHint: falseas indicating additive-only updates, so set the hint totruein both definitions. (github.com)
services/mcp/schema/tool-definitions.json#L451-L451: set the correspondingannotations.destructiveHinttotrue.services/mcp/schema/tool-definitions-all.json#L4737-L4737: set the correspondingannotations.destructiveHinttotrue.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: bc6f2b99-457a-4b06-99ab-0067b88729e6
📒 Files selected for processing (8)
services/mcp/schema/tool-definitions-all.jsonservices/mcp/schema/tool-definitions.jsonservices/mcp/schema/tool-inputs.jsonservices/mcp/src/api/client.tsservices/mcp/src/schema/tool-inputs.tsservices/mcp/tests/unit/__snapshots__/tool-schemas/event-definition-create.jsonservices/mcp/tests/unit/__snapshots__/tool-schemas/event-definition-update.jsonservices/mcp/tests/unit/api-client.test.ts
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 8 remain after this review.
This branch has not been deployed
1 participant
Problem
event-definition-create. Almost all failures were validation errors, dominated by a few production-to-acceptance sync intents.Origin
signals-scout-mcp-tool-callsChanges
event-definition-createis now idempotent. On a duplicate-name validation error, it adopts the existing definition, applies the supplied metadata (description, tags, verified, hidden) with aPATCH, and returns the definition as a success. A re-run of a taxonomy sync now converges instead of erroring.idempotentHintistrue. Mechanical regeneration of the mirroredtool-definitions-all.jsonentry.How did you test this code?
services/mcp/tests/unit/api-client.test.ts:PATCH, returns the existing definition, and applies the metadata;POST(no fallback update).Release status
Automatic notifications
Docs update
None.
🤖 Agent context
Autonomy: Fully autonomous
Agent: Claude Code, Opus 4.8
event-definition-createtool, the API client, and the DRF event-definition serializer (validate_nameraising the duplicate-name error)./writing-tests(guidance loaded via repo rule).Created with PostHog Desktop from this inbox report.
🤖 Generated with Claude Code