claude/guided-tour-system-spec-3eb412

updateWithVersion WHERE clause missing version predicate — optimistic lock is silently broken

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

The WHERE clause is `.where(eq(discoveryState.id, decision.id))` — it never checks `version = currentVersion`. Every other entity (api-key, context, secret-configuration, site, column-configuration) uses `and(eq(table.id, id), eq(table.version, currentVersion))`. Without the version guard, two concurrent 'seen' events race: both read version N, both issue the update, both succeed — the second write clobbers the first. seenCount undercounts by 1 per collision, causing spotlights to not retire at RETIRE_AFTER_UNCLICKED_SHOWS=3. Fix: add `and(eq(discoveryState.id, decision.id), eq(discoveryState.version, decision.currentVersion))` and treat 0-rows-updated as DiscoveryErrors.databaseError('update').

discovery_state schema: seen_count DEFAULT 1 contradicts the seenCount=0 domain invariant

packages/database/drizzle/0058_closed_vermin.sql:11

Migration emits `seen_count integer DEFAULT 1 NOT NULL`. The insert query always supplies seenCount explicitly so the DB default is overridden — no current bug. But any future path that omits seenCount from values() (backfill, test factory, second insert path) silently writes seenCount=1 for a row that was never shown, corrupting the budget check (isBudgetSpent reads seenCount > 0). Default should be 0 to match the domain invariant.

Budget bypass via direct API: dismiss-without-prior-seen creates seenCount=0 row invisible to budget

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

isBudgetSpent requires seenCount > 0 && lastSeenAt > windowStart. A first-ever 'dismissed' outcome (no prior row) creates seenCount=0, which passes through the budget check — the daily slot is not consumed. The UI prevents this (seen fires before dismissed can be clicked), but a direct API call with outcome:'dismissed' bypasses this UX guarantee. An attacker who wants to permanently retire a signal key without consuming the budget could send 2x 'dismissed' via direct API calls. Low risk (requires auth + deliberate manipulation), but worth noting.

Handler comment describes wrong auth chain order (validate before requireOrgAccess)

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

File header says chain is 'withAuth → requireOrgAccess → validate' but code runs 'withAuth → validate → requireOrgAccess'. An authenticated user with invalid body receives 422 before 403. No security impact — neither response leaks sensitive information — but the comment is misleading.

No DELETE RLS policy on discovery_state for authenticated role

packages/database/drizzle/0058_closed_vermin.sql:27

Intentional: no delete endpoint exists. Confirmed correct. Note for future: if a 'reset spotlight' feature is added, a user-scoped DELETE policy will be needed.

Per-Lambda-instance rate limiting (not distributed) for signal state endpoint

packages/api/src/middleware/rate-limit.ts:14

checkRateLimit uses a module-level Map — each Lambda cold start has an independent counter. On Vercel serverless the effective per-user limit is 100 × N Lambda instances. Acknowledged by existing comment. Discovery endpoints are low-risk but the 4-DB-query getSpotlight could be an amplification target. Note for future if load increases.

DiscoveryFCIS namespace missing from domains/core/src/index.ts FCIS re-export block

domains/core/src/index.ts:74

Every other entity is re-exported as `export * as XxxFCIS from './xxx'` at line 299+. Discovery exports flat individual symbols instead. External callers (handler, cross-domain shell) must import raw symbols like `import { recordSpotlightOutcomeShell } from '@batu/core-domain'` bypassing the namespace pattern. Add `export * as DiscoveryFCIS from './discovery'` alongside ProfileFCIS, ApiKeyFCIS, etc.

Entity file-name prefix inconsistent: discovery-state.* vs discovery.* within same directory

domains/core/src/discovery/discovery-state.type.ts:1

Five files use the `discovery` prefix (discovery.decisions.ts, discovery.errors.ts, discovery.queries.ts, discovery.shells.ts, discovery/index.ts) but three use `discovery-state` (discovery-state.type.ts, discovery-state.mapper.ts, discovery-state.type-check.ts). The canonical form requires uniform {entity} prefix. Since the entity is 'discovery', rename the three files to discovery.type.ts, discovery.mapper.ts, discovery.type-check.ts.

Outbox omission comment does not cite ADR-016 by name

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

ADR-016 requires entity+outbox writes in the same transaction (non-negotiable). The shell intentionally skips the outbox with a substantive justification (spec §6: measurement is PostHog-side). The rationale is sound but the comment doesn't reference ADR-016, making it hard for a future auditor to confirm this was a conscious override. Add `// ADR-016 conscious deviation: no outbox — see Feature Discovery v3 spec §6`.

Cross-domain shell imports discoveryQueries flat from @batu/core-domain instead of DiscoveryFCIS namespace

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

Follows from the missing DiscoveryFCIS namespace. Once the namespace is added, update this import to `DiscoveryFCIS.discoveryQueries.findByProfileId(...)` to match the established pattern.

DiscoveryErrors.databaseError operation union excludes 'delete' without explanation

domains/core/src/discovery/discovery.errors.ts:26

All other entity error constructors include 'delete' in the operation union. The omission is correct (discovery_state rows are never deleted), but the asymmetry will surprise engineers. Add a comment: `// No 'delete' — discovery_state rows are only ever inserted or counter-patched`.

cfe_setup spotlight budget behavior is ambiguous and untested

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

isBudgetSpent() runs before any per-signal logic, so cfe_setup IS subject to the daily budget. The signal's 'suppression-exempt' comment (line 150) refers only to CFE-credential suppression, NOT the budget. An admin without a CFE credential who was shown any spotlight within 20h gets null — they cannot see the cfe_setup card until the budget window passes. If this is intentional, add a test. If cfe_setup should be budget-exempt (it's a blocking admin CTA), fix the code. Either way, the current behavior is untested and the spec comment is misleading.

RLS UPDATE USING predicate on discovery_state not tested (forged update by foreign session)

domains/cross-domain/src/__tests__/feature-discovery.integration.test.ts:200

The integration test verifies forged INSERT is rejected by the INSERT WITH CHECK policy. But it does not test that a user with a different authUid cannot UPDATE another profile's discovery_state row by knowing its UUID. The UPDATE USING policy (`profile_id IN (SELECT id FROM profiles WHERE auth_id = auth.uid())`) should block this, but it is untested. Add a test: ownerB session, call updateWithVersion with memberA's row id → assert 0 rows updated (null returned).

getDiscoverySpotlightShell error path (try/catch → FeatureDiscoveryDatabaseError) never exercised

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

The shell wraps DB calls in try/catch and returns FeatureDiscoveryErrors.databaseError(). No test injects a DB fault to exercise this path. Since discovery is non-critical UX (spotlight failing silently is acceptable), the risk is medium — but the handler's mapGetSpotlightError for the 'FeatureDiscoveryDatabaseError' tag is equally untested, meaning a real DB failure has unknown response behavior.

All signals retired simultaneously on a page → null not explicitly tested

domains/cross-domain/src/__tests__/feature-discovery.decisions.test.ts:152

Individual retirement tests exist, but no test retires monitoring, payment_monitoring, AND internal_site_ids simultaneously and asserts null. The constituent parts are exercised but the exhaustive-retirement case is implicit.

E2E credentials/api anchor test has no guard against org-switch side effects

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

The api_keys anchor is admin-only; the test assumes the active org at test time makes the user an admin. If another spec switches the active org to one where the user is a member, the button may not render. A comment explaining why this is safe (or an explicit org-switch reset) would prevent future flake.

SPOTLIGHT_ANCHOR_BY_GROUP declared inside ContractsTable component body — recreated on every render

apps/platform/src/app/[locale]/(dashboard)/bills/contratos/_components/ContractsTable.tsx:186

SPOTLIGHT_ANCHOR_BY_GROUP is a static constant with no dependency on props or state, but it is declared inside the component function. HEADER_SORT_FIELDS and HEADER_FILTER_FIELDS at lines 39-46 are correctly at module scope. Move SPOTLIGHT_ANCHOR_BY_GROUP to module scope alongside them.

FeatureSpotlight.tsx locate effect uses [anchorRect !== null] dep array with eslint-disable

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

The expression `[anchorRect !== null]` as a dependency array triggers an eslint-disable for exhaustive-deps. anchorRef is read inside the effect but omitted. No actual bug (anchorRef is a stable ref), but the suppression masks future real omissions. A named boolean state (`anchorFound`) would make the dependency explicit and let exhaustive-deps run normally.

FeatureSpotlight.tsx position useMemo provides no memoization benefit

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

position recomputes exactly when anchorRect changes — which is already a state update and triggers a re-render. useMemo over a state-derived value that updates every time the state updates provides no benefit. A plain const would be simpler.

feature-discovery.queries.ts sql<boolean> annotations are TypeScript-unverified

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

sql<boolean> and sql<number> annotations are correct at runtime (PG driver returns JS booleans; ::int cast handles bigint). But TypeScript cannot verify these — a future schema change making a column nullable would not be caught at compile time. The ::int cast on contractCount is particularly load-bearing for the budget logic.