feat/discovery

Migration 0061 CREATE POLICY statements are not wrapped in DO$$ idempotency blocks

packages/database/drizzle/0061_discovery_state.sql:26

The project's `migrations.md` documents a drizzle-kit@0.29.1 bug where `CREATE POLICY` statements in new migrations can fail with `policy "..." already exists` on Supabase branch DBs where the journal was stamped at branch creation but the migration was never actually run. The prescribed fix is to wrap every `CREATE POLICY` in a `DO $$ BEGIN ... EXCEPTION WHEN duplicate_object THEN null; END $$;` block. Migration 0061 has four bare `CREATE POLICY` statements with no such wrapping. Because `discovery_state` is a brand new table (not an existing table being altered), this failure mode is unlikely on a fresh DB — but it can still surface on a Supabase preview branch where the journal entry for 0061 was stamped at fork time and then the team manually applies the SQL directly per the fix runbook. The table DDL itself uses `CREATE TABLE IF NOT EXISTS` so it is idempotent; the policy statements are not. The fix is to wrap each `CREATE POLICY` in the same DO$$ pattern used for the FK constraint immediately above them (lines 17–21 of the file).

Debug removal is complete — no loose ends

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

The three removal sites (handler, shells, schema) are fully consistent. No stray references to `includeDebug`, `DiscoveryGapDebug`, or the `debug` response field remain in any .ts/.tsx file. The one remaining `VERCEL_ENV` reference in the handler (line 74) is the unrelated `relaxDailyBudget` flag for the spotlight handler, not the removed debug path.

preview-db wipe adding DROP SCHEMA devops CASCADE is correct and safe

packages/database/src/preview-db.ts:71

The `devops` schema is created by migration 0051 (`CREATE SCHEMA "devops"`) and is the only non-public, non-Supabase-managed schema in the codebase. The wipe comment explicitly documents this and uses an allowlist approach (never a blanket drop of all non-system schemas), which protects Supabase's managed schemas (auth, storage, extensions, graphql, realtime, vault). Without this drop, the from-scratch re-migrate after a wipe would fail on `CREATE SCHEMA "devops"` because the schema already exists. The `IF EXISTS` guard makes it safe on DBs that predate 0051. No correctness issue.

taxRates barrel position is benign — no circular dependency

packages/database/src/schema/index.ts:50

The `taxRates` export added at line 50 of `schema/index.ts` (between `utilityContracts` and `siteUtilityContracts`) is a merge artifact from the billing PR. The `tax-rates.ts` schema file has no imports from `site-utility-contracts.ts` or `utility-contracts.ts`, and those files have no imports from `tax-rates.ts`, so the insertion position creates no circular dependency. The barrel is for runtime re-export only; drizzle-kit uses the `!(index).ts` glob and never sees it.

The removed debug schema had a stale non-Zod artifact: isExpression: false

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

The removed `debug` object in `DiscoveryGapResponseSchema` contained `isExpression: false` as a plain object key — not a Zod method call. This was a copy-paste artifact that had no Zod meaning (it would have been ignored at runtime by Zod's `z.object()` parser, which only reads recognized Zod schema values). Its removal along with the rest of the debug block is correct and leaves no trace. Noted only because it would have been a latent TypeScript error if the schema had been used in a stricter context.

Migration idempotency: table DDL is safe but policy DDL is not

packages/database/drizzle/0061_discovery_state.sql:26

The full idempotency profile of 0061: (a) `CREATE TABLE IF NOT EXISTS` — safe, (b) `ALTER TABLE ... ENABLE ROW LEVEL SECURITY` — idempotent in Postgres (enabling RLS on an already-RLS-enabled table is a no-op), (c) FK `ADD CONSTRAINT` — wrapped in `DO $$ EXCEPTION WHEN duplicate_object` — safe, (d) `CREATE UNIQUE INDEX IF NOT EXISTS` / `CREATE INDEX IF NOT EXISTS` — safe, (e) four bare `CREATE POLICY` statements — NOT idempotent, will fail with `duplicate_object` if run twice. Items (a)–(d) are correctly guarded; only (e) is missing the guard.

preview-db wipe: fail-open ref-parse is acknowledged but staging ref is only covered by CI env var

packages/database/src/preview-db.ts:27

The prod-safety model has three layers: (1) bash wrapper slug deny-list (`main|master|staging|stg|prod|production|preview`) + Supabase host check + project-ref deny-list, (2) TypeScript `protectedRefForUrl()` check. The TS check explicitly 'fails OPEN' (returns null) when the ref cannot be parsed from the URL — documented as acceptable because the bash guards still apply. In CI, `BATU_PROD_PROJECT_REFS` is set to both prod and staging refs (`SUPABASE_PROD_PROJECT_REF ${{ secrets.SUPABASE_STG_PROJECT_REF }}`). However, the hardcoded `DEFAULT_PROD_REFS` constant in preview-db.ts covers only the prod ref (`tljxdspuxeyscdhbbzkn`), not staging. If `db:preview-wipe` is run locally without setting `BATU_PROD_PROJECT_REFS`, the TS ref-guard would not block a staging URL — only the bash slug-guard (`stg`) would. This is a defense-in-depth gap but not an active vulnerability given the bash wrapper's slug guard covers `stg`.

RLS: No DELETE policy on discovery_state — intentional and safe

packages/database/drizzle/0061_discovery_state.sql:26

discovery_state has SELECT, INSERT, UPDATE, and service_role_all policies but no DELETE policy. With RLS enabled, the Postgres default-deny rule blocks all authenticated DELETE attempts. The design is intentional: the `profiles` FK has `ON DELETE CASCADE`, so rows are cleaned up when the parent profile is deleted. The absence of a DELETE policy means authenticated clients (PostgREST) cannot delete rows directly, which is the correct posture for an exposure-tracking table. No attack surface is created.

Debug block removal: fully scrubbed, no information leakage risk remains

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

The removed debug path (`includeDebug: process.env.VERCEL_ENV === 'preview'`) would have exposed the raw signal state (feature-adoption booleans) and org segment to any authenticated user on preview deployments. The removal is complete: the `includeDebug` field is gone from `GetDiscoveryGapInput`, `DiscoveryGapDebug` type is deleted, the `debug` field is removed from `DiscoveryGapView` and `DiscoveryGapResponseSchema`, and no stray references remain in handlers, shells, or UI components. The benign `relaxDailyBudget` flag (budget relaxation, not data exposure) stays and is correctly scoped to preview only.

DROP SCHEMA devops CASCADE: correctly scoped to preview-only wipe path, multiple prod guards in place

packages/database/src/preview-db.ts:71

The new `DROP SCHEMA IF EXISTS devops CASCADE` call is inside the `wipe()` function, which is only reached when: (1) the script is invoked directly (not imported), (2) the mode argument is `wipe`, (3) all prod guards pass (slug deny-list, Supabase host check, project ref deny-list). The explicit schema list pattern with a comment ('NEVER a blanket drop all non-system schemas') is the correct approach — it avoids accidentally nuking Supabase-managed schemas (auth, storage, extensions, realtime, vault). The devops schema is confirmed to be created by migration 0051 (`CREATE SCHEMA devops` in `0051_tiny_makkari.sql`), making its inclusion in the wipe correct and necessary for a clean re-migrate.

SQL injection: no risk in preview-db wipe — dynamic SQL uses quote_ident, not string concatenation

packages/database/src/preview-db.ts:72

The `DO $$ ... LOOP` block in `wipe()` builds dynamic DDL with `quote_ident(r.tablename)` over values from `pg_tables WHERE schemaname = 'public'`. `quote_ident` properly escapes identifiers, and the table names come from a catalog query not from user input. The two `sql.unsafe()` calls for the `drizzle` and `devops` DROP statements use hardcoded strings with no interpolated user data. No injection surface exists.

Removed Zod schema contained `isExpression: false` — an invalid Zod value that would have thrown at runtime

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

The deleted `debug` field in `DiscoveryGapResponseSchema` included `isExpression: false` — a plain boolean, not a Zod schema. This is invalid: `z.object({ isExpression: false })` throws `ZodError: Expected ZodTypeAny, received boolean` when `z.object` is constructed. In practice it was dead code (gated on `VERCEL_ENV === 'preview'` at the handler level and the optional field would only be present in preview responses), so no prod breakage occurred. The removal is the correct fix, but worth noting: if a similar debug block is re-introduced, all fields must be valid Zod schemas (`z.literal(false)` not `false`).

Debug block removal is clean — FCIS patterns preserved

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

The removal of `includeDebug` and `DiscoveryGapDebug` from `feature-discovery.shells.ts` is correct. The `DiscoverySignalState` type import is removed while the `getDiscoverySignalState` function import is retained (still used in the providers branch). `isAdmin` remains correctly forwarded to the decision. No inline Drizzle SQL was introduced. Shell follows the read-coordinator pattern (ADR-016 §Pattern 7): no writes, no outbox, transaction boundary left to the handler.

RLS policy naming follows the established convention exactly

packages/database/drizzle/0061_discovery_state.sql:27

Migration 0061 uses `discovery_state_service_role_all`, `discovery_state_select_own`, `discovery_state_insert_own`, `discovery_state_update_own` — identical in structure to `profile_roles_select_own`, `profiles_update_own`, and `user_preferences` analogs. The INSERT policy correctly emits only `WITH CHECK` (no `USING` clause, which Postgres silently ignores on INSERT anyway but would be a deviation). The Drizzle schema source (`discovery-state.ts`) also defines the INSERT policy without a `using` key. Consistent with the `rls-checklist.md` canonical shape.

`taxRates` export placement in schema/index.ts is a benign merge artifact

packages/database/src/schema/index.ts:50

`taxRates` is inserted between `utilityContracts` and `siteUtilityContracts` — not alphabetical and slightly out of logical order, but barrel re-export ordering in `index.ts` has no runtime effect and `drizzle.config.ts` uses the `!(index).ts` glob that excludes the barrel. No dependency or typecheck issue. Consider moving it to the end of the utility-domain block in a follow-up cleanup commit if ordering hygiene matters to the team.

preview-db wipe correctly adds `devops` schema to the explicit drop list

packages/database/src/preview-db.ts:65

The fix adds `DROP SCHEMA IF EXISTS devops CASCADE` before the per-table loop, matching the comment's stated policy ('explicit list — NEVER a blanket drop all non-system schemas'). The log message is updated to reflect the change. The guard against Supabase-managed schemas (`auth`, `storage`, `extensions`, `graphql`, `realtime`, `vault`) is preserved. No issues.

getDiscoveryGapShell has zero integration test coverage

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

The integration test file never calls `getDiscoveryGapShell`. The providers vs non-providers short-circuit (`orgSegment !== 'providers'` → empty items), `discoveryActive` flag, `tourSeen` propagation, and the checklist-item content all go untested at the integration level. This pre-existed the diff but these cleanup commits leave the gap intact. A basic integration test covering the non-providers short-circuit (returns empty with `discoveryActive: false`) and the providers happy path (returns checklist items) would close the gap.

preview-db wipe() is structurally untestable; devops DROP addition is unverified by tests

packages/database/src/preview-db.ts:71

The `wipe()` function in `packages/database/src/preview-db.ts` (line 63) executes raw `sql.unsafe()` calls and is not exported or injectable. The existing test suite correctly limits itself to the three pure parsing helpers (`projectRefFromUrl`, `protectedRefForUrl`, `parseProdRefs`). The new `DROP SCHEMA IF EXISTS devops CASCADE` statement (line 71) therefore has no unit or integration test coverage. The only way to verify it is operational (a GHA preview-reset run). This is an acceptable structural trade-off given the risk profile — the statement is idempotent and the only production risk is a wipe on a wrong DB, which is guarded by the prod-ref deny-list that IS tested. Noting it for awareness rather than blocking.

Debug path removal needed no test cleanup

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

No tests ever exercised the `includeDebug` / `DiscoveryGapDebug` path in `getDiscoveryGapShell`. The integration test file (`domains/cross-domain/src/__tests__/feature-discovery.integration.test.ts`) only covers `getDiscoverySpotlightShell` and the `recordSpotlightOutcomeShell` counter/RLS paths. Nothing to clean up — the removal is clean.

Migration 0061 is covered indirectly via RLS integration tests

packages/database/drizzle/0061_discovery_state.sql

Migration `packages/database/drizzle/0061_discovery_state.sql` creates `discovery_state` with RLS policies. The integration tests in `feature-discovery.integration.test.ts` exercise all three RLS policies (select-own isolation, insert own with-check, update forged-profile block) via real transactions against the live schema. No standalone migration test is needed — the integration suite is the verification mechanism.

DiscoveryGapResponseSchema has no dedicated schema validation test

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

The `DiscoveryGapResponseSchema` in `packages/api/src/schemas/discovery.schemas.ts` (line 48) has no test in `packages/api/src/__tests__/`. The removed `debug` field had a clearly broken `isExpression: false` property in it (a literal `false` where a Zod type was expected) — had there been a schema parse test, this would have been caught earlier. A smoke test (`DiscoveryGapResponseSchema.parse(validShape)`) would guard against future regressions of the same type.

preview-db wipe list drifts from drizzle.config.ts schemaFilter

packages/database/src/preview-db.ts:68

The non-public schemas to drop during a wipe are maintained as a manual explicit list in `preview-db.ts` (currently `['drizzle', 'devops']`). The canonical list of non-system schemas the project owns already exists in `drizzle.config.ts` as `schemaFilter: ['public', 'devops']`. These are now two places to update when a new non-public schema is added. The comment acknowledges the list is manual ('the only one today') but does not point to `drizzle.config.ts` as the companion. A future contributor adding a third schema (e.g. `analytics`) would update `drizzle.config.ts` for generate/migrate but could silently miss `preview-db.ts`, causing the wipe to leave a stale schema and subsequent re-migration to fail on `CREATE SCHEMA`. A low-friction improvement would be to add a comment cross-referencing `drizzle.config.ts schemaFilter` as the place that must stay in sync, making the coupling explicit without requiring runtime coupling (which would add a build-time import to a CLI utility). A harder improvement — deriving the drop list from `schemaFilter` at runtime — is possible but adds fragility since `drizzle.config.ts` throws at import if `POSTGRES_URL` is unset.

getDiscoverySignalState and DiscoverySignalState are over-exported from the cross-domain barrel

domains/cross-domain/src/index.ts:31

After the debug block removal, `DiscoverySignalState` and `getDiscoverySignalState` are re-exported from the cross-domain barrel (`src/index.ts` lines 31 and 34) but are not consumed by any external package. The grep across `apps/` and `packages/` returns zero hits for imports of these two names from `@batu/cross-domain`. Both are used exclusively WITHIN the cross-domain package (shells and queries) and tested in `__tests__/feature-discovery.decisions.test.ts`. Exporting them makes them part of the public API surface of `@batu/cross-domain`, which creates an implicit contract that consumers could depend on. The previous justification for exporting them was likely the debug handler path (the `DiscoveryGapDebug` interface imported `DiscoverySignalState` for the wire schema), which is now gone. Removing both from the barrel would tighten the module boundary and signal to future maintainers that signal-state internals are not a stable cross-package contract.

taxRates export added between utility contract exports — ordering is slightly incongruous

packages/database/src/schema/index.ts:50

The `taxRates` export (added by the merge from main) was inserted between `tariffRates` and `siteUtilityContracts` — inside the utility domain block, which is the right section. However it appears before `siteUtilityContracts`, `bills`, `billFiles` and other operationally heavier tables, while `taxRates` is a small reference/catalog table. The ordering is minor but the pattern in this file is roughly: core tables → utility reference tables → utility operational tables. `taxRates` as catalog data fits best alongside `tariffRates`. In its current position the ordering is logically consistent and not a correctness issue — this is purely a readability note for the next time this file is touched.

getGapHandler response could use direct spread from result.value

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

After the debug field was removed, `DiscoveryGapView` now has exactly three fields (`items`, `tourSeen`, `discoveryActive`) and the response object in the handler reconstructs all three explicitly. Since `DiscoveryGapView` is a readonly interface with no internal-only fields (no `id`, no debug, no derived fields that should be hidden), `return success(result.value)` would be equally correct and would automatically stay in sync if new fields are added to the view. The current explicit destructuring is the safe canonical pattern (api-patterns.md: 'only expose publicId, never internal id') and is appropriate if the view type could ever grow internal fields — but for a read-only coordinator result that already maps cleanly to the wire shape, it adds a maintenance seam. This is the weakest finding; noting for completeness.