feat/discovery

Stale `parked` state persists when RpuSubTour re-opens in the same session

apps/platform/src/components/discovery/RpuSubTour.tsx:82

The `parked` state is initialised to `false` via `useState(false)` but is never reset when the sub-tour closes and re-opens. The `useEffect` at line 82 resets `openedRef` and calls `setStepIdx(0)` when `!active`, but does not call `setParked(false)`. If a user parks (advances to a FORM_STEP without having entered an RPU), then dismisses the sub-tour (X / Saltar), and then re-opens it by clicking 'Nuevo RPU' again, `parked` is still `true` when the tour re-opens at step 0 ('upload'). The TourOverlay renders `hint={parked ? t('needRpu') : undefined}`, so the 'upload' step (which has nothing to do with RPU input) shows the misleading 'Ingresa un RPU válido para continuar.' hint until the user first clicks Next. The hint clears on the first handleStep call (line 128: `setParked(false)`), so it is cosmetically wrong for one interaction but does not block progress. Fix: add `setParked(false)` to the `useEffect` branch where `!active`.

`firstRunTourPending` defaults TRUE while query is in-flight, briefly suppressing ambient spotlight on contratos for tour-completed users

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

When `enabled=true` but the spotlight query has not resolved yet (`response` is undefined), `firstRunTourPending` evaluates to `true`. The guard at line 191 (`if (!requested && page === TOUR_GATED_PAGE && firstRunTourPending) return null`) then suppresses the ambient coachmark on the contratos page. For users who completed the tour, there is a brief window on every page load where the spotlight is hidden until the query resolves. The code comment and PR description explicitly document this as an intentional trade-off to avoid the race between the tour mount and the ambient coachmark mount (which previously caused the card to flash and burn the 20h budget). This is by design, not a bug, but noting it in case the UX becomes an issue at scale (e.g., slow API response on a cold session).

`isFirstRunTourPending` uses a bare string literal 'product_tour' rather than the exported `TOUR_KEY` constant

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

The helper at line 92-95 hard-codes `e.key === 'product_tour'`. The canonical string is also exported as `TOUR_KEY = 'product_tour' as const` from `apps/platform/src/components/discovery/tour-catalog.ts`, and appears as a union member in `DiscoveryFeatureKey` in `domains/core/src/discovery/discovery-state.type.ts`. The three sources are currently consistent so there is no live bug. However, `tour-catalog.ts` lives in the client (`apps/platform/src`), which the server-side shell cannot import (wrong dependency direction). The domain type (`DiscoveryFeatureKey`) is the authoritative location; if `product_tour` were ever renamed in the union the shell's literal would silently become wrong. A comment referencing the domain type as the SSOT would reduce drift risk, but this is not a current defect.

No auto-advance when user fills RPU while sub-tour is parked — requires manual Next click

apps/platform/src/components/discovery/RpuSubTour.tsx:124

When the sub-tour parks at a FORM_STEP (sync/submit) because `syncMounted` is false, it displays the hint and waits. There is no `useEffect` watching the DOM for the `[data-subtour='sync']` element to appear (which happens once the user types a valid RPU and the `InlineContractForm` renders). The user must click Next again manually after entering the RPU. The code comment acknowledges this ('Not a dead-end: Saltar (X) and the CTA stay live'), so the design is intentional. The behaviour is correct in the sense that nothing is blocked, but a MutationObserver or a polling effect on `syncMounted` could provide a smoother experience. This is a noted UX limitation, not a correctness defect.

Unsafe cast: `eligibleKeys` typed as `readonly SpotlightFeatureKey[]` but server schema permits `product_tour`

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

The client casts `response.body.data.eligibleKeys as readonly SpotlightFeatureKey[]`, but the Zod schema that validates the server response is `z.array(DiscoveryFeatureKeySchema)` and `DiscoveryFeatureKeySchema` includes `'product_tour'` — a value that is absent from the client-side `SpotlightFeatureKey` union. If `product_tour` were ever included in the returned array, the TypeScript cast would silently lie at runtime. In practice this cannot happen today because `listEligibleKeysOnPage` filters only entries from `DISCOVERY_SIGNAL_CATALOG`, and `product_tour` is intentionally absent from that catalog. The defence is a runtime invariant in the decision function, not the type contract. The same latent mismatch exists for `spotlightKey` (already present before this diff). Mitigation: narrow `DiscoveryFeatureKeySchema` used for the spotlight response to exclude `product_tour` (introduce a `SpotlightFeatureKeySchema` that omits it), or add a `.exclude(['product_tour'])` step in `SpotlightResponseSchema.eligibleKeys`. This makes the contract self-enforcing instead of relying on the decision function never emitting the value.

TOUR_RETURN_PARAM is unauthenticated client-controlled input — trivially forged, but impact is limited to showing an extra toast

apps/platform/src/app/[locale]/(dashboard)/credentials/cfe/page.tsx:88

`offerTourResume` fires whenever `searchParams.get(TOUR_RETURN_PARAM) === '1'`. An attacker can craft `/credentials/cfe?tour_return=1` and share it; any user who opens that URL and saves (or updates) their CFE credentials will see a toast with a `router.push` to a hardcoded tour URL. The href contains only compile-time constants (`TOUR_PAGE_PREFIX`, `TOUR_PARAM`, `TOUR_STEP_PARAM`, `TOUR_RESUME_STEP_AFTER_CFE`) — no user input is interpolated into the URL, so there is no open-redirect or injection risk. The consequence is a spurious toast that could mildly confuse a user. Not exploitable for data exfiltration or privilege escalation; acceptable as-is, but worth noting for completeness.

RLS and auth correctly scoped — no cross-user or cross-org leakage via new fields

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

The new `isFirstRunTourPending()` helper reads from `exposures`, which are fetched via `discoveryQueries.findByProfileId(db, input.profileId)`. The `profileId` is resolved from the authenticated JWT inside `withAuth` (never from a query param). The shell is called inside `createRLSDb(database, authReq.auth.userId).transaction(...)`, and `requireOrgAccess` is the first gate. Exposure rows in `discovery_state` are user-scoped by RLS policy (own rows only, per the comment in `recordOutcomeHandler`). A cross-org probe against `getDiscoverySignalState` sees zero rows and returns silence. No authorization issue introduced by this diff.

isFirstRunTourPending() is a pure function placed in shells.ts instead of decisions.ts

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

isFirstRunTourPending() has no async, no I/O, and no side effects — it is a pure boolean predicate over an exposure list. Per ADR-016 and the canonical form, pure business logic belongs in .decisions.ts so it is testable without the shell's async harness and its intent is clear at the right layer. Its placement in .shells.ts is the exact anti-pattern the FCIS separation is designed to prevent: a reader must look in the shell file to understand a pure business rule ('the tour is pending until the user has seen, dismissed, or clicked it'). The comment on the function even says 'ONE definition, shared by both shells' — that is precisely the case for moving it to decisions.ts and importing it from there. The two shells would then import it from feature-discovery.decisions.ts alongside listEligibleKeysOnPage, decideSpotlight, etc.

eligibleKeys client cast bypasses ts-rest response type but is safe in practice

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

The cast `response.body.data.eligibleKeys as readonly SpotlightFeatureKey[]` is needed because the ts-rest client types the response as the inferred Zod shape (DiscoveryFeatureKey[]), while the client-side catalog uses SpotlightFeatureKey which excludes 'product_tour'. The cast is safe because listEligibleKeysOnPage() is bounded by DISCOVERY_SIGNAL_CATALOG which never includes 'product_tour' — but the type widening is invisible at the call site. The same cast exists two lines above for spotlightKey (line 121). A cleaner pattern would be to add a type guard or a schema-derived client type that explicitly excludes 'product_tour', or to align SpotlightFeatureKey with DiscoveryFeatureKey and filter at use. Not a runtime bug given the current catalog, but a correctness time-bomb if 'product_tour' were ever added to the signal catalog.

Ref mutation during render at eligibleKeys guard is consistent with existing pattern but undocumented

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

Line 144 mutates latchRef.current during the render function body to drop a stale latched key. The existing pattern at line 130 already does the same with computeSpotlightLatch, and React explicitly allows ref mutation during render as a 'previous renders' memo. The comment at line 130 calls this out ('Mutating the ref during render is a pure derivation from the ref's own prior value'). The new mutation at line 144 follows the same pattern but lacks the explanatory comment, leaving a future reader to wonder whether it is safe or an accidental mutation. A short comment ('same as line 130 — drop stale latch; ref mutation during render is safe per React docs') would make intent explicit. Not an architectural violation, but a conventions gap.

tourStepIndex() throws at module load — fail-fast is correct but undocumented as intentional

apps/platform/src/components/discovery/tour-catalog.ts:112

tourStepIndex() is called at module init time to derive TOUR_RESUME_STEP_AFTER_RPU and TOUR_RESUME_STEP_AFTER_CFE. If either id ('recibos', 'rpu') is renamed or removed from TOUR_STEPS, the module throws on import, crashing the component tree before any UI renders. The comment ('Throws at module load if the id is gone, so a renamed/removed step fails loudly instead of silently re-targeting a hand-off') justifies the behavior. This is a good pattern (fail-fast over silent wrong index), well-documented inline. Flagged only to confirm this is intentional — it is. No action needed.

MCP server rule is not applicable — discovery endpoint is not on the public /v1 surface

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

The mcp-server.md rule requires that new fields on public-v1 endpoints be mirrored in the MCP tool definitions. The discovery contract (getSpotlight, getGap, recordOutcome) is mounted under the internal /api/* surface, not under /api/v1/*. Grep of apps/platform/src/api/contracts/public-v1/index.ts confirms no discovery routes are exported on the public surface. The new firstRunTourPending and eligibleKeys fields are internal-only additions and the MCP rule does not apply.

listEligibleKeysOnPage() correctly returns array not Result — pure query functions need not wrap in Result

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

listEligibleKeysOnPage() returns DiscoveryFeatureKey[] (not Result<DiscoveryFeatureKey[], E>). The canonical form says decisions return Result<T,E> for fallible operations, but pure query functions with no failure modes (this one can only return an empty list, never fail) may return the value directly — same pattern as listEligibleSpotlights(), listDiscoveryChecklist(), decideSpotlight() which all return bare values. No I/O, no async, no side effects. Fully consistent with the existing decisions in this file and with ADR-016 §Pattern.

FORM_STEPS hardcodes step ids that could be derived from RPU_SUBTOUR_STEPS

apps/platform/src/components/discovery/RpuSubTour.tsx:46

FORM_STEPS is defined as `new Set(['sync', 'submit'])` — the ids of every step that comes AFTER the 'rpu' step in RPU_SUBTOUR_STEPS. These ids are already declared in tour-catalog.ts. If a step is inserted before 'sync', or a step is renamed, FORM_STEPS silently goes stale and the park guard either never fires (user advances onto an unrendered step) or fires on the wrong step. The relationship is structural: FORM_STEPS = steps after the 'rpu' gate. Derive it once from the source array: `const rpuIdx = RPU_SUBTOUR_STEPS.findIndex(s => s.id === 'rpu'); const FORM_STEPS: ReadonlySet<string> = new Set(RPU_SUBTOUR_STEPS.slice(rpuIdx + 1).map(s => s.id));`. This mirrors the tourStepIndex() pattern already adopted in tour-catalog.ts and removes the silent drift risk.

Parked state offers no visual affordance that clicking Siguiente again is meaningless — user may loop

apps/platform/src/components/discovery/RpuSubTour.tsx:100

When parked is true, the hint text ('Ingresa un RPU válido para continuar.') appears under the step body, but the Siguiente button remains active and clickable. Clicking it calls handleStep(stepIdx + 1) again, hits the same FORM_STEPS guard again, stays parked, and shows the same hint. The user sees no feedback that their click was processed — the card appears identical before and after the click. Two improvements would eliminate the confusion: (1) disable the Siguiente button (or visually grey it) while parked so it is not clickable at all; (2) or change the button label to something like 'En espera' to signal the blocked state. The hint text alone does not convey that Siguiente is temporarily inert, which is the UX gap.

Consumer org short-circuit returns structured firstRunTourPending:false — semantic mismatch

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

For consumer orgs (where discovery is fully disabled), the shell returns `{ spotlightKey: null, firstRunTourPending: false, eligibleKeys: [] }`. The `firstRunTourPending: false` value means 'the tour is not pending', but the real reason is 'discovery is not active for this org'. The client's suppression logic reads `firstRunTourPending` and uses false to mean 'tour resolved, show ambient', but for consumers the ambient is off anyway because spotlightKey is null and the provider check happens independently. There is no functional bug here today, but the value is semantically misleading — if the client logic ever evolves to check firstRunTourPending independently of the spotlightKey result, returning false for a disabled path could produce incorrect behavior. A comment documenting 'false here means not-applicable, not completed' would be the minimum; an optional field (`firstRunTourPending: boolean | null` where null = not-applicable) would be the cleanest.

tourStepIndex() throws at module load — a compile-time tuple assertion would be safer

apps/platform/src/components/discovery/tour-catalog.ts:112

tourStepIndex() uses Array.findIndex + a runtime throw to guard against a renamed/removed step id. The throw fires at module evaluation time (the constants are module-level), so it will surface immediately in any environment that imports tour-catalog.ts — which is a reasonable safety net. An alternative that enforces this at compile time instead: define TOUR_STEPS as a const tuple with explicit id literals (`as const` already applies), then derive the index via a generic type-level helper. However, TypeScript does not support `findIndex` in the type system for string-literal unions, so the compile-time path requires an explicit id→index map object. Given the small catalog size (5 steps), the runtime throw is pragmatic. One actual improvement: add a test that imports tour-catalog.ts and asserts both exported constants are non-negative numbers — this turns the module-load crash into a named test failure that surfaces in CI rather than silently breaking a production page load.

listEligibleKeysOnPage duplicates the Map-construction pattern already in decideSpotlight

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

Both listEligibleKeysOnPage (line 288) and decideSpotlight (line 386) independently construct `new Map(exposures.map(e => [e.key, e]))` before calling eligibleCandidatesOnPage. In the shell, listEligibleKeysOnPage and decideSpotlight are called in sequence with the same exposures array, meaning the Map is constructed twice. This is cheap at current catalog size (< 10 entries), but the duplication could be eliminated by having eligibleCandidatesOnPage accept the pre-built Map directly (it already does — it takes a ReadonlyMap parameter), and having callers build the Map once. The shell could construct the Map once and pass it to both decision calls. Minor allocation savings, but more importantly it removes the repeated pattern.

eligibleKeys cast to readonly SpotlightFeatureKey[] bypasses Zod schema alignment check

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

The response body is typed via ts-rest + Zod, but `response.body.data.eligibleKeys` is cast with `as readonly SpotlightFeatureKey[]` rather than being used as-is. This cast is the same pattern used two lines above for spotlightKey (line 121). The underlying Zod schema uses DiscoveryFeatureKeySchema, which at runtime validates correctly. The cast itself is harmless since SpotlightFeatureKey and DiscoveryFeatureKey are the same set, but it signals a type alignment gap between the API schema type and the client-side type alias. If they ever diverge (a new key added to one but not the other), the cast would silently hide a type mismatch. Prefer narrowing via a type predicate or explicit guard rather than a cast.

isFirstRunTourPending() has no direct unit tests — three distinct conditions untested

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

isFirstRunTourPending() is a private module-level function that implements the canonical definition shared by both shells (spotlight + gap). It has three distinct branches: (a) no 'product_tour' exposure row exists → true, (b) exposure exists but seenCount=0, dismissCount=0, clickedAt=null → true, (c) any counter > 0 or clickedAt set → false. The integration test at feature-discovery.integration.test.ts only asserts result.value.spotlightKey on the shell results; neither firstRunTourPending nor the isFirstRunTourPending branches are asserted anywhere. Case (b) is particularly subtle — a row exists but all counters are zero is still 'pending'. Because this function is the SSOT for tour-suppression vs tour-seen, a regression here silently breaks either the ambient suppression (coachmark stacks under the tour) or the tour resumption (hub shows tour as already done). The integration test scope should be extended to assert result.value.firstRunTourPending under the three conditions: fresh user with no exposure row, exposure row with all-zero counters, and exposure row with seenCount > 0.

eligibleKeys latch-clear guard has no unit test — the stale-card fix is completely untested

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

The eligibleKeys latch-clear guard (lines 139–145 of FeatureSpotlight.tsx) is the fix for the 'Registra tu primer RPU' coachmark persisting after the user registers an RPU. The guard mutates latchRef directly during render: if the current latch key is not in eligibleKeys, it resets to null. The spotlight-latch.test.ts suite (apps/platform/src/components/discovery/__tests__/spotlight-latch.test.ts) tests computeSpotlightLatch in isolation but has no case for the eligibleKeys-driven reset. There is no test asserting: 'latch holds key K; server responds with eligibleKeys=[] (capability adopted); latch should drop K rather than keep it'. The self-seen protection test ('KEEPS the shown key when the query later resolves null') specifically tests null spotlightKey — but eligibleKeys-driven clearing is a distinct code path that runs even when spotlightKey comes back null for a different reason (budget spent). These two null-returning cases need to be distinguishable in tests.

offerTourResume() on credential UPDATE path has no test coverage

apps/platform/src/app/[locale]/(dashboard)/credentials/cfe/page.tsx:184

The offerTourResume() call was extracted and added to handleUpdate (line 184) as the explicit fix for users who already had credentials being left stranded after the tour's CFE step. Previously it was only called from handleCreate. This is a behavior change — the update path now shows the 'Resume tour' toast when ?tour_return=1 is present. There is no test (unit, integration, or E2E) that exercises the handleUpdate path with ?tour_return=1 in searchParams to verify the toast fires. The existing test suite has no component tests for the credentials page at all. At minimum, an E2E test for the update path or a unit test of offerTourResume with a mock searchParams containing tour_return=1 is needed to pin this regression.

TOUR_GATED_PAGE suppression (firstRunTourPending=true on tour page) has no test

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

The conditional at line 191 ('if (!requested && page === TOUR_GATED_PAGE && firstRunTourPending) return null') is the core fix: ambient coachmark does not appear on /bills/contratos while the first-run tour has never been seen. There is no unit test or E2E test that asserts this suppression. The discovery-anchors.spec.ts tests only anchor presence (not coachmark rendering), and FeatureSpotlight.tsx has no component tests at all. A targeted test should verify three states: (1) page=TOUR_GATED_PAGE + firstRunTourPending=true → no coachmark rendered; (2) page=TOUR_GATED_PAGE + firstRunTourPending=false → coachmark can render; (3) page=TOUR_GATED_PAGE + firstRunTourPending=true but requested is set → coachmark still renders (explicit pull is exempt). Without this, the tour/ambient stacking race is tested only by manual dogfooding, and the condition could silently invert.

RpuSubTour parked state logic has no unit tests — forward/park/clear cases untested

apps/platform/src/components/discovery/RpuSubTour.tsx:124

The parked state introduced in RpuSubTour.tsx handles the case where the user advances to a FORM_STEPS step (sync/submit) before a valid RPU is entered, preventing a tooltip that points at unmounted DOM. The handleStep callback has four distinct code paths: (1) goingForward + targetId=rpu + syncMounted → skip to next + setParked(false); (2) goingForward + FORM_STEPS.has(targetId) + !syncMounted → setParked(true), return without advancing; (3) any navigation → setParked(false) + advance; (4) backward navigation → advance without parked check. None of these paths are tested. The handleStep function is a pure-ish callback that can be extracted and unit-tested with a boolean syncMounted param. Missing cases include: park clears on backward navigation, park clears when the form mounts and user retries, and the RPU skip case when syncMounted=true.

tourStepIndex() throw-on-missing behavior has no test — a renamed step would fail silently until runtime

apps/platform/src/components/discovery/tour-catalog.ts:112

tourStepIndex() was introduced to derive TOUR_RESUME_STEP_AFTER_RPU and TOUR_RESUME_STEP_AFTER_CFE from TOUR_STEPS by id rather than hardcoded integers, with an explicit throw if the id is missing. The throw happens at module load time (the constants are module-level), so a renamed step is caught immediately. However, there is no test that imports tour-catalog.ts and asserts the derived values are the expected indices (3 for 'recibos', 2 for 'rpu' based on the current TOUR_STEPS array). Without a test pinning these values, a step insertion or reorder silently changes the resume target — the throw only catches outright removal. A simple test in the existing vitest suite importing TOUR_RESUME_STEP_AFTER_RPU and asserting it equals the index of 'recibos' in TOUR_STEPS (and likewise for CFE→'rpu') would pin this invariant.

Integration test does not assert firstRunTourPending or eligibleKeys on the spotlight shell response

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

The getDiscoverySpotlightShell integration tests (lines 217–299) assert result.value.spotlightKey but never assert result.value.firstRunTourPending or result.value.eligibleKeys. These are new fields on DiscoverySpotlightView added in this commit and flow through the handler to the client. Without integration-level assertions, a regression where firstRunTourPending is hardcoded false or eligibleKeys is empty (e.g., a refactor that drops the listEligibleKeysOnPage call) would pass all existing tests. The consumer org case explicitly returns firstRunTourPending: false (line 114 of shells.ts) — that specific value should be asserted in the consumer org integration test. At minimum one provider-org case should assert firstRunTourPending: true for a fresh user and confirm eligibleKeys is non-empty when eligible signals are present.

SpotlightResponseSchema contract not validated against handler output shape in tests

packages/api/src/schemas/discovery.schemas.ts

The SpotlightResponseSchema in packages/api/src/schemas/discovery.schemas.ts was extended with firstRunTourPending (z.boolean()) and eligibleKeys (z.array(DiscoveryFeatureKeySchema)). The handler wires these fields at apps/platform/src/api/handlers/discovery.handler.ts lines 92–93. There is no schema-roundtrip test that validates a mock shell result against SpotlightResponseSchema — if the handler mapper omitted one of these fields, the Zod parse would fail at runtime only, not in CI. A lightweight type-level test (using satisfies or a Zod.parse against a fixture) in the api package tests would catch this class of mapper-vs-schema drift.