Skip to content

feat(workflows): run ai decision steps in the cdp - #111685

Closed
Silthus wants to merge 37 commits into
PostHog:masterfrom
Silthus:feat/workflows-ai-decision-runtime
Closed

Silthus wants to merge 37 commits into
PostHog:masterfrom
Silthus:feat/workflows-ai-decision-runtime

Conversation

@Silthus

@Silthus Silthus commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Problem

Changes

  • A person who reaches an ai_decision step now goes down the edge of the model's answer. The answer, its probability, every probability, and the model go to mapped output variables.
  • The run log names the answer, every probability, and the model, for example AI decision answered "yes". Probabilities: yes 62%, no 38%. Model: jevk5-fp8-0.2. It never contains the rendered context.
  • How the step picks an answer from the probabilities:
Answer type Unsure off Unsure on
Yes or no Yes at or above yes_threshold, else No Yes at or above yes_threshold, No at or below no_threshold, else Unsure
Pick one The top option; a tie goes to the earlier option Unsure when the top option is below min_pick_probability, else the top option
  • A failed decision follows the "If the decision fails" edge (continue), or ends the run with on_error: abort. The run log names the failure code and the route's message.
  • A busy AI service no longer fails the person. The step parks the run and asks again, with no "Workflow will pause until..." and "Resuming..." pair in the run log:
Route answer Next attempt Gives up
429 Retry-After, capped at 60 s, plus up to 25% jitter 30 minutes after the first attempt, as throttled
503, any 5xx, a timeout, a connection error, or a reply cut off after its status line Retry-After when sent, capped at 60 s, else 2, 4, 8, 16, 32 s after 5 reschedules, as gateway_unavailable
  • A context over 8,192 bytes of JSON fails as state_too_large without a call to Django.
  • Test runs:
    • Mocked (the Test panel default): no call and no AI credits. The step follows mock_answer, or the first answer. The result carries the rendered context and its size for the panel.
    • Real (mock_async_functions: false): one call. A 429 or 503 ends the test with "The AI service is busy. Try again in a moment."
  • The runtime rejects exactly the configs a strict save rejects, down to null characters and broken emoji. One cases table drives both the Django serializer tests and the Node handler tests, and Node CI runs when only that table changes.
  • Docs: the ai_decision section and worked example in the building-workflows graph schema reference, the node type, credit use and mock_answer in the MCP workflows-create and workflows-test-run tools, and a CONTRIBUTING section on a native step that calls Django.
  • Mechanical: the ci-nodejs.yml path filters for the cases table, the regenerated MCP tool definitions, a testRun option and a testOutput result field on the action handler interface, and the client wiring in cdp-services.ts.

Design calls a reviewer may question:

  • 15 s worker timeout, not the spec's 10 s. The ml_inference facade's 5 s timeout applies per httpx phase, so Django can wait 5 s to connect and 5 s to read. A worker that gives up first retries a decision the gateway may still bill.
  • The quiet reschedule reuses routingOnlyReschedule. That flag already suppresses the pause and resume pair for queue routing. A second flag would duplicate the three executor sites that read it.
  • A mocked result is {answer, mocked: true}. It has no probabilities, so nothing in a mock reads like a real answer. The rendered context goes to the test panel only, so a mapped variable cannot hit the 5 KB variable cap in a test that a live run would pass.
  • An option named exactly unsure wins over the Unsure output when it is the mocked answer. Rejecting that name belongs to the save validation.

Note

Rollout: keep workflows-ai-decision off until this PR deploys, and provision WORKFLOW_AI_DECISION_JWT_SECRET for the CDP worker. Without the secret the step fails closed as gateway_unavailable.

How did you test this code?

Test seams (spec seams 3, 4, and 5):

  1. Answer selection, a pure function (answer-selection.test.ts, it.each at threshold boundaries).
  2. The handler inside the existing hogflow executor harness, with internalFetch stubbed (describe('ai_decision') in hogflow-executor.service.test.ts).
  3. Mock and live parity over one cases table (ai_decision_config_cases.json), run by the Node handler tests and by TestAIDecisionConfigCases in test_workflow_ai_decisions_api.py.

Test rationale:

  • Answer selection catches a threshold compared as p * 100 (0.29 * 100 falls under 29), a band edge on the wrong side, and a tie that goes to the later option. No answer selection existed before.
  • The executor tests catch a wrong branch edge, a mapped variable on failure, a missing on_error path, a reschedule outside its bounds, a pause and resume log pair, a call made for an oversized context, a scoped token without the team or workflow claim, and a test run that reschedules. No native step called Django before, so no existing executor test covered this path.
  • The cases table catches a Node check that drifts from the Django serializer, for example a length counted in UTF-16 units instead of code points.
  • Two cases in cdp-api.serial.test.ts catch a test-run API that stops forwarding mock_answer or mock_async_functions to the handler.
  • Mutation checks: removing the quiet reschedule flag, comparing p * 100, and counting UTF-16 units each fail one test.
Red, green, and the local gate
  • Red on the tree before the handler existed: 68 failed, 2 passed (the executor answered Action type 'ai_decision' not supported; selectAnswer threw).
  • Green: the answer selection and executor ai_decision tests pass (73), and the Django cases table passes (38).
  • The whole src/cdp/services/hogflows/ folder and cdp-api.test.ts pass except one email round-trip test, which needs a local SMTP server on port 1025 that this devbox does not run. That test does not touch this change.
  • pnpm typescript:check in nodejs/ reports no errors in changed files. It reports missing @posthog/replay-anonymizer types, a Rust-built package this devbox has not built.
  • Local gate: diff-aware semgrep (devex and security rules) found 0 issues on the changed files. hogli ci:preflight --strict exited 0. Repo-wide uv run mypy --cache-fine-grained . found no issues.

Not checked: a live gateway call, and a run on a deployed CDP worker. Fork CI is held for maintainer approval.

Release status

  • No feature flag controls this change
  • This change is behind a feature flag and is not available to users
  • This change makes a previously flagged feature available to everyone

Automatic notifications

  • Publish to changelog?

Docs update

The building-workflows graph schema reference, the MCP tool descriptions, and products/workflows/CONTRIBUTING.md cover the step in this PR.

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

Agent: Claude Code, claude-opus-5-5 (tests, review, docs, refactor of the handler); Codex, GPT-6.1 Sol (first implementation of the handler, client, and config parse from a written brief)

  • Built test-first from the spec's seams. Skills invoked: /tdd, /writing-tests, /implementing-mcp-tools, /writing-user-facing-copy, /writing-code-comments, /writing-pr-descriptions, then qa-swarm.
  • The token minting, the input parsing, and the mock and live parity cases carry the pattern of feat(workflows): add a classify with jev workflow action #107832's classify.ts, credited to Harley Alexander with a Co-authored-by trailer.
  • CodeRabbit CLI: skipped. The CLI is signed out on this devbox, and no person was available to sign in during this background run.
  • Nothing in this PR comes from customer material. The fixtures are invented.

🤖 Generated with Claude Code

mayteio and others added 30 commits October 4, 2026 17:29
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 step becomes a native AI decision action instead of a function template, so the Django
route, its JWT purpose, audience, and secret take the ai_decision name. The template, its
bytecode fixture, the postHogClassify async function, the callInternalApi timeout option,
the flag-gated template entry, and the frontend flag and toolbar node are removed.

Co-authored-by: Harley Alexander <harley.a@posthog.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The decide route now asks the hosted decision model through ml_inference
decide_when_available, with privacy mode on, ai_product "workflows", the invocation
as trace id, and the workflow and step as labels. The question goes only into the
instructions and the rendered context only into the state.

Before the model call the route checks, in order: the workflows-ai-decision flag for
the organization, AI data processing approval, AI credits, an 8 KiB cap on the compact
UTF-8 state, and a per-team then global token bucket. The answer is an envelope:
200 succeeded with every answer's probability, 200 failed with a terminal code and
run-log message, 429 with Retry-After for admission or gateway throttling, and 503
when the gateway or Redis is unavailable. Logs carry codes only, never the state or a
gateway body.

Co-authored-by: Harley Alexander <harley.a@posthog.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
ai_decision joins the supported action types. A save validates its config with the
same serializer the decide route uses, so a step that saves cannot fail at run time
on its own question, options, or thresholds. Only inputs.context is templated: it
compiles through a fixed server-side inputs schema, and the question and options stay
plain text that the executor never renders.

The branch slot count is one edge per answer plus Unsure when it is on, and a strict
save requires every one of those edges. A new ai_decision is rejected while the
workflows-ai-decision flag is off for the organization, and one an active flow
already holds keeps saving. The test invocation request takes an optional mock_answer
and passes it to the CDP for the mocked walk.

Co-authored-by: Harley Alexander <harley.a@posthog.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…onfigured

get_client raises ImproperlyConfigured, not RedisError, when no Redis URL is set, so the
admission check now answers 503 for both instead of erroring.

Co-authored-by: Harley Alexander <harley.a@posthog.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- A stored ai_decision no longer grandfathers a flag-gated template saved under the
  same action id, and the reverse: each gate keeps its own set of stored action ids.
- A gateway 200 the facade cannot read, and a pick_one answer whose probabilities do
  not match the options or fall outside 0 to 1, fail as model_refused. Any other 4xx
  fails as gateway_unavailable. Only 5xx and an unreachable gateway answer 503, so a
  retry never repays for a failure that repeats.
- A flag evaluation error answers 503 at run time instead of failing the person for
  good. Saves still fail closed.
- The team gets its admission token back when the global bucket denies the request.
- A rendered context may hold null values and lone surrogates.
- answer_type is required, so options sent without it are not saved as a yes or no step.
- 503 logs carry the cause, throttling logs at debug, and the admission budgets are
  checked at startup.

Co-authored-by: Harley Alexander <harley.a@posthog.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
posthog-foss ships without ee, so the billing import now sits inside the guarded
lookup and a missing module fails open like any other lookup error, instead of a 500.

Co-authored-by: Harley Alexander <harley.a@posthog.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A local pnpm install reordered two importer entries, and the previous commit picked it up.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- A lone surrogate in the rendered context becomes U+FFFD before the size check and
  the gateway call, which could not encode it and answered 500.
- A flag the SDK cannot evaluate (definitions not loaded) answers 503 at run time
  instead of failing the person for good.
- The global bucket's Redis failure no longer triggers a refund on the default
  Redis timeouts.
- The route goes through the workflows facade, and the decision contracts move into
  it, so presentation imports only presentation and facade modules. The shared
  config serializer moves to its own validation module.
- products.workflows declares its facade-only dependency on products.ml_inference.
- Throttling carries its source into a route outcome counter.
- Tests prove the gates spend no admission token, the flag rejects a second decision
  on an active flow, and a builder draft saves before its outputs are wired.

Co-authored-by: Harley Alexander <harley.a@posthog.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- A state nested deeper than the decision contract reads fails as model_refused
  before admission, instead of a 500 whose log quoted the state. The call contract
  keeps the state out of its repr.
- The decision contracts carry team_id instead of a Team model, and the service
  loads the team, as the products architecture asks of a facade.
- A strict save checks AI decision edges only when it sends the graph, so an
  unrelated programmatic edit of an unwired builder draft still saves.
- Admission spreads Retry-After over the bucket's refill window, so a throttled
  batch does not retry all at once.
- Terminal failures carry a log-only cause, organization-level codes log at debug,
  and gateway_unavailable no longer blames the customer's deployment.
- Tests prove the dedicated signing key, the credit lookup key, that a throttled
  request never reaches the model, the global bucket's Redis failure, and that a
  flag the SDK cannot evaluate blocks a save.

Co-authored-by: Harley Alexander <harley.a@posthog.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- A strict save requires an AI decision's 'continue' edge unless on_error is abort,
  because a failed decision follows it and the runtime stops without it.
- The /graph endpoint reports AI decision edge errors only through validate_graph,
  together with the rest of the graph's errors.
- A gateway 429 spreads Retry-After so a throttled batch does not retry at once.
- Failure causes that hit every decision on the deployment (gateway status, region
  without decisions) log at warning; per-organization causes stay at debug.
- Tests prove the per-team bucket spends no global token, the state never reaches a
  log on the unreadable-state path, and grandfathering holds when the flag cannot be
  evaluated.

Co-authored-by: Harley Alexander <harley.a@posthog.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- Malformed pick_one options give an AI decision no answer slots instead of a 500
  from the graph checks.
- The team-token refund on a global denial uses the short-timeout admission Redis
  client; token_bucket.refund takes an optional client like consume.
- Failure causes are a typed set with the gateway status in its own field; only
  causes that fail every decision (gateway_unavailable, a region without
  decisions, an answer that does not fit its question) log at warning.
- The route declares its 400 response and says 503 carries no Retry-After; the
  settings comment states the worker bound the right way round.
- Tests use an explicit failure-edge switch, cover abort-on-error without the
  failure edge, and prove an unreadable state spends no admission token.

Co-authored-by: Harley Alexander <harley.a@posthog.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…qa round

- The failure reasons get one named Literal that the route's log rule takes, and
  the option bound has one name in the validation module.
- The log-leak test renders logged exceptions before it looks for the state, and a
  programmatic save that sends only edges is shown to need every answer edge.

Co-authored-by: Harley Alexander <harley.a@posthog.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A strict save accepted an answer or failure edge that pointed at a missing action,
which strands the person at run time just like a missing edge.

Co-authored-by: Harley Alexander <harley.a@posthog.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… logs

Flag and Redis outages answer 503 before admission, so a batch run repeats them for
every person; they log at debug and the outcome counter carries their rate. A test
proves that activating an unwired builder draft still needs every decision edge.

Co-authored-by: Harley Alexander <harley.a@posthog.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… tests

Tests now fail when the refund ignores its short-timeout client, when a
deployment-wide decision failure logs at debug, or when a strict save
rejects a decision for a reason other than its missing 'continue' edge.
The ai_decision action validation runs each serializer once.

Co-authored-by: Harley Alexander <harley.a@posthog.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…e in tests

The billing-missing case swapped sys.modules before the first request, so
running it first evicted the urlconf imports: it failed with a 500 and the
next test patched a fresh facade copy. The config docstring now promises
only what the serializer enforces, and the context check takes an object.

Co-authored-by: Harley Alexander <harley.a@posthog.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A gateway 200 the facade cannot read fails every decision on the
deployment, like an answer that does not fit its question, so it gets its
own reason and logs at warning. Tests now cover a token minted for another
team, the config defaults a save stores, and a blank context field name.

Co-authored-by: Harley Alexander <harley.a@posthog.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…acade

The facade imported the ai decision service at module level, and
django.setup() loads the facade, so every process imported the openai and
anthropic SDKs at boot and the startup import budget guards failed. The
facade now imports the service when a decision runs. Tests cover the log
level of every cause, the Retry-After bounds, and a lenient draft that
keeps an incomplete config.

Co-authored-by: Harley Alexander <harley.a@posthog.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A strict save keeps every config key, so a stored decision that carried a
flag-gated template_id landed in the template grandfathering set, and a
later edit could turn that action id into the gated template without its
flag.

Co-authored-by: Harley Alexander <harley.a@posthog.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…to publish

A stage_draft save on an active flow that sends only actions or only edges
was checked against the other half of the live graph, not the draft's, so
a fully wired draft got a false 400. Publish re-validates the whole draft
strictly, so the early check now waits for it.

The AI decision test helpers take flag_enabled explicitly.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…s-only half

The early check condition now lives in _sends_a_graph_to_check_early.
A test case removes a decision through an actions-only stage and an
edges-only stage, so the edges-only half of the staged skip is proven.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Tests now cover a context template that fails to compile, debug logging
for a disabled flag and a gateway 402, the team Retry-After floor under a
frozen clock, and no Retry-After on any 503. The early edge check comment
names activation, and the flag test reuses the second-decision helpers.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Move helper functions and methods so each definition appears before its
first call, per the repo's Python style rule. Pure moves.

Add a test that a stored non-decision step does not let an ai_decision
saved under its id through the flag gate.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…_refused

The outcome counter labeled a failed decision by its code only, so a
gateway that refuses every request looked the same as bad customer
input. Failed outcomes now also carry their failure reason.

The model_refused message no longer tells the customer to edit the
question when the gateway sent an answer it could not read.

Drop a Redis flush in the decide route tests that the base test case
already does on teardown.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Silthus and others added 6 commits October 5, 2026 04:06
…n skip a person

The runtime follows a step's continue edge for a person its filters skip,
whatever on_error says. A strict save let an ai_decision with filters and
on_error abort through without that edge, so a skipped person's run failed
with "No next action found". The abort waiver now applies only to a step
without filters.

Tests also prove that a signed-in user without a service token cannot
call the decide route, and that the outcome counter labels every outcome.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
An oversized state was repaired for lone surrogates before the size
check, so the reject path copied it several times before admission.
Measure first with surrogatepass, which gives the same size because a
lone surrogate and its U+FFFD replacement both take 3 bytes, and repair
only a state that fits.

The ai_decision config help text now says the continue edge is also
taken when the step's filters skip a person, and when it may be left out.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… edge docstring

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
An ai_decision step now renders its context, asks the workflows decide
route once with a scoped token, and follows the answer's branch edge. A
busy route reschedules the person within the spec's bounds without a
pause and resume pair in the run log. Test runs mock the answer, or ask
once and stop on a busy route. One cases table, read by the Django
serializer tests and the Node handler tests, keeps the runtime's config
checks equal to a strict save.

Co-authored-by: Harley Alexander <harley.a@posthog.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…butors

The graph schema reference describes the ai_decision node, its edge
indexes, its result, and a worked example. The MCP create and test-run
tools name the node, its AI credit use and mock_answer. CONTRIBUTING
covers a native step that calls Django and reschedules.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@trunk-io

trunk-io Bot commented Oct 5, 2026

Copy link
Copy Markdown

Merging to master in this repository is managed by Trunk.

  • To merge this pull request, check the box to the left or comment /trunk merge below.

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

@coderabbitai

coderabbitai Bot commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

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 configuration
  • Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
  • Review profile: QUIET
  • Plan: Enterprise
  • Run ID: ec9fa0cc-b527-4498-bc19-d7b9178ae578
📥 Commits

Reviewing files that changed from the base of the PR and between 6ef6edd and f5cfb89.

📒 Files selected for processing (13)
  • .github/workflows/ci-nodejs.yml
  • nodejs/src/cdp/cdp-api.serial.test.ts
  • nodejs/src/cdp/services/hogflows/actions/action.interface.ts
  • nodejs/src/cdp/services/hogflows/actions/ai_decision/answer-selection.test.ts
  • nodejs/src/cdp/services/hogflows/actions/ai_decision/answer-selection.ts
  • nodejs/src/cdp/services/hogflows/actions/ai_decision/client.ts
  • nodejs/src/cdp/services/hogflows/actions/ai_decision/config.ts
  • nodejs/src/cdp/services/hogflows/actions/ai_decision/handler.ts
  • nodejs/src/cdp/services/hogflows/hogflow-executor.service.test.ts
  • nodejs/src/cdp/services/hogflows/hogflow-executor.service.ts
  • nodejs/src/cdp/types.ts
  • products/workflows/backend/tests/api/ai_decision_config_cases.json
  • products/workflows/skills/building-workflows/references/graph-schema.md

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 7 remain after this review.


📝 Walkthrough

Walkthrough

The change adds ai_decision workflow actions with yes/no and pick-one answers, configuration and graph validation, and answer-based routing. A new Django endpoint and service check decision eligibility, apply team and global admission limits, and call the decision gateway. The Node executor renders decision context, supports mocked test runs, and handles live replies and retries. The update also adds scoped JWT configuration, tests, and workflow documentation.

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to f5cfb

No actionable issue remains from this review. The AI-decision change is mergeable after normal checks and the stated deployment prerequisites are met.

Security Architecture Review

Security architecture risk: 🔵 Low · up to f5cfb

Team-scoped credentials, data-processing approval and resource limits constrain the new execution path. The main remaining concern is repeated paid work after an interrupted response; access across teams was not demonstrated.

Retained concerns

  • Low · reliability · inferred: A lost response can cause a new request for paid model work that may already have completed. The inspected path carries correlation identity, but does not demonstrate deduplication or recovery of the prior result. Repeated charging or a later, different answer is therefore possible unless the downstream gateway independently prevents it. Retry and admission bounds limit the failure-containment impact.
Security review details

Security Blast Radius

  • inferred — Workflow authors and workflow data can influence the question and context sent for a decision, but the inspected destination is service-configured and access requires a dedicated service credential. A valid token is restricted to its team. Compromise of the signing authority would have broader reach than compromise of one team-bound token; downstream requests additionally consume shared global admission capacity.

Security Findings and Attack Paths

  • observed — The canonical Security assessment retains no verified findings. Source and test counterevidence show rejection of user-only authentication and tokens for another team before decision execution; these observations do not establish safety of uninspected downstream behavior.

Trust Boundaries and Controls

  • observed — The endpoint authorizes the authenticated team, not an independently resolved workflow or action. Workflow identity is explicitly optional labeling for unsaved test workflows. The normal executor supplies workflow and action identity from its active invocation; absence of an entity lookup therefore does not, by itself, demonstrate cross-team access or increased authority.
  • observed — Local credit lookup exceptions fail open, leaving credit enforcement to the gateway. The workflow service treats a gateway 402 as a final quota failure. The deployed gateway's actual credit-enforcement behavior was not verified.

Resilience and Maintainability Implications

  • observed — Admission consumes a team token before a global token. Global denial attempts to refund team capacity, whereas a global-store failure deliberately does not refund against the failed store and returns unavailable without calling the model. This conservatively preserves admission enforcement but can temporarily reduce team capacity during partial failure.

Hardening Proposals

  • proposed — Establish an explicit ambiguous-completion contract with the gateway: either document that bounded repeated charging is intentional, or support deduplication and result recovery using an identity unique to one decision execution, including repeated visits to the same action.
🚥 Pre-merge checks | ✅ 1
✅ Passed checks (1 passed)
Check name Status Explanation
Description check ✅ Passed The description is complete and stands alone. It explains the problem, user-visible changes, design choices, tests, release status, and agent context. The agent context omits a session link and does n…
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/skills/building-workflows/references/graph-schema.md-118-118 (1)

118-118: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Remove ai_decision authoring instructions from the shared MCP skill.

This skill ships to every MCP client, but strict save validation can reject new ai_decision actions when workflows-ai-decision is disabled. The warning does not make the action available to every user. conditional_branch supports deterministic property filters, so it is not a general fallback for an AI decision.


ℹ️ Review info
⚙️ Run configuration
  • Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
  • Review profile: QUIET
  • Plan: Enterprise
  • Run ID: 1ac8d7c6-bddf-4ca9-adbc-dd5348493878
📥 Commits

Reviewing files that changed from the base of the PR and between 282f536 and 6ef6edd.

⛔ Files ignored due to path filters (4)
  • products/workflows/frontend/generated/api.schemas.ts is excluded by !**/generated/**
  • products/workflows/frontend/generated/api.zod.ts is excluded by !**/generated/**
  • services/mcp/src/generated/workflows/api.ts is excluded by !**/generated/**
  • services/mcp/src/tools/generated/workflows.ts is excluded by !**/generated/**
📒 Files selected for processing (43)
  • nodejs/src/cdp/cdp-api.ts
  • nodejs/src/cdp/cdp-services.ts
  • nodejs/src/cdp/config.ts
  • nodejs/src/cdp/schema/hogflow.ts
  • nodejs/src/cdp/services/hogflows/actions/action.interface.ts
  • nodejs/src/cdp/services/hogflows/actions/ai_decision/answer-selection.test.ts
  • nodejs/src/cdp/services/hogflows/actions/ai_decision/answer-selection.ts
  • nodejs/src/cdp/services/hogflows/actions/ai_decision/client.ts
  • nodejs/src/cdp/services/hogflows/actions/ai_decision/config.ts
  • nodejs/src/cdp/services/hogflows/actions/ai_decision/handler.ts
  • nodejs/src/cdp/services/hogflows/hogflow-executor.service.test.ts
  • nodejs/src/cdp/services/hogflows/hogflow-executor.service.ts
  • nodejs/src/cdp/services/hogflows/hogflow-functions.service.ts
  • nodejs/src/cdp/types.ts
  • nodejs/src/cdp/utils/jwt-utils.ts
  • posthog/jwt.py
  • posthog/settings/data_stores.py
  • posthog/settings/web.py
  • posthog/test/test_token_bucket.py
  • posthog/token_bucket.py
  • products/workflows/CONTRIBUTING.md
  • products/workflows/backend/facade/api.py
  • products/workflows/backend/facade/contracts.py
  • products/workflows/backend/facade/enums.py
  • products/workflows/backend/facade/service_jwt.py
  • products/workflows/backend/models/hog_flow/hog_flow.py
  • products/workflows/backend/presentation/views/ai_decision_validation.py
  • products/workflows/backend/presentation/views/graph_validation.py
  • products/workflows/backend/presentation/views/hog_flow.py
  • products/workflows/backend/presentation/views/workflow_ai_decisions.py
  • products/workflows/backend/routes.py
  • products/workflows/backend/service_jwt.py
  • products/workflows/backend/services/ai_decision.py
  • products/workflows/backend/tests/api/ai_decision_config_cases.json
  • products/workflows/backend/tests/api/test_graph_validation.py
  • products/workflows/backend/tests/api/test_hog_flow.py
  • products/workflows/backend/tests/api/test_workflow_ai_decisions_api.py
  • products/workflows/mcp/tools.yaml
  • products/workflows/skills/building-workflows/references/graph-schema.md
  • services/mcp/schema/generated-tool-definitions.json
  • services/mcp/schema/tool-definitions-all.json
  • services/mcp/src/api/generated.ts
  • tach.toml

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.

- A reply whose body breaks after the status line now reschedules as an
  unavailable route instead of failing the step.
- A 503 Retry-After is capped at 60 s, like a 429.
- A yes or no answer needs a valid probability for both yes and no.
- The runtime rejects null characters and lone surrogates in the step's
  text, as the Django serializer does; the shared cases table covers both.
- The run log no longer prints P(yes) next to a "no" answer, and shows
  one decimal so 79.5% does not read as 80%.
- A mocked answer the step does not have lists the step's answers.
- Node CI runs the parity tests when only the shared cases table changes.
- The graph schema reference says what probability holds, and what to do
  when the organization does not have the step.
- Tests: the test-run API forwards mock_answer and mock_async_functions,
  the 8,192 byte boundary in live and mocked runs, and a mocked result in
  an output variable.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants