Tags: formbricks/hub
Tags
feat: add filters and sort control to the feedback-record list (ENG-2… …059) (#120) * feat: add enum filter parsing and sort field types (ENG-2059) Groundwork for the repeatable enum filters and sort control on the feedback-record list endpoint. No behavior change yet — nothing calls these. - ParseFieldTypes / ParseSentimentValues / ParseEmotionValues parse the repeated occurrences of one query parameter into deduplicated labels. They live here rather than behind a `dive` validate tag so that `?field_type=` keeps meaning "no filter" (a dive tag would turn it into a 400), and so a rejected label is attributed to the plain parameter name instead of `sentiment[2]`. - InvalidSentimentValueError / InvalidEmotionValueError mirror the existing InvalidFieldTypeError, carrying the rejected value and unwrapping to a sentinel. - SortField / SortOrder allowlist the two columns that are safe to keyset on. updated_at is deliberately excluded: the enrichment workers and every PATCH bump it, so a row can move across the cursor between pages and be silently dropped. * feat: make list cursors sort-aware (ENG-2059) A keyset cursor's timestamp bounds one specific column in one specific direction. Once the list endpoint accepts a sort parameter, presenting a cursor under a different ordering still produces a valid, index-served query and a plausible-looking page — just not the continuation of the previous one, with an arbitrary set of rows skipped and another repeated, and no way for the client to notice. Record the ordering in the cursor and refuse a mismatch: - Key/EncodeKey/DecodeKey/Match carry the (sort, order) the cursor was issued under. Encode/Decode stay as wrappers so webhooks, which share this package and have one fixed ordering, keep a single code path. - The payload's s/o fields are omitempty and declared after t/i, so a cursor with no recorded ordering is byte-identical to the old format. Cursors already in client hands keep working, and a legacy cursor matches any ordering. - ErrCursorSortMismatch is a standalone sentinel, deliberately not wrapping ErrInvalidCursor, so the response layer can tell a client to keep its sort unchanged rather than to start over. The payload carries a position and nothing else; the doc comment says why tenancy must never be added to it. * feat: decode repeatable enum query filters (ENG-2059) Register form-decoder type funcs for []FieldType, []SentimentValue and []EmotionValue so a repeated query parameter (?sentiment=negative &sentiment=very_negative) decodes into a validated, deduplicated slice. The decoder resolves a custom type func from the field's own type before it reaches its slice branch, and a top-level struct field is decoded at index 0, so each func receives every repetition at once. Without a registration the elements would decode through the plain string branch and skip enum validation entirely. Parsing here rather than with a `dive` validate tag keeps ?field_type= meaning "no filter" — a dive tag rejects the empty string — and attributes a bad label to the plain parameter name instead of "sentiment[2]", which a client cannot act on. Nothing uses these yet; the filter struct still holds scalars. * feat: add feedback-record list filters and sort control (ENG-2059) Make the feedback-record list usable at scale. GET /v1/feedback-records and /count share one filter struct, so both gain all of this at once. Filters: - the eight identity filters become repeatable, emitted as `col = ANY($n)`. A single occurrence produces the same plan and the same rows as the scalar equality did, so this is backward compatible; a test pins the legacy placeholder layout. - new: source_name, language, created_since/created_until (created_at diverges from collected_at on a historical re-import), value_number and value_date ranges, sentiment, emotions, sentiment_score range, and has_sentiment/has_emotions/has_translation. - emotions matches with `&&` (ANY), not `@>` (ALL): two selected chips read as "anger or fear". `&&` is strict, so the partial GIN index still applies. - field_type binds through `::text[]::field_type_enum[]`. The column is a PostgreSQL ENUM and pgx has no codec for field_type_enum[]; without the cast it works only via a reflection fallback that resolves []string to _text by coincidence. - an inverted min/max pair is a 400 naming the lower bound rather than an empty page the caller reads as "no such feedback". This now also covers the pre-existing since/until pair, which previously returned an empty 200 when supplied backwards. Sort: - sort=collected_at|created_at and order=asc|desc, defaulting to the ordering the endpoint already had. resolveListOrdering is an exhaustive allowlist that fails closed rather than interpolating a request value, and it is tested by driving it directly from Go the way a worker would, bypassing HTTP validation. buildFilterConditions now fails closed when tenant_id is missing or blank. It previously emitted no tenant predicate at all in that case — a query spanning every tenant, prevented only by the HTTP layer's `required` tag, which workers and tests bypass. The condition also moved into a grouped helper as part of this refactor, which is exactly the kind of move that loses a guard silently, so there is a direct unit test for it. * feat: wire sort through the feedback-record list service (ENG-2059) Resolve sort and order to their defaults once in the service, so the keyset predicate, the ORDER BY and the next cursor are all derived from the same values and cannot disagree about what a page is ordered by. The next cursor now carries the last row's value from whichever column the listing is sorted by, plus the ordering itself, and an incoming cursor is checked against the request before it reaches the repository. A cursor issued under a different ordering is refused rather than served a page that looks like a continuation and is not. A cursor with no recorded ordering — anything issued before this change — still works. Also guards the records[len-1] index behind the pagination invariant, mirroring the webhooks list path. It is unreachable today because hasMore implies a non-empty page, but an out-of-range index would panic in the request goroutine instead of surfacing as an error. The repository mock's List/ListAfterCursor returned "not implemented", so the service list path had no unit coverage at all; it now returns a configurable page and captures what the service passed in. * feat: index the new feedback-record filters (ENG-2059) Three indexes for the filters that had no usable coverage. (tenant_id, created_at DESC, id) is the one that matters: filtering created_at while ordering by collected_at cannot use the existing keyset index for the ordering, so a narrow created_at window over a wide collected_at range degrades to a residual scan. The DESC/id shape mirrors 006, so this index serves both the filter and `ORDER BY created_at DESC, id ASC` with no sort step. The ASC direction reads it backward plus an incremental sort on id, whose tie groups are effectively size 1 because created_at is a server NOW() per single-row transaction — so the mirrored ASC index is not created. value_date and source_name get partial indexes, both sparse columns, matching the shape 014/015/018 use. The migration documents what is deliberately left unindexed and why: language is too low-cardinality to be chosen, value_number already has a non-tenant-prefixed index whose replacement needs an EXPLAIN rather than a guess, and the FALSE form of the presence filters selects exactly the rows the existing partial indexes exclude. * test: cover the feedback-record filters against a real database (ENG-2059) The unit tests assert on generated SQL; these assert on rows, which is the only place several of these decisions can actually be checked. - field_type multi-value settles the ::text[]::field_type_enum[] cast. It is the one filter whose column is a PostgreSQL ENUM, and no amount of string assertion proves the cast is valid SQL. - emotions returns both a {joy} record and a {joy,anger} record for ?emotions=joy&emotions=anger. A containment implementation returns one; this test is the whole specification of ANY-vs-ALL. - every range filter includes a row sitting exactly on the bound. - a record with an old collected_at and a new created_at — the historical re-import shape — is matched by created_since and not by since, which is what stops the two pairs being collapsed into one. - every new filter runs against two tenants holding identical data. - the four (sort, order) combinations each traverse 25 records with deliberate collected_at ties in pages of four, asserting the result is a permutation of the seeded set: nothing skipped, nothing repeated, globally ordered across page boundaries. Without the ties a broken tiebreak passes. - a cursor cannot be carried to another ordering, and one issued before sort control existed still works. Verified with EXPLAIN on 60k rows that created_at DESC is a pure index-only scan on the new index and created_at ASC is an index-only scan backward plus an incremental sort, so no mirrored ASC index is needed. * docs: document the feedback-record filters and sort in the API contract (ENG-2059) The spec is hand-authored and Schemathesis generates requests from it, so it is the contract, not a description of one. - The eight identity filters become arrays with an explicit `style: form, explode: true`. That is the OpenAPI default, but it is also the whole agreement with the Go decoder: `explode: false` would render comma-separated values, which go-playground/form does not split. These are the spec's first array-valued query parameters. - 15 new parameters, added to BOTH the list and count operations. The count endpoint documents itself as taking the same parameters as list, and the two share one filter struct, so a parameter on one and not the other would be a spec lie. - sort and order are list-only, alongside limit and cursor. Sort cannot change a COUNT(*), and documenting a parameter with no effect invites misuse. - FieldType, SentimentValue and EmotionValue become named schemas. field_type was written out inline three times and was about to gain a fourth; three sources of truth with the Go constants and the DB CHECK is already the limit. - The cursor description now states that a cursor is bound to the ordering it was issued under, and the 400 carries worked examples for an inverted range and a cursor/sort mismatch. Also documents the 401 both operations have always been able to return — `security: [ApiKeyAuth]` is global, but only 8 of 30 operations said so — and the value_number examples now form a valid range. Schemathesis caught that pair: taken together the old examples described a request the new inverted-range check rejects. * fix: validate repeatable enum filters on the indexed query form (ENG-2059) Security review found the "no unknown label" invariant was false. go-playground/form consults its custom type funcs only when the plain query key is present. Its indexed form takes a different branch and skips them entirely, so `?field_type[0]=bogus` decoded to []FieldType{"bogus"} with no error. For field_type that reached Postgres and failed the field_type_enum cast as an unmapped 500 where a 400 belongs; for sentiment and emotions it was accepted silently and compared as raw text. The values were always bound as parameters, so this was never SQL injection and never crossed a tenant boundary — but the invariant that addAnyOfEnum relies on did not hold, and the gap was untested. Add dive element validators as the gate that actually holds it: they run on the decoded struct however it was populated. The decode-time parsing stays, because it is what keeps `?field_type=` meaning "no filter" and what attributes a bad label to the bare parameter name rather than to sentiment[2]. The max caps now also bound the indexed path, where the decoder's dedupe does not run. Also from the review pass: - Untrack .schemathesis/, swept in by a `git add -A`, and gitignore it. Local run cache; no credentials (Authorization was filtered). - Split ordering out of filter_conditions.go into list_ordering.go, and the query-side model types out of feedback_records.go into feedback_records_filters.go. Ordering is not a filter concern, and the model file was carrying the record, its requests, and the whole list-query surface at 829 lines. - Drop the dead WHERE branch in ListAfterCursor. buildFilterConditions fails closed without a tenant, so the clause it returns is never empty and the joiner is always " AND " — pinned by a test rather than left as an unreachable branch. Coverage on new code is 98.1% (252/257 statements). The five uncovered are unreachable defensive branches — RegisterValidation failure paths and reflect.Kind guards — matching the shape of the pre-existing validators beside them. * test: pin the repeatable-filter value caps to the sets they mirror (ENG-2059) Re-review finding. A struct tag cannot reference a constant, so the `max=` caps added with the dive validators were nine hand-written literals with nothing tying them to anything. The enum caps are the ones that matter. `max=6` on emotions mirrors the Ekman six; adding a seventh label — a live possibility — would leave the cap in place and start rejecting a request that legitimately asks for all seven, with no test going red. TestFilterValueCapsMatch TheirSets reflects over the tags and asserts each enum filter caps at its own label-set cardinality and each string filter at MaxFilterValues. Verified it fails on a drifted cap, not just on a correct one. MaxFilterValues was also left describing a rule it no longer stated accurately, since the enum filters do not use it; its comment now says which filters it governs and points at the test that enforces it. * fix: refuse a legacy cursor under a non-default ordering (ENG-2059) Review found a hole in the guard this branch exists to add. Key.Match treated a cursor with no recorded ordering as a wildcard, so it matched any sort. But such a cursor is not orderless — it was issued before sort control, which means it holds a position in the one ordering the endpoint had then, collected_at DESC. Presenting it with sort=created_at bound that collected timestamp to the created_at keyset predicate, skipping or repeating records: precisely the failure the cursor rework was written to prevent. Match now compares exactly, and the legacy rule moves to the caller as Key.ResolveOrdering(defaultSort, defaultOrder). That keeps pkg/cursor free of endpoint policy — it is shared with webhooks — and puts the knowledge of what the original ordering was in the service that owns it. Old cursors keep working on the default listing and are refused anywhere else. Covered at the cursor, service and integration levels, and the OpenAPI cursor description no longer claims they "remain valid" unqualified. Also from the review: - submission_id caps element length at 255 like every other repeatable string filter. It is stored as VARCHAR(255), so a longer value could never match a row and only widened the bound array. OpenAPI items schema updated to match. - InvertedRanges pre-allocates for five pairs, not four. - The two new problem examples used a type URI that appears nowhere else; every other validation example in the spec uses hub.formbricks.com/problems/validation, which is also what the server actually emits. - has_translation is now covered in the presence-partition test, with the fixture reworked so it discriminates: a record carrying sentiment and emotions but no translation means a predicate reading the wrong column now fails. Verified by pointing it at the sentiment column and watching the test go red. Also fixes the wantoutID typo. * docs: correct the Key doc and strengthen the presence-filter tests (ENG-2059) Follow-up re-review of 9d2e77d. The Key type doc still said "Empty means unspecified: Match accepts it against any ordering" — the exact behaviour 9d2e77d removed. Match's own doc was rewritten and ResolveOrdering documented, but the type doc three declarations above was missed, so it now described the bug as if it were the contract. Someone trusting it could reintroduce the wildcard. The presence-partition fixture also overclaimed. Three records left has_sentiment and has_emotions returning identical sets, so those two stayed silently swappable and only has_translation was really pinned. A fourth record (sentiment, no emotions) gives all three filters a distinct partition. Verified by pointing each of the three at a wrong column in turn and confirming each is now detected — previously only one of the three was. TestFilterValueCapsMatchTheirSets read only the slice-length half of each validate tag, so the per-element caps it appeared to cover were unpinned — including the max=255 just added to submission_id. TestStringFilterElementCapsMatchTheStoredWidth now pins each to its column width, so a cap set below the stored width (which would reject a legitimate value) fails instead of shipping. Verified against a drifted cap. * fix: address ENG-2059 PR review findings (Dhru) Addresses the 4 open review comments on #120. - migrations/021: sentiment_score is a range filter with no index of its own and was missing from both the index set and the "deliberately unindexed" note. Documented the omission instead of adding an index: the feedback_records_sentiment_pairing CHECK from 014 makes sentiment and sentiment_score NULL together, so the existing partial (tenant_id, sentiment) index already narrows to the candidate rows whenever the score filter is combined with a sentiment filter — only the uncombined case is exposed, and a permanent write-path cost isn't worth paying for that on a guess. Left the ready-to-run index for whoever does the EXPLAIN. - feedback_records_filters_openapi_test.go: new test tying openapi.yaml's maxItems/items.maxLength back to the Go-side caps they're supposed to mirror, extending the existing reflection-based TestFilterValueCapsMatchTheirSets as suggested. Nothing previously caught a spec that understates what the API accepts — schemathesis fuzzes inside the documented bound, so a stale maxItems would pass silently. Mutation-tested five ways (understated maxItems, understated maxLength, removed maxItems, an undocumented string filter, and a differently-sized element cap on language) to make sure it actually fails on drift rather than just running green. - feedback_records_filters_test.go: corrected TestStringFilterElementCapsMatchTheStoredWidth's comment. source_type and source_name are bare unbounded VARCHAR (confirmed against the live DB), not VARCHAR(255) — for those two the 255 cap is an API-level choice, not a mirror of the schema, so a value between 256 and unbounded is now storable but not filterable. - openapi.yaml: sentiment and emotions descriptions now state that an empty value is a no-op, matching field_type's existing wording and the actual parseEnumValues behavior all three share. Smoke-tested against a live hub-api + Postgres: confirmed the served /openapi.yaml reflects the new descriptions, empty sentiment=/emotions= return 200 (not 400), sentiment_score_min/max both accept a valid range and reject an out-of-range value, and the new 255-char cap 400s at 256 while still accepting exactly 255. * fix: document /count 400 response and filters in-place mutation (ENG-2059) The /v1/feedback-records/count 400 response now mirrors the list endpoint's detailed description and inverted_range example, so both endpoints document the same validation failures consistently. ListFeedbackRecords now documents that it mutates *filters in-place to fill in default Sort/Order/Limit — safe because the HTTP handler constructs a fresh struct per request.
fix: reset leader session state and withdraw backlog gauge on shutdow… …n (ENG-1670) (#116) * fix(enrichment): reset leader session state, clear gauge on shutdown (ENG-1670) Review follow-ups on the leader election and backlog gauge: - The leader's `SET idle_session_timeout` leaked onto the pooled connection. SET is session state and the pool hands that exact backend to unrelated callers afterwards, so Postgres could eventually terminate some other component's idle connection. release() now RESETs it before returning the connection. Pinned by a MaxConns=1 test that fails with "30min" if the reset is removed. - The gauge was not withdrawn on graceful shutdown -- the most frequent handover of all. Close released the advisory lock while the values stayed, so the meter provider's final export published a backlog this process no longer owned. Added the matching defer. - Corrected the alerting guidance, which the withdraw-on-demotion change had inverted: a failed poll now removes the series rather than freezing it, so the symptom is a MISSING gauge and a value-based staleness rule would not catch it. Updated the interface doc, the metric description and the escalated log. - Fixed a comment that contradicted migration 020 about the 016 index predicate, and one left over from the transaction-scoped lock. - Made the withdraw test load-bearing: it counted zero iterations and would have passed even with a wrong metric name. - Documented why the 020 index rebuild accepts a brief no-index window (its only consumer is the manual backfill command, which degrades to a seq scan) rather than adding a temp-name-and-rename dance. * fix(enrichment): never return an uncleaned leader connection to the pool (ENG-1670) Review follow-ups on #116: - release() shared one deadline between the advisory unlock and the idle-timeout RESET, so a slow unlock could consume the budget the reset needed, and both errors were discarded before handing the connection back -- exactly the leak the reset exists to prevent. Each statement now gets its own detached deadline, both errors are checked and logged, and if either fails the connection is taken out of the pool (Hijack + Close) instead of being reused: ending the session releases whatever the statements could not undo. Losing one pooled connection is cheaper than leaking a lock or a timeout onto a shared one. - The session-state test could have passed vacuously: tryAcquire sets the timeout best-effort and still grants leadership if the SET fails, so asserting a hard-coded "0" proved nothing on a server that ignores it. It now captures the original value, proves the server honours idle_session_timeout at all, and asserts release restores that captured value. - Register the leader's deferred Close before taking leadership so a failing assertion cannot leave the connection checked out and wedge db.Close(). The probe that verifies server support goes through the pool for the same reason, and uses set_config() because pgx's extended protocol rejects "SET ...; SELECT ...". * fix(enrichment): join the backlog poller before shutting down metrics (ENG-1670) Run returns the moment its context is cancelled and never joins runEnrichmentBacklogPoller, so the poller's deferred ClearEnrichmentPending raced Shutdown. Losing that race defeats the point of the clear: the meter provider's final collect-and-export publishes one last backlog reading for a process that has already lost leadership, which is the stale series the clear exists to withdraw. Track the poller with a done channel and join it in Shutdown, registered after the observability defer so LIFO runs the join first. A defer rather than an inline call, so the early-return error paths are covered too. The join is bounded by the shutdown context: a poller stuck in its leader cleanup must not hold shutdown past its budget, and a stale gauge sample is cheaper than a wedged shutdown. Postgres releases the session lock when the socket closes regardless. Waiting also fixes a second, quieter race: the leader's advisory-lock release and idle-timeout reset are best-effort with nothing awaiting them, so process exit could cut them off mid-statement. Only this poller is tracked. runRiverQueueDepthPoller has no clear-on-exit semantics -- it just stops updating, and its last reading is meant to be exported -- so joining it would add shutdown latency for no benefit. TestShutdownWaitsForEnrichmentBacklogPoller pins the ordering; deleting the join defer makes it fail with [metrics-exported metrics-shutdown], the gauge exported before the clear runs at all.
feat: add feedback-record count endpoints (tenant total + per taxonom… …y node) (#103) * chore: feedback records count api * fix: inline count endpoint query params in openapi spec The count operation $ref'd components/parameters/QueryParam* entries that never existed (the spec inlines all params; there is no components/parameters section). This broke Spectral lint, the Stainless SDK preview build, and the schemathesis contract test. Inline the 10 filter params, mirroring the list endpoint. * refactor: use named schema and typed response for count endpoint Promote the count 200 response to a named CountFeedbackRecordsOutputBody component (additionalProperties: false, count as int64) to match the sibling output bodies, and return a typed models.CountFeedbackRecordsResponse from the handler instead of an inline map — mirroring DeleteFeedbackRecordsByUser. Addresses CodeRabbit review feedback. * feat: add taxonomy per-node feedback-record counts endpoint Add GET /v1/taxonomy/runs/{run_id}/record-counts, returning the feedback-record count for every visible node in a run so the taxonomy view can show totals per topic and subtopic. Each count is a subtree total (a branch sums its subtopics, the root reports the run total). Records attach to nodes indirectly via cluster memberships, and the (run_id, feedback_record_id) uniqueness constraint means a record belongs to one cluster per run — so per-cluster counts sum across a subtree without double counting. The repo runs a single GROUP BY over taxonomy_cluster_memberships and rolls the counts up the visible tree in memory. Tenant-scoped via GetRunForTenant (404 for another tenant), mirroring the tree endpoint. * refactor: dedupe shared feedback-record filter params in openapi spec The list and count endpoints repeated the same 10 filter query params verbatim (~90 lines), which would drift as filters are added. Extract them into components/parameters and reference from both; the resolved spec is unchanged. Also stop the count repository error from re-using the service-layer message (was 'count feedback records: count feedback records: ...'); the repo now names the DB op ('query feedback records count'), matching the repository convention. * refactor: count taxonomy node records with distinct-record SQL Replace the per-cluster count plus in-memory subtree rollup with a single recursive descendant-closure query using COUNT(DISTINCT feedback_record_id). The previous approach summed per-cluster counts, which was only correct while each cluster was referenced by exactly one node — an invariant the schema does not enforce. The SQL now counts distinct records per subtree, so a cluster shared by multiple nodes is never double counted. Cover the shared-cluster case in the integration test (two leaves referencing one cluster: the branch and root report the distinct total, not the sum), and decode the feedback-count responses into the typed model.
feat: add value_id to feedback records for stable option identity (#104) * feat(feedback-records): add value_id column and partial index Add a nullable value_id VARCHAR(255) to feedback_records for stable option identity (ENG-1671): the source system's option id, stored alongside the human-readable value_text. The migration runs without a transaction so it never holds a long lock on this high-write table: the ADD COLUMN is metadata-only, and the lookup index is built CONCURRENTLY. The index is a partial composite on (tenant_id, field_id, value_id) WHERE value_id IS NOT NULL — it serves the "all records for option X" lookup and the value_id list filter while staying small and off the ingestion write path for the common text-record (value_id NULL), mirroring the table's other sparse-column partial indexes (sentiment, emotions). * feat(feedback-records): accept, return, and filter records by value_id Wire value_id through the API (ENG-1671): it can be set on create and update, is returned on retrieve/list, and is usable as a list filter (the "update all records for option X" flow). Hub treats it as an opaque string (max 255, no null bytes); it is NULL for free-text/non-choice answers and sources without option ids, where value_text remains the fallback identity. - model: ValueID on FeedbackRecord + create/update requests and the list filter; included in FieldsChangedFrom/ChangedFields so a value_id edit fires the update event like the other value fields. - repository: value_id in the shared column list + scan, the Create insert, buildUpdateQuery (a plain assignment — not an enrichment output, so it is never eager-cleared), and the list filter conditions. - taxonomy: value_id added to the parallel node-records column list that feeds the shared scan. - openapi: value_id on the create/read/update schemas and as a list query param. Covered by repository round-trip, list-filter, model change-detection, update-query, and handler validation tests. * fix(feedback-records): make list-filter placeholder numbering order-independent buildFilterConditions derived each $N from a manual argCount that the trailing Until branch never incremented. It was harmless only because Until is appended last and callers recompute the keyset/limit params from len(args) — but any filter added after Until would have bound the wrong placeholder. Derive each placeholder from len(args)+1 at the point of appending instead, so a condition's $N always equals its argument's 1-based position regardless of order, and the whole class of counter-drift bugs is gone. Locked by TestBuildFilterConditions_PlaceholdersMatchArgs, which asserts placeholder/arg alignment with every filter set.
feat(observability): enrichment round-2 (event correlation, HNSW-degr… …adation gauge) + perf nits (#100) * perf(embeddings): compute similarity score in Go; build embedding input once Two free wins surfaced by the Fable-5 review, no behavior change: - The nearest/after-cursor queries selected both `(e.embedding <=> $1) AS distance` and `(1 - (e.embedding <=> $1)) AS score`, evaluating the cosine-distance operator twice per row. Drop the score expression and derive score = 1 - distance in Go after the scan (distance is still what the query orders and cursors by). The semantic-search integration test already asserts score == 1 - distance. - The embedding provider's create path called BuildEmbeddingInput once for the empty-text check and again inside the dedupe hash. Build it once and reuse it; the now-single-caller embeddingValueTextHash helper is inlined to hashContent. * feat(observability): gauge for HNSW iterative-scan degradation When the server rejects hnsw.iterative_scan (pgvector < 0.8), the repo latches the fallback and logs once — after which nearest-neighbor recall is silently capped at ef_search for the process lifetime, invisible to monitoring once the log scrolls. Expose the latch as EmbeddingsRepository.IterativeScanDegraded() and register an observable gauge (hub_hnsw_iterative_scan_degraded, 0/1) that polls it, wired alongside the search handler so it is only registered when embeddings are enabled. No-op when metrics are disabled. * feat(observability): carry originating event id in enrichment job args Enrichment jobs recorded no link back to the domain event that enqueued them, so a job in the River UI (or its logs) could not be correlated to the event, and an event could not be traced to the jobs it fanned out. Add an EventID field to all four enrichment job args (sentiment, emotions, translation, embedding), populated from event.ID at enqueue — threaded through the generic provider's buildArgs hook and set directly on the embedding provider's insert. EventID is deliberately NOT part of the River unique key (no river:"unique" tag): it is correlation metadata, must not affect content dedupe, and is uuid.Nil for backfill jobs, which have no originating event. Adding the field is a backward-compatible JSON change (older in-flight jobs decode with a zero EventID). Worker-side logging of it follows in the next commit. * feat(observability): log the originating event id in enrichment workers Build on the EventID now carried in job args: emit it on the worker log lines so a job's logs correlate to the event that enqueued it (and, with feedback_record_id, to the record). In the generic enrichment worker this is done once by binding a slog.Logger with feedback_record_id (+ event_id when the eventID hook is set) and threading it through Work and its error helpers — which also removes the feedback_record_id key duplicated across ~12 call sites. Sentiment, emotions, and translation wire the eventID hook; the standalone embedding worker adds event_id to its record-gone/get-failed/stored lines (its remaining lines fold in with the scaffold migration, ENG-1643). event_id is uuid.Nil for backfill jobs, which unambiguously marks a non-event-driven origin. * refactor(observability): complete event_id coverage on the embedding path Addresses the #100 review: the standalone embedding worker logged event_id on only 3 of its ~10 lines, while the generic worker logs it on every branch. Bind a slog.Logger once in Work with feedback_record_id + event_id and thread it through handleEmbedError, handleSetEmbeddingError, and handleEmptyText — so every embedding outcome (rate-limit snooze, API failure, superseded/purge write skips, clear, skip) now carries event_id, and the feedback_record_id key is no longer repeated across 10 call sites. Also add event_id to the two enqueue-side provider debug lines that had feedback_record_id but not event_id (enrichment "skip, not enabled for tenant"; embedding "skip, no value_text on create"), matching their sibling log lines. * test(observability): lock event-id propagation and the iterative-scan gauge Addresses the #100 review's test-coverage gap: - Each enqueue provider test (sentiment, emotions, translation, embedding) now asserts the enqueued job args carry EventID == event.ID, so a regression that dropped EventID in any buildArgs closure or the embedding insert fails loudly (previously it would pass silently — the field was set but never asserted). - New RegisterHNSWIterativeScanGauge test: nil meter / nil callback are no-ops, and via a manual-reader meter the gauge reports 0 when the latch is healthy and 1 once degraded (the callback is re-polled per collect). * feat(observability): counter for enrichment outputs cleared on edit Round-1 made the eager-clear observable via a log line; this adds the metric the Fable review (A3) asked for so the clear RATE is queryable, not just grep-able. New hub_enrichment_outputs_cleared_total{output=sentiment|emotions|translation} increments once per output an edit's eager-clear nulls. The label set is bounded (NormalizeClearedOutput → "other" for anything unexpected). Wired through a new EnrichmentClearMetrics collector + aggregate field; the service records it via a SetEnrichmentClearMetrics setter, set only on the API instance (the eager-clear fires on UpdateFeedbackRecord) and left unset — a no-op — on worker/backfill. Tests cover the counter (emit + label normalization) and the service wiring (a value_text edit that nulls sentiment records one "sentiment" clear). * docs: refresh AGENTS.md for the enrichment framework The guidance drifted as the enrichment work landed. Update it to match the tree: - Structure: list the backfill commands (cmd/backfill-*), the four worker enrichment pipelines, and the internal packages that had appeared since (observability, llm/openai/googleai, datatypes, huberrors) + pkg/cursor, pkg/embeddings. - Commands: `make run` starts both api and worker (was described as api-only); add the run/build backfill targets. - New "Enrichment Framework" section: points at the shared classify scaffold (registry, classifyStructured, EnrichmentProvider, generic enrichmentWorker, shared metrics) and the thin per-type recipe, so a new enrichment reuses the scaffold instead of copy-pasting one — plus the read-only/eager-clear and supersession invariants. - Testing: split unit (internal, no DB) from integration (tests/, needs a pgvector test_db); note the tenant-isolation alternate-path test requirement. - Config: point at the enrichment/OTel env groups documented in .env.example. * fix(observability): correlate the translation short-circuit log line The translation worker's "source already in target language, copying value_text" short-circuit logged only feedback_record_id — the one main-line in the translation path still missing the event_id that every other worker log line now carries. Thread the event id (already present on the job args) into translate() and log it, matching the generic worker's field order. Completes event_id coverage for the translation path; no behavior change.
feat(sentiment): add sentiment enrichment for open-text feedback (#94) * feat: add sentiment + sentiment_score to Hub feedback records Adds the two server-generated sentiment-enrichment fields to feedback_records (ENG-1529): sentiment (6-level label very_negative..very_positive + mixed) and sentiment_score (signed polarity, -2..2), both nullable until enriched. Migration 014 adds the columns with CHECK constraints + a partial composite index; the model gains a typed SentimentValue enum and the two fields; the read path (feedbackRecordColumns + scanFeedbackRecord) and the OpenAPI feedback-record schema carry them. Also relocates scanFeedbackRecord next to its feedbackRecordColumns const (it lived in taxonomy_repository.go) so the SELECT and scan order stay in lockstep. * feat(sentiment): add SENTIMENT_* provider config gate Sentiment enrichment (ENG-1529) is disabled unless SENTIMENT_PROVIDER and SENTIMENT_MODEL are both set, mirroring the provider+model gate used by embeddings and translation (no separate enable flag). Add SentimentConfig with an Enabled() helper, document the vars in .env.example, and cover the gate in config tests. * feat(llm): add structured-JSON completion to the LLM clients Add a provider-agnostic structured-output seam for enrichments that need a typed result (sentiment, and later emotion), with no existing pattern to reuse: - internal/llm: an SDK-neutral Schema/Property spec so a structured request is described once and rendered to each provider's native schema type. - openai.Client.CompleteJSON: OpenAI Structured Outputs (response_format json_schema, strict) — a closed object with every property required. - googleai.Client.CompleteJSON: Gemini responseMimeType=application/json with a typed responseSchema. Both return the raw JSON string; the service layer owns the prompt, schema, and parsing/validation (numeric bounds are validated post-parse, since OpenAI strict mode does not support them). Factor the shared 429->RateLimitError mapping and completion-text extraction out of Translate so both calls share one error path. Tested via httptest against each real SDK: the request carries the strict schema and the JSON response parses. * feat(sentiment): add sentiment classification client and factory Add the SentimentClient seam (Classify -> {label, score}) and a provider factory mirroring the translation factory (openai / google / google-gemini registry, ValidateSentimentConfig, NewSentimentClient), driven by SENTIMENT_* config. The client owns the contract: it builds the prompt (six-level scale + mixed, optional source-language hint), passes the structured-output schema to the LLM client's CompleteJSON, then parses and validates — unknown label or unparseable response is rejected, and the score is clamped to [-2, 2] so a stray value still satisfies the DB CHECK. The schema enum derives from models.SentimentValues, now an ordered slice that also backs IsValid, so the label set has one in-Go source. Extend SentimentConfig with BaseURL / GoogleCloud{Project,Location} for full provider parity, and document them in .env.example. * feat(sentiment): add sentiment pipeline metrics Add SentimentMetrics (jobs-enqueued / provider-error / outcome / worker-error / duration) mirroring the embedding and translation collectors, with bounded reason and status allow-lists, and wire it into the aggregate Metrics bundle. The provider and worker depend on this interface (nil when metrics disabled). * feat(sentiment): add sentiment enqueue provider and job args SentimentProvider enqueues one feedback_sentiment job per eligible record event, mirroring the embedding provider (the simpler, target-free shape): text fields with non-empty value_text on create, or a value_text change on update (enqueued even when now empty so the worker can clear). Dedupe is by (record, value_text hash); unlike translation the source language is not part of the hash or the re-enqueue trigger, since sentiment is classified directly from the text. Enqueue failures are logged and counted, never blocking ingestion. * refactor(workers): extract shared rate-limit snooze helper The translation worker's rate-limit snooze (provider 429 -> bounded JobSnooze, capped by a max snooze window) applies verbatim to any LLM-backed enrichment worker. Lift it to a package-level rateLimitSnoozeDelay(err, jobCreatedAt) helper (with the bound constants) so the upcoming sentiment worker reuses it instead of duplicating the logic. Behavior is unchanged; the translation worker and its test now call the shared helper. * feat(sentiment): add sentiment worker and SetSentiment write FeedbackSentimentWorker fetches the record, classifies value_text via the SentimentClient, and persists the result (or clears it when value_text is empty), mirroring the embedding worker's not-found/tenant-conflict handling and reusing the shared rate-limit snooze for provider 429s. Skips non-text fields and a record gone before write; a provider failure retries to the attempt limit. SetSentiment (repository + service) writes both columns under the shared tenant write lock with no domain event and — unlike SetTranslation — no supersession guard, since sentiment has no per-tenant target. The service clears on a nil label and rejects an unknown label or a label without a score; the column CHECKs are the final guard. * feat(sentiment): wire the sentiment pipeline end to end Gate the sentiment client, enqueue provider, and worker on cfg.Sentiment.Enabled() (SENTIMENT_PROVIDER+MODEL), mirroring the translation/embedding wiring: - hub-worker builds the client + a lean records service and registers the worker on the sentiments queue (cfg.Sentiment.MaxConcurrent). - hub-api registers the worker/queue so it can enqueue at insert time, and registers the SentimentProvider off the River client (no tenant-settings dependency, unlike translation). Add SENTIMENT_MAX_CONCURRENT / SENTIMENT_MAX_ATTEMPTS (defaults 5 / 3) and document them. With sentiment unset, nothing is registered and no jobs run. * test(sentiment): add worker pipeline integration test Drive FeedbackSentimentWorker end to end against Postgres with a fake classifier: it classifies and persists the label+score through the real SetSentiment write, clears a stale sentiment when value_text is empty (without calling the provider), and SetSentiment on a missing record returns NotFound. * fix(sentiment): mark Gemini enum fields format:enum so they are enforced The genai schema contract enforces an enum only when the field is also marked format:"enum"; geminiResponseSchema set Enum without it, so on the Gemini path the sentiment labels were a hint, not a constraint — an out-of-enum label would fail validation and waste a retry. OpenAI's strict json_schema path was already enforced. Also: document that the sentiment model must support structured output (.env.example), and correct migration 014's comment to say the label/score relationship is steered by the prompt, not enforced by the worker. * test(sentiment): dedupe client-test helper, guard schema/parser coupling - Extract the duplicated mustMap JSON-body assertion into a shared internal/llm/llmtest package (imported only from _test.go, so testify stays out of the production import graph); the openai and googleai client tests use it. - Add a test that builds a structured response keyed by the sentiment schema's own property names and parses it, so renaming a schema property without updating sentimentResponse's json tag fails loudly (verified via mutation). * fix(sentiment): make the 014 migration lock-safe on feedback_records feedback_records is the primary, high-write table, so avoid holding a long ACCESS EXCLUSIVE lock during the migration (it previously added two validated CHECK constraints in-transaction, forcing a full-table verification scan under ACCESS EXCLUSIVE). Switch to the NO TRANSACTION + CONCURRENTLY pattern already used by the index migrations (005/006/009): - add the CHECKs NOT VALID (instant; immediately enforced for new/updated rows), then VALIDATE them as separate auto-committed steps that take only SHARE UPDATE EXCLUSIVE, so reads and writes proceed during the scan; - build the partial index CONCURRENTLY. ADD COLUMN of a nullable column with no default stays metadata-only. Verified: down+up re-applies cleanly, both constraints end up convalidated, and the sentiment integration tests pass against the rebuilt schema. * feat(sentiment): gate enrichment on a per-directory enable setting ENG-1529 requires sentiment to honor a per-directory AI-enrichment switch, not just the deployment-level provider/model gate. Add a tenant-scoped sentiment_enabled setting and enforce it on the enqueue path: - tenant_settings gains sentiment_enabled (stored, PUT, PATCH/RFC-7396). It is tri-state and defaults to enabled (opt-out): a tenant is enriched unless it has explicitly switched sentiment off. EnrichmentSettings.SentimentEnrichmentEnabled() centralizes that default. - SentimentProvider now resolves the tenant's setting (after the cheap eligibility checks) via a shared tenant-settings cache and skips disabled tenants; a settings read failure skips rather than enqueuing blindly. - Translation and sentiment now share one tenant-settings cache, evicted on any settings write so a toggle takes effect immediately. - OpenAPI documents the new field on the settings schemas. Tested: service PUT/PATCH/null + key-tag pinning, provider gate (disabled skips, enabled/absent enqueue, read-error skips), and a full HTTP settings round-trip. * feat(sentiment): default to neutral when the model is unsure ENG-1529 specifies neutral (score 0) as the default when the classifier is uncertain. Make that explicit in the prompt. * refactor(observability): share one implementation across enrichment metrics The embedding, translation, and sentiment metrics were three ~118-line files of near-identical logic (the same five instruments, record methods, and status normalization), differing only in metric names and the bounded reason/status label sets. That copy-pasted logic was the real duplication risk — a fix or new label had to be made in three places. Extract one enrichmentMetrics implementation (instrument creation, the shared Record* methods, normalization) parameterized by a per-pipeline spec. Each pipeline keeps its public interface and constructor but is now a 2-method adapter (its differently-named outcome/duration methods) over the shared impl, so the three interfaces and every caller (workers, providers, wiring, the aggregate bundle) are unchanged — the change is confined to the observability package. Left as-is on purpose: the client factories and worker error-handlers look parallel but diverge per pipeline (client type, embedding DocPrefix, translation supersession, the google-vertex alias); collapsing them would need long parameter lists / generics-over-config that read worse than the parallel code. * fix(sentiment): enforce a closed-object schema on both LLM providers CodeRabbit: the Gemini path used ResponseSchema (the OpenAPI subset), which does not enforce additionalProperties:false, so extra keys could pass the provider boundary — unlike the OpenAI strict path. Switch Gemini to ResponseJsonSchema (standard JSON Schema) and render the schema once via a shared llm.Schema.JSONSchema() builder used by both clients. Both providers now enforce the same closed-object contract, and the OpenAI-map vs Gemini-genai.Schema duplication is gone (one builder, tested in internal/llm). * fix(sentiment): tighten persistence and parsing integrity Address several CodeRabbit data-integrity findings: - SetSentiment now returns NotFound when the UPDATE affects zero rows (record deleted after the tenant lock), matching its doc contract and the worker's benign-skip path instead of reporting a phantom success. - sentimentResponse.Score is now *float64: a response omitting score is rejected as ErrSentimentResponseInvalid rather than silently persisted as 0. - migration 014 adds a paired-nullability CHECK (sentiment and sentiment_score set together or both NULL) as defense-in-depth against a half-populated write, and uses the documented lowercase -- +goose up/down annotations. - models.SentimentValues is no longer an exported mutable slice: it is an unexported canonical slice exposed via SentimentValues() returning a copy, so the label set can't be mutated out from under IsValid / the schema enum. * fix(workers): bound the rate-limit snooze to the max window CodeRabbit: the snooze window was only checked before choosing the delay, so a job still inside maxRateLimitSnoozeWindow could be re-queued for another full snooze and overshoot the cap by up to maxRateLimitSnooze. Also reject the snooze when elapsed + the chosen delay would exceed the window, so a rate-limited job falls through to the normal-failure/backfill path on schedule. Covered by a new overshoot case in the snooze test. * chore(sentiment): address review nits - embeddings worker-error metric description is provider-neutral (was openai-specific but the constructor is multi-provider). - .env.example lists SENTIMENT_PROVIDER=google (a supported provider that was missing). - sentiment enqueue test asserts the River UniqueOpts (ByArgs + ByPeriod), pinning the dedupe contract. - the integration not-found case now drives worker.Work end to end (GetFeedbackRecord -> NotFound -> benign skip), alongside the repository-level SetSentiment NotFound check. * refactor(sentiment): normalize sentiment_score to the -1..1 polarity range Rescale the signed sentiment_score from -2..2 to the conventional -1..1 polarity range (Google Cloud NL / TextBlob / VADER) across the model bounds, prompt anchors, structured-output schema description, the 014 migration CHECK, OpenAPI min/max, .env.example, and the tests. * fix(sentiment): record retriable worker failures as retry, not failed_final The get-record and generic-write error paths recorded the job outcome as failed_final on every attempt, but River retries the job while attempts remain, so hub_sentiment_outcomes_total{status="failed_final"} overcounted final failures. Record retry vs failed_final by attempt (as the classify and tenant-write-conflict branches already do) and surface final_attempt in the error log. Adds table tests pinning both paths across a non-final and the final attempt. * fix(sentiment): fail open on a settings-read error; make the worker the gate The enqueue provider dropped the job when the per-tenant settings read failed, permanently losing enrichment for events ingested during a transient tenant_settings/cache outage (the event path is best-effort, not retried). Fail open instead: the provider enqueues on a settings-read error (still logging and recording the provider-error metric), and the sentiment worker re-checks the per-directory switch as the authoritative gate before the LLM call. A read error in the worker retries (so a fail-open enqueue is never lost); a tenant that turned sentiment off is skipped without classifying or clearing, matching what the provider gate does when settings are readable. Wires an uncached tenant-settings reader into the worker in both hub-worker (where jobs run) and hub-api (insert-only), adds the settings_read_failed worker metric reason, and covers the disabled/read-error/fail-open paths with tests.
feat: serialize tenant-owned writes against tenant data purges (#87) * feat: serialize tenant-owned writes against tenant data purges Tenant data purges now take an exclusive transaction-scoped advisory lock per tenant while every tenant-owned mutation (feedback records, webhooks, embeddings, taxonomy) holds the same key in shared try-lock mode inside a single repository-level helper. Writes for a tenant under purge fail fast with a retryable 409 (code tenant_write_conflict); the purge waits up to TENANT_PURGE_LOCK_TIMEOUT_SECONDS (default 5s) for in-flight writes to drain and then returns the same retryable conflict. Other tenants are unaffected. ID-based mutations now resolve the row's tenant inside the transaction and mutate with tenant-scoped WHERE clauses; the GDPR delete-by-user locks every spanned tenant; tenant-changing webhook updates lock both tenants. Workers skip or retry cleanly on conflicts. Closes ENG-1013 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * chore: consolidate tenant write lock helpers and interfaces * chore: address PR comments * chore: address pr issues --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
PreviousNext