feat(subscriptions): AI subscription API, validation, telemetry, and UI#59632
Draft
vdekrijger wants to merge 2 commits into
Draft
feat(subscriptions): AI subscription API, validation, telemetry, and UI#59632vdekrijger wants to merge 2 commits into
vdekrijger wants to merge 2 commits into
Conversation
This was referenced May 22, 2026
Contributor
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
|
🎭 Playwright didn't run on this PR — your changes touch code that could affect E2E behavior, but Playwright is opt-in via label now to keep CI cost down. Add the Most PRs don't need this. Real regressions still get caught on master and fix-forward. |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Contributor
|
Size Change: +5.2 kB (0%) Total Size: 119 MB 📦 View Changed
ℹ️ View Unchanged
|
Contributor
🔍 Migration Risk AnalysisWe've analyzed your migrations for potential risks. Summary: 0 Safe | 0 Needs Review | 1 Blocked ❌ BlockedCauses locks or breaks compatibility 📚 How to Deploy These Changes SafelyAddField: This operation acquires a brief lock but doesn't rewrite the table. Deployment uses lock timeouts with automatic retries, so lock contention will cause retries rather than connection pile-up. RunPython: Use batching for large data migrations:
See the migration safety guide Last updated: 2026-05-27 07:48 UTC (af2c303) |
Contributor
|
👋 Visual changes detected for this PR. Review and approve in PostHog Visual Review If these changes are unexpected, they may be caused by a flaky test or a broken snapshot on master. Don't approve — rerun the job or wait for a fix. |
vdekrijger
commented
May 26, 2026
| _AI_CONFIG_ALLOWED_KEYS = frozenset({"model", "planner_model"}) | ||
|
|
||
|
|
||
| def _validate_ai_config_dict(value): |
Contributor
Author
There was a problem hiding this comment.
As mentioned this and the AI_CONFIG_ALLOWED_KEYS should all be removed as I think we should remove that field to begin with.
Contributor
Author
There was a problem hiding this comment.
Similar to the ai config fields below, will ignore flagging them and consider this one to be the main signal.
Contributor
Author
There was a problem hiding this comment.
Done in af2c303 — removed the API-layer ai_config usage and the AI_CONFIG_ALLOWED_KEYS import. The model field + migration removal belongs in #59634 (where it was added), so I scoped that out of this PR.
|
|
||
|
|
||
| def _ai_create_gate_reason(organization, *, kind: str = "subscriptions", verb: str = "creating") -> Optional[str]: | ||
| """Returns the human-readable reason why creating an AI subscription / ad-hoc AI |
Contributor
Author
There was a problem hiding this comment.
Please simplify this doc! It's a bit too verbose.
Contributor
Author
There was a problem hiding this comment.
Simplified in af2c303 — trimmed the docstring/comment down to the essential rationale.
|
|
||
|
|
||
| class AiReportRequestSerializer(serializers.Serializer): | ||
| """Input for the ad-hoc AI report endpoint — same prompt validation as a scheduled AI subscription.""" |
Contributor
Author
There was a problem hiding this comment.
Wait what is this? Why do we have an ad hoc AI report endpoint? why isn't everything part of the prompt / scheduled sub with a possibility to "fire off now"?
Contributor
Author
There was a problem hiding this comment.
Removed in af2c303 — dropped the ad-hoc ai_report action and its serializers entirely. Everything now goes through the scheduled prompt subscription path.
| if existing is None and "content_type" not in attrs: | ||
| attrs["content_type"] = content_type | ||
|
|
||
| if content_type == Subscription.ContentType.INSIGHT: |
Contributor
Author
There was a problem hiding this comment.
This entire validation fiesta is new right? I think we can do better here to avoid the if elif stuff that it's currently looking like. Also I think that the content type fallbacks might not be needed as we just need ot ensure it's always set in the migration PR.
Contributor
Author
There was a problem hiding this comment.
Refactored in af2c303 — replaced the if/elif chain with a dict-dispatch on content_type (one validator per type) plus a fail-soft fallback that returns a 400 instead of a 500 on an unexpected value (e.g. a stale row).
| # on the first scheduled run is a poor first impression — fail fast here. | ||
| effective_target_type = attrs.get("target_type") or (existing.target_type if existing else None) | ||
| if effective_target_type and effective_target_type not in ( | ||
| Subscription.SubscriptionTarget.EMAIL, |
Contributor
Author
There was a problem hiding this comment.
I thought we already had checks like this at creation time?
Contributor
Author
There was a problem hiding this comment.
Confirmed — folded into the create-time gate in af2c303 rather than duplicating the check.
| </LemonBanner> | ||
| )} | ||
| <LemonBanner type="info" className="text-sm"> | ||
| The AI plans up to 3 HogQL queries against your project's events and writes a |
Contributor
Author
There was a problem hiding this comment.
Does it make sense to tell these implementation details:? if it ever deviates, this risks becoming outdated, I wonder if we can instead keep it more abstract?
Contributor
Author
There was a problem hiding this comment.
Agreed — abstracted in af2c303. The banner copy no longer names pipeline internals (HogQL / query count) so it won't go stale if the implementation changes.
| * (turns red at the cap), so we don't add our own. Example chips sit beneath | ||
| * the textarea on their own row so they wrap cleanly without competing with | ||
| * the counter for horizontal space. | ||
| */} |
Contributor
Author
There was a problem hiding this comment.
No need for this comment.
Contributor
Author
There was a problem hiding this comment.
Removed in af2c303.
| ), | ||
| render: (_value: unknown, sub: SubscriptionApi) => { | ||
| // All three kinds render as neutral grey LemonTags differentiated by icon, not | ||
| // colour — status/accent colours (red, green, purple) would read as |
Contributor
Author
There was a problem hiding this comment.
This comment feels unneeded as well
Contributor
Author
There was a problem hiding this comment.
Removed in af2c303.
| Mine = 'mine', | ||
| Dashboard = 'dashboard', | ||
| Insight = 'insight', | ||
| Ai = 'ai_prompt', |
Contributor
Author
There was a problem hiding this comment.
Please capitalise the I in AI as well.
Contributor
Author
There was a problem hiding this comment.
Done in af2c303 — Ai → AI across the enum and its references.
| @@ -0,0 +1,567 @@ | |||
| /** | |||
| * Auto-generated from the Django backend OpenAPI schema. | |||
Contributor
Author
There was a problem hiding this comment.
These files indicate that we had no MCP behaviour yet for the subscription flow? Or what is the goal of all these auto generated files?
Contributor
Author
There was a problem hiding this comment.
These are the auto-generated API types (Orval from the OpenAPI spec) for the subscriptions product module, regenerated here because the serializer changed — not hand-edited. They're produced by hogli build:openapi. No MCP behaviour changed in this PR.
Fixes from the review-swarm pass: fail-soft content_type dispatch (400 not 500 on unexpected values), persist trimmed prompt, add type annotations to validate() helpers, field-keyed validator errors, SLO content_type assertion guard, stale comment cleanup.
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
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
Changes
How did you test this code?
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Publish to changelog?
Docs update
🤖 Agent context