feat/discovery

updateWithVersion omits version check in WHERE clause — optimistic lock is broken

domains/core/src/discovery/discovery.queries.ts:96

WHERE clause filters only on `id`, not `version`. `currentVersion + 1` is set but the concurrent-write guard is missing. A race between two simultaneous `recordOutcome` calls silently overwrites the first. Add `eq(discoveryState.version, decision.currentVersion)` to the WHERE. Every other versioned entity in the codebase (api-key, secret-configuration, site, context) includes this guard.

RLS binding in recordOutcomeHandler: createRLSDb passed to shell which calls .transaction() itself

apps/platform/src/api/handlers/discovery.handler.ts:86

Handler passes `createRLSDb(database, userId)` to the shell, which then calls `db.transaction(async tx => …)`. The invariant is `createRLSDb(db, uid).transaction(tx => …)` — RLS binds per-transaction only. If the proxy does NOT re-run SET LOCAL inside the transaction spawned by the shell, writes execute without RLS enforcement. Verify the proxy correctly sets the RLS context at the start of every .transaction() call, or restructure so the handler wraps the shell call in the transaction.

Budget window blocks ALL spotlights for 24h when ANY key was seen — one impression starves all others

domains/cross-domain/src/feature-discovery.decisions.ts:3

Budget check uses `exposures.some(e => within budget window)` across ALL keys. A single 'seen' on spotlight A prevents spotlight B from appearing for 24h. If the spec intends 'at most one new spotlight per day per user' (not 'zero spotlights once anything was seen'), the filter should check the candidate key's own lastSeenAt, not any key. Verify the spec intent — if cross-key suppression is intentional, document it explicitly.

recordedRef not reset on spotlightKey change — new spotlight on same mount won't record 'seen'

apps/platform/src/components/discovery/FeatureSpotlight.tsx:8

recordedRef is never reset when spotlightKey changes. If the layout stays mounted and a new key is received (SPA navigation), recordedRef.current is already true and 'seen' is not recorded. Store the last-recorded key (string | null) instead of a boolean to deduplicate by key.

Missing version-conflict error variant — null from updateWithVersion maps to generic 500

domains/core/src/discovery/discovery.shells.ts:66

Once the WHERE version-guard is added (critical finding), a null return means version conflict, not a database failure. The shell should emit a dedicated DiscoveryErrors.versionConflict() (4xx retryable) rather than a generic 500. Add the variant before shipping the version-guard fix.

getDiscoverySpotlightShell accepts DbOrTx — bare db would bypass RLS silently

domains/cross-domain/src/feature-discovery.shells.ts:2

Shell relies on callers to have wrapped the arg in a createRLSDb transaction. Accepting `tx: Transaction` instead of `DbOrTx` makes this contract explicit at the type level and prevents future call sites from accidentally passing a bare db.

Raw database error message leaked to client via serverError(error.message)

apps/platform/src/api/mappers/discovery.mapper.ts:14

mapGetSpotlightError and mapRecordOutcomeError pass error.message directly into serverError(). If DiscoveryDatabaseError wraps a raw Drizzle/Postgres exception, the response body may contain schema names, table names, or query fragments. Sanitize to a generic message and log the raw error server-side only.

orgId path param not bound to discovery_state queries — design intent should be documented

apps/platform/src/api/handlers/discovery.handler.ts:80

requireOrgAccess gates on orgId but discovery_state rows are scoped only by profileId (not orgId). Safe today via RLS own-rows-only + profileId binding. If discovery_state ever gains org scope, this silently stops scoping correctly. Add a comment documenting the intentional profile-scope design.

createRLSDb seeded with userId (Supabase auth UID) but rows filtered on profileId — verify RLS predicate matches

apps/platform/src/api/handlers/discovery.handler.ts:84

If the RLS policy uses auth.uid() and discovery_state.profileId stores a domain profile ID (not the Supabase UID), the policy predicate silently doesn't filter and only the app-layer WHERE guards the rows. Confirm the column type stored in discovery_state.profileId is the Supabase auth UID or that the RLS policy uses the correct identifier.

Missing DiscoveryFCIS namespace export in barrel

domains/core/src/discovery/index.ts:1

canonical-form.md requires the barrel index.ts to export a named {Entity}FCIS namespace (e.g. DiscoveryFCIS grouping queries, shells, decisions). Every other entity's barrel exports this namespace. The discovery barrel exports individual symbols only.

Entity file naming mismatch: table is discovery_state but decision/query/shell files use discovery.*

domains/core/src/discovery/:1

canonical-form.md requires {entity}.decisions.ts etc. where {entity} matches the domain table slug. The Drizzle table is discovery_state, so canonical names would be discovery-state.decisions.ts. Using discovery.* is inconsistent with the one-to-one convention used by all other entities.

Cross-domain feature-discovery files have no FCIS namespace or barrel export

domains/cross-domain/src/feature-discovery.queries.ts:1

Cross-domain coordinators should follow the entity file pattern with a namespace export. The standalone .queries.ts with no companion barrel or namespace makes the module inconsistent with how other cross-domain coordinators are structured.

ValidationErrorResponseSchema defined locally instead of imported from packages/api

apps/platform/src/api/contracts/discovery.contract.ts:34

The schema is copy-pasted across 4+ contract files (column-configurations, credentials, api-keys, discovery). Should be extracted to packages/api/src/schemas/jsend.schemas.ts alongside JSendSuccessSchema/JSendFailSchema/JSendErrorSchema.

Multi-paragraph docstring block in handler file violates comment policy

apps/platform/src/api/handlers/discovery.handler.ts:1

Project policy: one short line max, no multi-line comment blocks explaining what code does. The file-level JSDoc block describes both handlers, org segment sourcing, and ts-rest caveats. Should be removed or reduced to a single-line reference.

Multi-line docstring in FeatureSpotlight.tsx violates comment policy

apps/platform/src/components/discovery/FeatureSpotlight.tsx:1

Same comment policy violation as the handler. The multi-paragraph JSDoc block listing spec references and behavior rules should be removed.

recordSpotlightOutcomeShell: null-from-DB error path (write returns null) has no integration test

domains/core/src/discovery/discovery.shells.ts:66

The shell guard at line 66 returns err(DiscoveryErrors.databaseError(operation)) when the DB write returns null. No test covers this path. Once the version-guard is added (critical fix), this path becomes the version-conflict path — add integration test before merging.

updateWithVersion: version increment in DB not verified at the query layer

domains/core/src/discovery/discovery.queries.ts:85

No integration test directly exercises updateWithVersion and asserts the version field is incremented in the DB row. Integration tests verify seenCount bumps through the shell but never assert version = currentVersion + 1 post-write. Add a direct query-layer integration test.

API handlers: no tests for auth/authz failure paths (401, 403, 422, 500)

apps/platform/src/api/handlers/discovery.handler.ts:45

Neither getSpotlightHandler nor recordOutcomeHandler has a test. Missing: (1) unauthenticated → 401; (2) no profileId → 401; (3) not org member → 403; (4) invalid outcome value → 422; (5) shell returns DiscoveryDatabaseError → 500. Handler tests are the canonical way to lock in the full validate→authorize→shell→map chain.

recordOutcomeHandler: profileId-from-auth (not body) security invariant has no test

apps/platform/src/api/handlers/discovery.handler.ts:100

The handler passes profileId from authReq.auth to the shell, preventing callers from recording outcomes against another user's profile. No test asserts this property. This is a load-bearing security invariant that should be an explicit handler-level assertion.

findByProfileAndKey: no-match (null return) path not directly tested

domains/core/src/discovery/discovery.queries.ts:36

The function returns null for a missing (profileId, key) pair, tested only implicitly through the shell's first-seen flow. A direct integration test asserting null for a genuinely missing key is needed per the testing.md thin-DB-wrapper guideline.

FeatureSpotlight: missing anchor behavior (no 'seen' recorded, analytics event fires) not tested

apps/platform/src/components/discovery/FeatureSpotlight.tsx:136

The 10×300ms retry exhaustion path emits discovery.spotlight_anchor_missing and renders nothing without recording 'seen'. This 'do not consume pacing budget on missing anchor' invariant is uncovered by any test. Add an RTL test that renders the component with a key whose data-tour anchor is absent.

FeatureSpotlight: dismiss and click mutation paths have no component test

apps/platform/src/components/discovery/FeatureSpotlight.tsx:178

onDismiss and onCta call record() and track() but there are no RTL tests asserting 'dismissed' or 'clicked' outcome mutations fire, or that the coachmark disappears after dismiss.

spotlight-catalog: resolveSpotlightPage mapping coverage incomplete

apps/platform/src/components/discovery/spotlight-catalog.ts:33

No unit tests cover: (1) exact match per page; (2) sub-path match (/bills/contratos/detail → bills-contratos); (3) unrelated path → null; (4) near-miss prefix that must NOT match (/bills/contratoss). Pure function — trivially fast unit tests.

E2E: actual spotlight show/dismiss/click flow not covered

e2e/platform/discovery-anchors.spec.ts:1

The E2E spec only asserts anchor DOM presence. No test exercises: PostHog flag enabled → GET spotlight returns key → coachmark appears → dismiss → outcome recorded → next page load returns null (budget gate). This is the primary user-visible behavior of the feature.

findByProfileId: multi-row return path not directly tested at the query layer

domains/core/src/discovery/discovery.queries.ts:24

findByProfileId is used by the read coordinator to load all exposure rows. No test asserts that after recording two distinct keys for the same profile, findByProfileId returns both rows correctly mapped.

FeatureSpotlight: desktop-only suppression (isDesktop=false → query disabled) not tested

apps/platform/src/components/discovery/FeatureSpotlight.tsx:60

No component test mocks a narrow viewport and asserts the GET /discovery/spotlight query is never triggered and nothing renders. Spec-level behavior (v3 spec §5 'desktop only') with no test coverage.

ValidationErrorResponseSchema duplicated across 4+ contract files — extract to shared location

apps/platform/src/api/contracts/discovery.contract.ts:34

Byte-for-byte identical across discovery.contract.ts, column-configurations.contract.ts, credentials.contract.ts, api-keys.contract.ts. Extract to packages/api/src/schemas/jsend.schemas.ts alongside existing JSend schemas.

hasContracts in DiscoverySignalState is redundant with contractCount > 0

domains/cross-domain/src/feature-discovery.queries.ts:77

Both hasContracts (boolean) and contractCount (number) are populated, but hasContracts is just contractCount > 0. Remove hasContracts and update decisions to use contractCount > 0 directly — one field to keep in sync instead of two.

Scroll/resize effect uses `anchorRect !== null` as dependency with eslint-disable

apps/platform/src/components/discovery/FeatureSpotlight.tsx:159

The dependency array uses an expression rather than a stable value, requiring an eslint-disable suppression. Introduce a boolean ref for whether listeners are attached, or derive `const anchorFound = anchorRect !== null` outside the effect to make the dependency stable.