Repository navigation
Conversation
|
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 |
|
Important Review skippedWe couldn't safely recover the incremental review. No full review was started, and the last reviewed checkpoint was preserved. Retry later, or explicitly request a full review by commenting You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a feature-flagged AI decision action to workflow editing. The Node.js template validates inputs and sends requests to a new authenticated backend endpoint. The endpoint checks AI data-processing approval and calls Jev. The action exposes decision and confidence outputs, and workflow validation enforces option limits. Priority: ⬇️ Low Merge Risk: 🔵 Low · up to A persistent malformed AI response can cause repeated billed requests for one workflow step. This is a bounded risk to address or explicitly accept before enabling the feature. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to Access is restricted to project-scoped service requests, and organizational approval is required before AI processing. No introduced security failure was established, but production configuration and the effects of repeated requests remain unconfirmed. 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❌ Failed checks (1 warning)
Full details: Description checkExplanation The description includes the required sections, but it describes a different classification feature. It names the wrong feature flag and JWT purpose, claims different inputs and outputs, and refers to different routes and tests than this AI decision change. It also omits the required before-flow diagram. Resolution Update the Problem and Changes sections to describe the AI decision step, its question/context/options inputs, decision and confidence outputs, actual feature flag, JWT purpose, route, and test names. Replace the classification screenshots and flow details with accurate material. Add separate before-and-after Mermaid flowcharts, and verify the test and release-status details against this change. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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)
products/workflows/backend/presentation/views/workflow_ai_decisions.py-145-148 (1)
145-148: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winKeep malformed gateway responses out of the retryable 503 path.
When the gateway returns a missing or malformed answer,
parse_system_one_responseraisesSystemOneRequestFailedwithout a status code. This handler maps that error to 503, andcallInternalApiretries 503 up to three times. A gateway that keeps returning malformed answers can therefore trigger up to three billed model requests for one workflow action. Distinguish malformed-response failures from network failures and return a non-retriable result for malformed responses.
🧹 Nitpick comments (1)
products/workflows/backend/tests/api/test_workflow_ai_decisions_api.py (1)
122-125: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winTest rejection of a valid token without
hog_flow_id.The endpoint rejects a missing workflow claim, but no API test protects this check. Add a correctly scoped token without the claim, then assert a 401 and that Jev construction does not run.
Suggested test
+ def test_rejects_a_token_without_a_workflow_claim(self) -> None: + token = encode_jwt( + {"team_id": self.team.id}, + timedelta(minutes=5), + PosthogJwtAudience.WORKFLOW_AI_DECISION, + signing_key=SECRET, + ) + with patch(_BUILD) as build: + response = self._post(token=token) + + assert response.status_code == status.HTTP_401_UNAUTHORIZED + build.assert_not_called()
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
- Review profile: QUIET
- Plan: Enterprise
- Run ID:
175e76a7-c0c4-46db-b71f-245c6ce8f2d6
📒 Files selected for processing (21)
frontend/src/lib/constants.tsxnodejs/src/cdp/async-functions/ai-decision.test.tsnodejs/src/cdp/async-functions/ai-decision.tsnodejs/src/cdp/async-functions/index.tsnodejs/src/cdp/async-functions/internal-api-call.tsnodejs/src/cdp/config.tsnodejs/src/cdp/templates/_destinations/posthog_workflows/posthog-ai-decision.template.tsnodejs/src/cdp/templates/index.tsnodejs/src/cdp/utils/jwt-utils.tsnodejs/tests/fixtures/hog-bytecode/76959e3b7e5dee74f023dde37eedcfa8e23115b4284a9c903e3698c72adac389.jsonposthog/cdp/flag_gated_templates.pyposthog/jwt.pyposthog/settings/data_stores.pyproducts/workflows/backend/facade/service_jwt.pyproducts/workflows/backend/presentation/views/hog_flow.pyproducts/workflows/backend/presentation/views/workflow_ai_decisions.pyproducts/workflows/backend/routes.pyproducts/workflows/backend/service_jwt.pyproducts/workflows/backend/tests/api/test_hog_flow.pyproducts/workflows/backend/tests/api/test_workflow_ai_decisions_api.pyproducts/workflows/frontend/Workflows/hogflows/panel/HogFlowEditorPanelBuild.tsx
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 6 remain after this review.
323249a to
ecb4772
Compare
ecb4772 to
1c2d0fe
Compare
|
Risk: No findings Since the last review, the PR's substantive change is the feature-flag gate on the classification route: Sentinel reviewed |
👀 Auto-assigned reviewersThese soft owners were skipped because they only have minor changes here. Nothing blocks merge, so self-assign if you'd like a look:
Soft owners come from each directory's |
1c2d0fe to
3957d7e
Compare
|
Note 🤖 Claude Opus 5.5 responding on behalf of Michael @coderabbitai both findings from your review body are fixed in 3957d7e.
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
fd -t f 'workflow_(classifications|ai_decisions).*\.py$' .
fd -t f 'workflow_(classifications|ai_decisions).*\.py$' . --exec ast-grep outline {} --items all
rg -n -C 12 --type py 'def _is_transient\b|malformed_answer|def test_rejects_a_token_without_a_workflow_claim\b' products/workflows
rg -n -C 8 --type py 'hog_flow_id|_is_transient\(' products/workflows/backend/presentation/views/workflow* 2>/dev/nullLength of output: 46072
I checked the source only; I did not run the tests. No further changes are needed for these two findings. You are interacting with an AI system. |
The worker used the default 3 second internal fetch budget, but Django gives the gateway 5 seconds and maps a gateway timeout to a 503 for the worker to retry. The worker aborted first, discarded late answers, and sent the same classification again. callInternalApi now accepts an optional per-attempt timeoutMs. The classify step passes 7 seconds. Other callers keep the default. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 8cb2ba00-a783-4633-a0ce-424140c553eb
The AI usage report groups workflow AI spend by the team_id event property. The classify call sent only hog_flow_id, so any billable Jev generation grouped under team 0. The call now sends team_id with the other labels, as the Anthropic gateway client does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 8cb2ba00-a783-4633-a0ce-424140c553eb
The classification endpoint refuses more than 16 categories or a description over 500 characters with a 400, so such a step failed on every run. A strict workflow save now runs the endpoint's own categories serializer on the step inputs. The mocked test run checks the same limits, so the test panel no longer reports success for inputs a live run rejects. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 8cb2ba00-a783-4633-a0ce-424140c553eb
The classification endpoint mapped a gateway rejection to 502, which the worker retries, so a permanent rejection reached the gateway three times. A gateway 5xx also got the rejection message that blames the step inputs. A rejection now returns 422, which the worker does not retry, and a gateway 5xx returns 503 with the busy message. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 8cb2ba00-a783-4633-a0ce-424140c553eb
The classification endpoint forwarded any context size to the gateway. It now rejects a context over 65,536 characters of JSON with a 400 that names the limit, the same state limit the ml_inference decide API uses for the same model. The worker checks the same limit, so a mocked test run fails on the same input as a live run. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 8cb2ba00-a783-4633-a0ce-424140c553eb
The classification view imported posthog.llm.system_one_client at module level, which pulls the OpenAI and Anthropic SDKs into Django startup through the route registry and fails the startup import budget. The view now imports it in the request path and keeps its own copy of the 16-category limit. It also passes team_id to build_system_one_client, which labels the gateway event and derives the distinct ID itself. The route test patches the client where the view now looks it up. Co-authored-by: Harley Alexander <harley.a@posthog.com> Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The request field named context shadows DRF's Field.context in the type stubs, so repo-wide mypy fails. DRF pops declared fields off the class, so the runtime is unaffected. The ignore follows the signals scout rubric serializers. Co-authored-by: Harley Alexander <harley.a@posthog.com> Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A question over 2,000 characters passed a strict save and the mocked test run, then failed every live run with a 400 from the classification route. The worker's checks and the save-time validator now apply the route's question limit too. A category row with a description but no name reached Jev as a category named "", so a classification could match no branch. The route and the worker now ask for a name on every category. Co-authored-by: Harley Alexander <harley.a@posthog.com> Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…tion Every gateway refusal told the author to check the step inputs, including an out-of-credits 402 or a gateway key problem that the inputs cannot fix. The step now fails with the gateway status instead. Co-authored-by: Harley Alexander <harley.a@posthog.com> Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A malformed gateway answer raised the same error as a network failure, so the endpoint returned 503 and the worker asked Jev up to three times. Only an unreached gateway is transient now; an unreadable answer returns 422. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
3957d7e to
b07fd97
Compare
|
Note 🤖 GPT-6.1 Sol responding on behalf of Michael Replaced by the draft fork PR #366 in its native fork stack. @meikelmosby will start migrating the stack from the fork into PostHog/posthog for upstream review and CI. Closing this master-targeted copy to keep the handoff clear. |
Revives the "Classify with Jev" step from #107832 and #111710 on current master. The classification endpoint asks the model through the ml_inference facade with privacy mode on, and rejects answers it cannot branch on. The step and its endpoint stay behind the workflow-jev-classify-action flag. Generated-By: PostHog Desktop Task-Id: 8ed258e7-0201-436d-9625-5743d7c23174
Revives the "Classify with Jev" step from #107832 and #111710 on current master. The classification endpoint asks the model through the ml_inference facade with privacy mode on, and rejects answers it cannot branch on. The step and its endpoint stay behind the workflow-jev-classify-action flag. Generated-By: PostHog Desktop Task-Id: 8ed258e7-0201-436d-9625-5743d7c23174
Revives the "Classify with Jev" step from #107832 and #111710 on current master. The classification endpoint asks the model through the ml_inference facade with privacy mode on, and rejects answers it cannot branch on. The step and its endpoint stay behind the workflow-jev-classify-action flag. Generated-By: PostHog Desktop Task-Id: 8ed258e7-0201-436d-9625-5743d7c23174
Problem
Note
🔵 Merge risk: low (merge-risk rubric)
posthog:workflows:classify).Changes
workflow-jev-classify-actionflag on, the builder palette offers "Classify with Jev".{category, confidence, probabilities}. The palette entry maps them to thejev_categoryandjev_confidencevariables.jev_categorywith the regular conditional branch.Step configuration with output variables
instructionsand the context only asstate, so event data never becomes instructions.build_system_one_clientwithai_product="workflows"and the team, on the gateway only. No TypeSafe fallback.WORKFLOW_CLASSIFY_JWT_SECRETon web and the CDP worker.contextfield name.Not in v1: a native action type, an Unsure path, thresholds, token-bucket admission, a credit pre-check (a gateway 402 surfaces as an error), a byte cap beyond the character cap, and builder UI beyond the palette entry.
How did you test this code?
products/workflows/backend/tests/api/test_workflow_classifications_api.py.TestJevClassifyActionValidationintest_hog_flow.py.nodejs/src/cdp/async-functions/classify.test.ts.posthog/test/repo_invariants/, the async-function jest suites, repo-wide mypy, frontend and Node typechecks, diff-aware semgrep,hogli ci:preflight --strict.Test rationale: #107832's two seams. The route tests catch a context leaking into instructions, a missing approval check, and a retry status mapped to a failure. The jest tests catch the worker and the route disagreeing on limits or the JWT audience.
End-to-end proof against a local Django server
"step" lines run
postHogClassifyitself, so the worker mints the token. "raw" lines post with tokens minted by the worker'sScopedServiceJwt.Release status
Keep the flag off until the CDP worker with the new async function deploys and both sides have
WORKFLOW_CLASSIFY_JWT_SECRET.Automatic notifications
Docs update
None. The step is hidden behind a flag.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Agent: Claude Code, claude-opus-5-5
Co-authored-by./tdd,/improving-drf-endpoints,/writing-ui-components,/writing-pr-descriptions,/reviewing-with-coderabbit,/qa-frontend(reel).🤖 Generated with Claude Code