feat: manual bucket membership, PostHog identity map, and typed journey references - #627
Open
dougwithseismic wants to merge 34 commits into
Open
feat: manual bucket membership, PostHog identity map, and typed journey references#627dougwithseismic wants to merge 34 commits into
dougwithseismic wants to merge 34 commits into
Conversation
added 30 commits
July 27, 2026 18:02
Eleven PRDs plus locked decisions for making plugin-posthog the deep integration: cohort-as-journey-trigger on the manual-bucket spine, typed catalog codegen, retroactive enrollment, engagement writeback, surveys as triggers. Corrected after an adversarial critique pass: the membership-writer contract was inverted, cohort binding had no seed path, a deleted cohort read as an empty one, the loop guard was specified at boot where a PostHog outage would block startup, and the hogsend_* prefix guard was evadable via a custom postHogPropertyKey.
The engine and plugins need request spacing, 429 handling and Retry-After backoff to talk to PostHog, and neither may import from @hogsend/cli without creating a workspace dependency cycle. Behaviour is preserved exactly; the CLI re-exports so existing call sites are unchanged. Core is bundled into the CLI via noExternal because its main entry is raw TypeScript, which plain node cannot load from dist.
A PostHog person has many distinct_ids under one person_id, and Hogsend stored none of them. Model it the way crm_links already models CRM records: own the internal key, treat the external id as a mapped alias. FK-based, so it survives a contact merge without participating in identity resolution. The import_jobs cursor column lands here so a resumable paged pull can ship independently of the cohort sync.
…t_id set getPersonProperties discarded both the person id and the other distinct_ids on a response it had already paid for. Adds a sibling read returning them, keyed on the uuid rather than the numeric row PK: the uuid is what HogQL and the query endpoints expose, so keying on the PK would miss on every query-shaped path and re-resolve every poll. The result is discriminated found/absent/failed. A rate-limited or failed read must never be indistinguishable from a person who does not exist, because downstream that difference is the difference between no-op and mass unenrollment. getPersonProperties keeps its existing signature and soft-fail contract untouched.
…e split Two functions with deliberately different contracts. lookupPostHogPerson is find-only and creates nothing, which resolveOrCreateContact cannot do: a zero-candidate call there falls through to an unconditional insert. Read paths need to be able to answer 'not here' without minting a contact as a side effect. Resolution uses the whole distinct_id set plus email and is independent of array order. A mapping miss means re-resolve, never absent, so a stale mapping can never be read as a departure.
…tch dead rows The membership lookup read contacts by external_id alone, so any contact keyed on an anonymous id was invisible to it. Moving to the canonical coalesce key fixes that but introduces a sharper bug: a soft-deleted merge loser keeps its identity keys and the survivor inherits a copy, so both rows coalesce to the same key and an unordered limit(1) can return the dead one. The predicate is named and exported so tests assert on the row set it matches rather than on whatever the heap happened to return. A test that only reaches it through a downstream effect catches a dropped live filter by luck: with the filter removed it failed one run in three.
kind:"manual" was declared but rejected at registration because nothing populated it. A membership write path now exists, so the reject inverts: a manual bucket is valid, and declaring criteria on one is the error, since nothing would ever evaluate them. minDwell/maxDwell coherence still applies to manual buckets and is checked above the early return. The registry's kind clause in the index skip is deleted rather than tested: the schema makes manual-with-criteria unreachable, so the clause was dead code an assertion could not fail on.
addBucketMember/removeBucketMember are the single mutation seam. They own
the row write, the epoch, maxDwellAt, the entryLimit gate and minDwell
deferral, then emit. emitBucketTransition only emits and takes epoch as
an input, so a caller that emits without writing first produces
transitions with no membership behind them.
Returns { emitted, epoch, verdict } so a poller can tell a real join from
a suppressed one without re-querying. Callable from a workflow task with
no request container.
A re-add disarms a pending deferred leave. Without that, add then
deferred-remove then re-add leaves a current member carrying a live leave
deadline, and the cron force-leaves them and emits a spurious departure.
… buckets The cron skipped the entire per-bucket loop for manual buckets, which was right when they could never gain members. maxDwell TTL leaves, deferred leave resolution and dwell reactions are criteria-independent and must run; criteria evaluation and criteria-driven joins/leaves must not. The passes joined contacts on external_id, so an anonymous-only member could be added and then be invisible to every pass meant to manage them. They now join on liveContactByCanonicalKey, the same predicate the membership seam resolves with. This is the second instance of that bug class on this branch.
POST/DELETE /v1/buckets/:id/members, guarded like /v1/groups: secret key plus ingest scope. Publishable keys may never mutate membership. Rejects mutation on a non-manual bucket, since criteria own that membership.
…oute Four statements still described the removed v1 behaviour and quoted its deleted error message.
The set-based reconcile passes need it as a join ON clause. Without this they hand-roll the coalesce, which is what let an anonymous-only member slip past three passes.
A trigger authored as `{ event: "bucket:entered:at-risk" }` names something
declared elsewhere in the author's own repo, and the compiler cannot check it:
a typo or a later bucket rename yields a journey that silently never fires.
`{ bucket }` desugars to the bucket's own `entered` at definition time, at the
same seam where `trigger.where` is already resolved from a builder. The stored
`JourneyMeta.trigger` is unchanged — always a plain event string — so the
registry, Hatchet routing, blueprints and Studio learn nothing new.
`BucketTriggerRef.entered` is the template literal `bucket:entered:${string}`,
never a bare string, so a hand-rolled `{ entered: someConfigValue }` or a typo'd
`bucket:enter:` prefix is a compile error rather than a journey bound to an
event nothing emits. Declaring both keys, or neither, throws at defineJourney —
silent precedence would make the form worse than the string it replaces.
`sendEmail({ template })` was typed as `TemplateName`, narrowed against the
consumer-augmented `TemplateRegistryMap`, while `ctx.history.email({ template })`
one line away took a bare string. So `template: "welcom"` compiled and answered
"never sent" — silently, forever — and a journey branching on that answer took
the wrong branch with nothing to show for it. Same defect on `ctx.history.sms`.
Both history readers now use the same key unions as their send counterparts.
When a consumer has augmented nothing, `keyof Registry` is `never` and the type
degrades to `string` exactly as the send path does, so an app that has not
authored templates yet still compiles.
Pure type change: no runtime behaviour is touched.
…alytics The engine's own docs say PostHog is an optional plugin that is never load-bearing, while its public API named the vendor. docs/adr/0001-provider-boundary is ACCEPTED and had already shipped the internal half — engine internals call getAnalytics(). This finishes it at the boundary. getAnalytics() is exported first, because it was not exported at all and there was nothing for consumers to migrate to. Then getPostHog() is deleted outright: no deprecated shim, no re-export alias. A silent runtime no-op would surface as "analytics stopped working" weeks later; a compile error is found at build time. Swapping `analytics` on createHogsendClient now actually swaps what comes back, which was never true of a PostHog-shaped singleton. isFeatureEnabled is dropped rather than migrated — it had no live call sites and wrapLegacyAnalyticsService already discarded it. Docs, the journey-authoring skill reference, the scaffold template and the marketing code samples move with it; the skill doc matters most, since it is what agents read when authoring journeys.
PRD 02 was built, reviewed and reverted off this branch; it lives on
parked/posthog-cohort-sync (b2ada111..1d7d91db) and the spec stack said it had
shipped. PRD 03 is blocked behind it and PRD 04 — the cohort trigger sugar whose
`trigger: { cohort: "some-name" }` magic string started this review — is CUT,
not deferred.
Two reasons for parking, both recorded in DECISIONS: PostHog is repositioning
away from being the passive data layer other tools act on, and roughly 20k of
the 26k lines built were the integration while the ~6k underneath it (the
rate-limited fetch in core, the fix for bucket membership missing anonymous
contacts and matching soft-deleted merge losers, and the manual-bucket
membership primitive) is vendor-neutral engine capability worth more than what
sat on top. AudienceSource was dropped with it: it existed solely to replace the
raw cohortId, so with the cohort bet parked it had no consumer.
Also records the durable rule this produced, which outlives PostHog: a reference
to something declared in the consumer's own repo is passed as the typed object,
never as an unchecked name — on read paths as much as write paths. That was the
defect class throughout: sends were narrowed against a registry while the
matching history reads took bare strings.
`pnpm release-doctor` is a required CI job and it failed on this branch: the two changesets bumped 2 of 23 engine-line packages. Left alone, the engine would publish with getPostHog deleted while create-hogsend still shipped a template calling it, and packages/testing's public template narrowing would publish unversioned. The changeset also claimed this removed "the last vendor name from the public API", which is false — lookupPostHogPerson, EXPECTED_POSTHOG_SCOPES, seedPostHogDestination and posthogDestination all remain, and correctly so: they name PostHog because they ARE PostHog. Changeset text publishes verbatim into the CHANGELOG, so the claim is now accurate about what actually changed — the vendor name is gone from the general-purpose wire, not from the surface.
…Error
`null !== undefined` is true, so `trigger: { bucket: null }` from a JS caller
walked straight past both guards and died dereferencing `bucket.entered` — a raw
TypeError thrown while BUILDING the friendly diagnostic that exists to catch
exactly that shape.
Null is now folded to undefined before the guards run, so an absent bucket is
absent however it was spelled. `{ event: "a", bucket: null }` therefore resolves
to the event rather than tripping the exclusivity guard, which is the correct
reading of null-as-absent.
The sweep's own rule, left unapplied one field from the fields it fixed.
`journeyId: "onbaording"` compiled and returned `{ completed: false }` forever —
indistinguishable from "never completed" — on a path whose whole job is
cross-journey gating. A rename did the same thing silently.
`journey: onboarding` cannot be misspelled: an unknown symbol is a compile
error, and a rename follows the symbol. `journeyId` stays legal for ids that
only exist at runtime (config, blueprints, Studio, agent input) — the
serialization-boundary exemption DECISIONS §9 now states explicitly, not a
second spelling of the same thing. Exactly one of the two, never both.
…ange The rule as written said a declared reference is "passed as the typed object" — wrong on its own evidence, since half the sweep that produced it narrows a STRING's type rather than passing an object. Object-ness was never the mechanism; carrying the declaration's type is. Restated with the three legal forms, plus the boundaries it does NOT cross: open-world event names that no consumer declares, serialization boundaries where fail-closed runtime validation is the sanctioned form, and cross-package edges where the narrowing would invert layering. A rule the codebase visibly does not follow teaches people to discount rules. The parked commit range was written `b2ada111..1d7d91db` for seven commits, but git's `..` excludes its left endpoint — a copy-pasted replay would have silently dropped the foundational binding commit. Now `b2ada111^..1d7d91db`, which yields the seven the prose claims. Also: document that the analytics write in feedback-nps is awaited on purpose (an unawaited promise in a durable task can be abandoned when the run completes, silently losing the write), teach the authoring skill reference the trigger's bucket form, and drop the stale half of createPostHogService's deprecation note.
…path `checkBucketMembership` is awaited inside `ingestEvent`, and the move to the canonical key replaced an indexed `external_id` equality with `coalesce(external_id, anonymous_id, id::text)` — an expression no index covered. Every ingested event therefore sequentially scanned `contacts` once any bucket carried property criteria, and the three reconcile joins degraded to full hash joins. Measured on 20k contacts: 286 buffers / 2.07ms (Seq Scan, 19998 rows discarded) before, 3 buffers / 0.017ms (Index Scan) after — and the old cost grew linearly with the contact table, on the per-event path. Partial on live rows to match the read predicate exactly. Deliberately NOT unique: two live rows can coalesce to the same key transiently mid-merge, and a unique index would turn that into a hard write failure.
`resolveManualBucket` read the registry with `get()` while every other membership writer iterates `getEnabled()`, so `POST /v1/buckets/:id/members` on a DISABLED manual bucket wrote the row and emitted `bucket:entered:<id>` into live journeys. `enabled: false` is the operator's kill switch; a hole in it is worse than no kill switch, because it reads as working. Now refused with a `bucket_disabled` code, which the route's existing mapping already renders as 409 (the bucket exists, the write is what is refused).
…, or mis-reasoned Three defects in the minDwell-deferred leave path, all of which left a member active with nobody able to resolve them, or fired an event that should not have. 1. STRANDED. The pending-leave pass was selected on the bucket's CURRENT minDwell and the sweep iterated getEnabled(), so removing minDwell after a leave was deferred — or disabling the bucket — switched off the only pass that resolves the marker, permanently. The pass now keys on the MARKER, and the sweep iterates getAll(): a disabled bucket still FINISHES work accepted while it was enabled, with the emit suppressed. Discovering new work and finishing accepted work are different things, and only the first is what a kill switch must stop. 2. RACED. bulkLeave's CAS never re-asserted the marker or expiresAt, so a concurrent re-add that disarmed the deferral between the pass's SELECT and its UPDATE still got flipped to left, emitting a spurious bucket:left for a member who had just been re-added. The pending pass now re-asserts both. 3. MIS-REASONED. The TTL pass ran first and force-left rows carrying a deferral: with emit:false that fired a SEEDED population into live journeys, and with emit:true it emitted reason "maxDwell" where the truth is "manual" — an operator removed the member and minDwell merely delayed it. The TTL pass now yields any marker the pending pass can resolve on the same tick. A marker not yet due stays eligible: nothing else can resolve it and the row really has outlived its ceiling. seedBucketMembers additionally gates on a live contact, like every other membership writer. A row written for an erased contact is invisible to every sweep (they all join live contacts), so it sits active forever with no path out. Dropped rather than thrown — one stale id must not abort a 40k seed — and REPORTED via skippedNoContact so a caller can tell "everyone was already a member" from "every id was dead".
Both shipped in this branch and neither appeared anywhere a user reads — only
in the agent-facing skill references and CLAUDE.md.
concepts/buckets.mdx gains `trigger: { bucket }` alongside the existing
`trigger: { event: bucket.entered }`, says plainly that they are equally
canonical, and states what the sugar does NOT change: the stored, registered and
Studio-drawn trigger is the same plain event string either way. Also notes the
asymmetry rather than leaving it to be discovered — `exitOn` has no bucket form,
because it takes a list of transitions rather than one trigger.
guides/journeys.mdx documents `ctx.history.journey({ journey })` and, more
usefully, why: `journeyId` is unchecked, so a typo or a later rename returns
`{ completed: false }` forever — indistinguishable from a user who genuinely
never finished, on a call whose whole job is deciding whether to route someone
into another flow. `journeyId` is kept and documented for ids that only exist at
runtime.
added 2 commits
July 28, 2026 12:11
getAnalytics() had no page of its own — it was only mentioned in passing on analytics-access.mdx, which is about credentials and identity rather than the API. The new page covers the surface, why the optional chain is load-bearing (no provider configured, or no container built in this process yet — the silent-void case), which capabilities to check rather than assume, the replay-safety split between a `$set` upsert and a non-idempotent capture, and the getPostHog migration including the one call that changes shape. The buckets guide gains the guarantees this branch added: a disabled bucket refuses membership writes; bulk seeding skips ids with no live contact and reports the count; and a deferred leave survives its bucket losing minDwell or being disabled, is disarmed by a re-add, and keeps reason "manual" even when maxDwell comes due in the meantime. It also states plainly what is NOT true yet: the single-member POST does not apply the live-contact check. Documenting the gap is the point — a guide that implies the endpoint is safe would be worse than one that says where to gate yourself.
addBucketMember, removeBucketMember and seedBucketMembers are public exports of @hogsend/engine and appeared in no doc — the buckets guide covered only the HTTP endpoints, which is the wrapper rather than the thing. A workflow, webhook source or custom job calls the functions. Notes why dependencies are passed explicitly instead of resolved from a request container: a Hatchet task bootstrapping its own db/logger then behaves identically to an HTTP call.
Three conflicts, all where main's recent identity fixes (#621 ghost contacts, #624 email-only bucket members) overlapped this branch's work on the same code. The load-bearing one is checkBucketMembership's contact read. Both sides moved it to the canonical key independently, then diverged on soft-deleted rows: main must still SEE an erased row, because finding one is what fires the GDPR guard; this branch EXCLUDED them, because a merge loser keeps its identity keys and an unordered limit(1) could return the dead row and skip evaluation for a live contact. Taking either side alone reverts the other fix. Resolved as one query that sees everything but orders `deleted_at NULLS FIRST`, so a live row always wins and a purely-erased contact is still found. Both behaviours are now mutation-proven: breaking the ordering and breaking the GDPR leg each turn tests red. The other two are additive — contacts.ts keeps this branch's liveContactByCanonicalKey alongside main's extracted ResolveContactOptions, and both test suites are kept in full.
|
🚅 Deployed to the hogsend-pr-627 environment in Hogsend
2 services not affected by this PR
|
…y type CI's scaffold check caught a build break this branch shipped: packing the engine and installing it into a real scaffolded app failed `tsc` inside @hogsend/testing's harness. Reproduced locally with `pnpm --filter create-hogsend verify`. Cause: narrowing the test harness's history rows to the registry union while the effect they are built from still carried a bare `string`. The boundary declared `template: string`, and `sendEmail` widened its own already-correct value with a redundant `String(opts.template)` on the way in — so the type the send path enforces was discarded one layer down and re-imposed one layer up, which only breaks once a consumer actually registers templates. This repo's own apps never did, so it passed everywhere except a real scaffold. The effect types now carry the registry key, the redundant coercion is gone, and the harness rows are fed from the effect. Both channels, symmetrically — the SMS twin had the identical latent break and was passing only because no SMS templates are registered here. Also raises the timeout on the core barrel-import test: it pulls the whole barrel and exceeded vitest's 5s default on a cold CI runner while passing locally. The import cost is the point of the test, so the budget moves rather than the assertion.
Owner
Author
|
Parked, not abandoned. Conflicts with main and needs a rebase; revisiting after Hogsend Cloud is closed out. |
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.
Manual bucket membership, a PostHog person identity map, and a sweep that removes the last magic strings from the journey authoring surface.
The PostHog cohort sync built alongside this was parked, not merged — see "What was parked" below.
What ships
Manual bucket membership (PRD 01).
kind: "manual"is now usable end to end:addBucketMember/removeBucketMember/seedBucketMembers, plus a secret-key/v1/buckets/:id/membersroute. Each mutation returns{ emitted, epoch, verdict }so a caller can tell "wrote a row and emitted" from "wrote a row and deliberately did not" without re-querying.emit: falsematerialises an existing population without firing it into live journeys.PostHog person identity map (PRD 00). A PostHog
person_idgets a mapped-alias home incrm_links, with a find-onlylookupPostHogPersonand a creatingresolvePostHogPerson. Deliberately an FK side-table rather than a resolver identityKind, so it is immune to the string-key value-fold class fixed in 0.36.1.Authoring surface: references, not names.
trigger: { bucket }— hand over the bucket object; the engine derives the event. Stored trigger is unchanged (a plain event string), so nothing downstream learns a new concept.ctx.history.email/sms({ template })narrowed to the same registry union the send path enforces. A typo was previously a compile-clean lookup that answered "never sent" forever.ctx.history.journey({ journey })— same fix, same file, one field over.getPostHog()removed;getAnalytics()exported. No shim, no alias — the break is a compile error by design. Finishes ADR 0001.Fixes found by review
external_idequality with an uncoveredcoalesce(...).checkBucketMembershipis awaited insideingestEvent, so every event scannedcontacts. Measured at 20k rows: 286 buffers / 2.07ms → 3 buffers / 0.017ms, and the old cost grew linearly. Migration0069.POST /v1/buckets/:id/memberswrote and emitted for disabled buckets.minDwellor disabling the bucket switched off the only pass that could resolve it), raced (the CAS didn't re-assert, so a re-add lost and emitted a spuriousbucket:left), mis-reasoned (the TTL pass fired seeded populations into live journeys and reportedmaxDwellwhere the truth wasmanual).seedBucketMembersgated on a live contact — rows for erased contacts are invisible to every sweep and sit active forever.What was parked
PRD 02 (cohort sync) was built, reviewed, then reverted onto
parked/posthog-cohort-sync(b2ada111^..1d7d91db). PRD 03 is blocked behind it; PRD 04 (thetrigger: { cohort: "name" }sugar that started this review) is cut — its vendor-neutral half shipped astrigger: { bucket }.Reasoning is recorded in
docs/posthog-deep/DECISIONS.md§8: PostHog has no cohort-entry signal, realtime cohorts are unshipped, and behavioural cohorts error in CDP filters — so it meant polling a surface the vendor is actively destabilising, with slowly-wrong membership driving real sends. Roughly 20k of the 26k lines were the integration; the ~6k underneath it is vendor-neutral engine capability worth more than what sat on top.Known gaps, deliberately open
Recorded in
DECISIONS.md§10 rather than silently left:resolvePostHogPersoncan mint a phantom-twin contact. A fix was written and reverted — it was wrong in both directions and a half-fix here is worse than the defect, because it looks closed. Latent: its only consumer was the parked sync.addBucketMemberhas the seed path's ungated-contact defect and is HTTP-reachable. Refusing needs a decision on erased-versus-not-yet-created first, since lazy contact creation by ingest is legitimate. Documented in the buckets guide.Docs
New
guides/analytics.mdxforgetAnalytics(). Buckets and journeys guides updated for the new authoring forms, the membership SDK functions, and the deferred-leave guarantees.Verification
release-doctor16/16--force. One failure:gtm-score-batch"the keyset cursor advances and terminates" — pre-existing onmain, verified at the branch point.user.createdenrolled 5 journeys,trigger: { bucket }desugared and stored as a plain event string.Publishes the engine line at 0.57.0.