feat(llma): wire up auto-create hook, generated types, and MCP tools#54367
Open
andrewm4894 wants to merge 1 commit intoandy/llma-eval-reports-4-frontendfrom
Open
feat(llma): wire up auto-create hook, generated types, and MCP tools#54367andrewm4894 wants to merge 1 commit intoandy/llma-eval-reports-4-frontendfrom
andrewm4894 wants to merge 1 commit intoandy/llma-eval-reports-4-frontendfrom
Conversation
This was referenced Apr 13, 2026
Contributor
|
Hey @andrewm4894! 👋\nThis pull request seems to contain no description. Please add useful context, rationale, and/or any other information that will help make sense of this change now and in the distant Mars-based future. |
Member
Author
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Contributor
MCP UI Apps size report
|
Contributor
|
Size Change: 0 B Total Size: 129 MB ℹ️ View Unchanged
|
Contributor
Prompt To Fix All With AIThis is a comment left during a code review.
Path: products/llm_analytics/frontend/generated/api.ts
Line: 548-557
Comment:
**Wrong return type for `runs` endpoint**
The backend's `runs` action returns `EvaluationReportRunSerializer(runs, many=True).data` — a JSON array of run objects — but this generated function types the response as `Promise<EvaluationReportApi>` (a single report config object). Any frontend code calling this will receive an array but TypeScript will treat it as a single `EvaluationReportApi`.
The root cause is that `EvaluationReportViewSet.runs` in `evaluation_reports.py` is missing `@extend_schema(responses=EvaluationReportRunSerializer(many=True))`, so drf-spectacular fell back to the viewset's default serializer class. Adding that decorator and regenerating will produce the correct list type here.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: products/llm_analytics/backend/api/evaluations.py
Line: 235-246
Comment:
**Auto-create and evaluation save are not in the same transaction**
`serializer.save()` commits the evaluation in its own implicit transaction. If `EvaluationReport.objects.create()` then raises (e.g. a DB error or constraint violation), the user receives a 500 but the evaluation row is already persisted. They will likely retry, creating a duplicate evaluation, while the original silently exists without any report.
Wrap both operations in `transaction.atomic()` so a failed report creation rolls back the evaluation too:
```python
from django.db import transaction
def perform_create(self, serializer):
with transaction.atomic():
instance = serializer.save()
from ..models.evaluation_reports import EvaluationReport
EvaluationReport.objects.create(
team=self.team,
evaluation=instance,
start_date=instance.created_at,
)
# tracking/analytics below can remain outside the transaction
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: products/llm_analytics/backend/api/evaluations.py
Line: 240
Comment:
**Deferred import should be at module level**
There's no circular-import risk here — `evaluation_reports.py` only imports from `posthog.models.utils` and standard library modules. Move this to the top of the file alongside the other `..models.*` imports, using the same relative-import style:
```suggestion
from ..models.evaluation_reports import EvaluationReport
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "feat(llma): wire up auto-create hook, ge..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+548
to
+557
| export const llmAnalyticsEvaluationReportsRunsRetrieve = async ( | ||
| projectId: string, | ||
| id: string, | ||
| options?: RequestInit | ||
| ): Promise<EvaluationReportApi> => { | ||
| return apiMutator<EvaluationReportApi>(getLlmAnalyticsEvaluationReportsRunsRetrieveUrl(projectId, id), { | ||
| ...options, | ||
| method: 'GET', | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
runs endpoint
The backend's runs action returns EvaluationReportRunSerializer(runs, many=True).data — a JSON array of run objects — but this generated function types the response as Promise<EvaluationReportApi> (a single report config object). Any frontend code calling this will receive an array but TypeScript will treat it as a single EvaluationReportApi.
The root cause is that EvaluationReportViewSet.runs in evaluation_reports.py is missing @extend_schema(responses=EvaluationReportRunSerializer(many=True)), so drf-spectacular fell back to the viewset's default serializer class. Adding that decorator and regenerating will produce the correct list type here.
Prompt To Fix With AI
This is a comment left during a code review.
Path: products/llm_analytics/frontend/generated/api.ts
Line: 548-557
Comment:
**Wrong return type for `runs` endpoint**
The backend's `runs` action returns `EvaluationReportRunSerializer(runs, many=True).data` — a JSON array of run objects — but this generated function types the response as `Promise<EvaluationReportApi>` (a single report config object). Any frontend code calling this will receive an array but TypeScript will treat it as a single `EvaluationReportApi`.
The root cause is that `EvaluationReportViewSet.runs` in `evaluation_reports.py` is missing `@extend_schema(responses=EvaluationReportRunSerializer(many=True))`, so drf-spectacular fell back to the viewset's default serializer class. Adding that decorator and regenerating will produce the correct list type here.
How can I resolve this? If you propose a fix, please make it concise.
Member
Author
There was a problem hiding this comment.
Good catch — the runs action is indeed missing an @extend_schema annotation, so drf-spectacular falls back to the viewset default. Will add @extend_schema(responses=EvaluationReportRunSerializer(many=True)) to the runs action and regenerate the product-scoped types.
Comment on lines
235
to
+246
| def perform_create(self, serializer): | ||
| instance = serializer.save() | ||
|
|
||
| # Auto-create a default report config so reports are generated from the start. | ||
| # Users can later add email/Slack delivery targets if they want notifications. | ||
| from products.llm_analytics.backend.models.evaluation_reports import EvaluationReport | ||
|
|
||
| EvaluationReport.objects.create( | ||
| team=self.team, | ||
| evaluation=instance, | ||
| start_date=instance.created_at, | ||
| ) |
Contributor
There was a problem hiding this comment.
serializer.save() commits the evaluation in its own implicit transaction. If EvaluationReport.objects.create() then raises (e.g. a DB error or constraint violation), the user receives a 500 but the evaluation row is already persisted. They will likely retry, creating a duplicate evaluation, while the original silently exists without any report.
Wrap both operations in transaction.atomic() so a failed report creation rolls back the evaluation too:
from django.db import transaction
def perform_create(self, serializer):
with transaction.atomic():
instance = serializer.save()
from ..models.evaluation_reports import EvaluationReport
EvaluationReport.objects.create(
team=self.team,
evaluation=instance,
start_date=instance.created_at,
)
# tracking/analytics below can remain outside the transactionPrompt To Fix With AI
This is a comment left during a code review.
Path: products/llm_analytics/backend/api/evaluations.py
Line: 235-246
Comment:
**Auto-create and evaluation save are not in the same transaction**
`serializer.save()` commits the evaluation in its own implicit transaction. If `EvaluationReport.objects.create()` then raises (e.g. a DB error or constraint violation), the user receives a 500 but the evaluation row is already persisted. They will likely retry, creating a duplicate evaluation, while the original silently exists without any report.
Wrap both operations in `transaction.atomic()` so a failed report creation rolls back the evaluation too:
```python
from django.db import transaction
def perform_create(self, serializer):
with transaction.atomic():
instance = serializer.save()
from ..models.evaluation_reports import EvaluationReport
EvaluationReport.objects.create(
team=self.team,
evaluation=instance,
start_date=instance.created_at,
)
# tracking/analytics below can remain outside the transaction
```
How can I resolve this? If you propose a fix, please make it concise.
Member
Author
There was a problem hiding this comment.
Valid point. Wrapping both in transaction.atomic() is the right call — if the report creation fails we shouldn't leave an orphaned evaluation. Will fix.
|
|
||
| # Auto-create a default report config so reports are generated from the start. | ||
| # Users can later add email/Slack delivery targets if they want notifications. | ||
| from products.llm_analytics.backend.models.evaluation_reports import EvaluationReport |
Contributor
There was a problem hiding this comment.
There's no circular-import risk here — evaluation_reports.py only imports from posthog.models.utils and standard library modules. Move this to the top of the file alongside the other ..models.* imports, using the same relative-import style:
Suggested change
| from products.llm_analytics.backend.models.evaluation_reports import EvaluationReport | |
| from ..models.evaluation_reports import EvaluationReport |
Prompt To Fix With AI
This is a comment left during a code review.
Path: products/llm_analytics/backend/api/evaluations.py
Line: 240
Comment:
**Deferred import should be at module level**
There's no circular-import risk here — `evaluation_reports.py` only imports from `posthog.models.utils` and standard library modules. Move this to the top of the file alongside the other `..models.*` imports, using the same relative-import style:
```suggestion
from ..models.evaluation_reports import EvaluationReport
```
How can I resolve this? If you propose a fix, please make it concise.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!
Member
Author
There was a problem hiding this comment.
Agreed, no circular import risk here. Will move to module-level alongside the other ..models.* imports.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d5be7f452d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
Comment on lines
+552
to
+553
| ): Promise<EvaluationReportApi> => { | ||
| return apiMutator<EvaluationReportApi>(getLlmAnalyticsEvaluationReportsRunsRetrieveUrl(projectId, id), { |
There was a problem hiding this comment.
llmAnalyticsEvaluationReportsRunsRetrieve is typed as returning EvaluationReportApi, but the backend runs action actually returns a list of report-run records (EvaluationReportRunSerializer(..., many=True) in products/llm_analytics/backend/api/evaluation_reports.py). This mismatch means frontend callers generated from this SDK will treat an array payload like a single object, which can lead to incorrect UI logic or runtime undefined field access once this endpoint is used.
Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment.
Same issue as the greptile comment above — missing @extend_schema(responses=EvaluationReportRunSerializer(many=True)) on the runs action. Will fix together with that one.
andrewm4894
force-pushed
the
andy/llma-eval-reports-4-frontend
branch
from
2eb779d to
da19b28
Compare
andrewm4894
force-pushed
the
andy/llma-eval-reports-5-wiring-generated
branch
2 times, most recently
from
2f4ca4d to
6243131
Compare
andrewm4894
force-pushed
the
andy/llma-eval-reports-4-frontend
branch
from
da19b28 to
5859546
Compare
andrewm4894
force-pushed
the
andy/llma-eval-reports-5-wiring-generated
branch
from
6243131 to
fd3fff7
Compare
andrewm4894
force-pushed
the
andy/llma-eval-reports-4-frontend
branch
2 times, most recently
from
409b90c to
d8a5ccf
Compare
andrewm4894
force-pushed
the
andy/llma-eval-reports-5-wiring-generated
branch
from
fd3fff7 to
2ee84b8
Compare
Auto-create an EvaluationReport when an evaluation is created via perform_create hook. Add product-scoped generated API types and MCP tool definitions for evaluation reports. Note: global generated files (schema.json, schema.py, snapshots, MCP codegen) should be regenerated via `hogli build:openapi` after merge rather than cherry-picked.
andrewm4894
force-pushed
the
andy/llma-eval-reports-5-wiring-generated
branch
from
2ee84b8 to
ef36efc
Compare
andrewm4894
force-pushed
the
andy/llma-eval-reports-4-frontend
branch
from
d8a5ccf to
cb89a1b
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.
Problem
With the backend pipeline and frontend in place, new evaluations should automatically get a default report configuration, and MCP tools should expose report operations.
Part 5 of 5 in a stacked PR series. Depends on #54366.
Stack:
Changes
perform_createhook inEvaluationViewSetauto-creates anEvaluationReportwithfrequency=every_n,trigger_threshold=100,enabled=True, empty delivery targetsapi.schemas.ts,api.ts)Note: Global generated files (schema.json, schema.py, snapshots, MCP codegen) should be regenerated via
hogli build:openapiafter merge.How did you test this code?
The auto-create hook is covered by existing API tests. Generated types are mechanical output from serializer definitions.
No
Docs update
skip-inkeep-docs
🤖 LLM context
Agent-assisted PR stack creation from a single reference branch.