Skip to content

fix(db): migrate rega_offerings.securities_offered_type to a list, and type-check the tests - #295

Open
sroussey wants to merge 2 commits into
mainfrom
claude/keen-knuth-d74u5n-rega-array
Open

fix(db): migrate rega_offerings.securities_offered_type to a list, and type-check the tests#295
sroussey wants to merge 2 commits into
mainfrom
claude/keen-knuth-d74u5n-rega-array

Conversation

@sroussey

Copy link
Copy Markdown
Contributor

Two sequenced findings on the Reg-A securities_offered_type array change: the test shape and the type gate first, so the migration lands compile-checked, then the migration.

(a) The array change was untested and unreachable by the type checker

securities_offered_type became string[] | null, but five test sites still assigned a scalar — RegAOfferingRepo.test.ts:34,58,155,354 and RegAQuery.test.ts:16. Nothing caught it: tsconfig.json carries "exclude": ["**/*.test.ts"] and vitest does not typecheck.

  • The five scalar assignments become arrays. The existing?.securities_offered_type ?? null carry-forward is untouched.
  • Multi-value round trip through saveOfferingAsOf: store, read back, element-wise equality, and again on a same-date replay (isStaleByAsOf admits an equal date, so build re-runs and the list has to survive that). The only multi-value coverage before this was the pure toSecuritiesOfferedTypes helper in isolation.
  • Real-backend round trip in schemaRoundTrip.sqlite.test.ts, driven by a new ALL_SECURITIES_OFFERED_TYPES fixture (the six-value combination that overflows 100 chars), with the same fixture asserted through the Postgres twin postgresSchemaParity.integration.test.ts. One fixture pins both backends: they differ in storage, they must not differ in what the repository hands back.
  • New tsconfig.test.json + bun run typecheck-tests.

⚠️ The CI step is NOT wired, and here is why

The new gate surfaces 157 pre-existing errors across 52 unrelated test files — schema fields added without updating fixtures (spac, spac_deal, spac_candidate, spac_merger_extraction), IExecuteContext gaining disown, SecFetchJob gaining response_type, subclassed tasks overriding static type, and so on. None are related to this change.

Per the brief, weakening the config to make that backlog pass would defeat its purpose, so the config + script land and the backlog is reported instead. Adding the workflow step today would turn CI red on every PR. Suggested follow-up: burn the backlog down (it is highly repetitive — a handful of fixture builders would cover most of it), then add

 - run: bun run typecheck-tests

after bun run build in .github/workflows/test.yml.

The reg-a area itself is clean under the new gate — two adjacent pre-existing errors in the two files being edited (RegAQuery.test.ts typing repo as a ServiceToken, and a TUnion → Record conversion in RegAOfferingSchema.test.ts) are fixed here so the touched files pass.

tsconfig.test.json clears files/exclude explicitly rather than leaving them unstated — extends inherits both — and clears composite/declaration/declarationMap/emitDeclarationOnly because the base is a composite declaration-emitting project.

(b) rega_offerings.securities_offered_type varchar(100) → text[] had no migration

Neither generic db setup catch-up pass can express a type change: planMissingColumns only ADDs, and planColumnAlignment acts only where declaredStringType() reports varchar/text — it returns {kind:"other"} for arrays (alignPostgresColumnTypes.ts:110-135,196-225). So an existing Postgres deployment kept varchar(100), the 57 Form 1-A filings kept failing STORE_ERROR, and db setup reported success. A fresh database got text[], so the two backends silently diverged.

Teaching alignPostgresColumnTypes about arrays would re-derive the storage emitter's array DDL rules from memory — precisely what the schemaTypeMirror allowlist exists to refuse. The allowlist is untouched. Instead: one hand-rolled named migration, alongside AddressRegionNullableMigration and backfillExtractorRunsOutcome.

src/storage/reg-a/RegASecuritiesOfferedTypeArrayMigration.ts, modelled on the address migration: isDryRun() bail → SEC_DB_TYPE probe → migratePostgres() / migrateSqlite(), in-memory no-op. Called from setupAllDatabases.ts immediately before the REGA_OFFERING_REPOSITORY_TOKENsetupDatabase(), mirroring the migrateAddressRegionNullable / ADDRESS_REPOSITORY_TOKEN pairing.

Postgres. Probes information_schema.columns WHERE table_schema = current_schema(); returns early on data_type = 'ARRAY' (idempotent) or an absent row (fresh DB). Otherwise one schema-qualified ALTER COLUMN ... TYPE text[] (via quote/currentSchemaName) whose USING maps NULL→NULL, blank→NULL, a well-formed {...} literal through ::text[], and anything else to ARRAY[col].

This is safe by construction: Postgres rejects an over-length value rather than truncating it — that rejection is exactly the 57 STORE_ERRORs — so every value that reached the column is complete, either a bare scalar or a well-formed literal. There is no truncated-literal case to defend against. Failures are re-thrown naming the table and the recovery (set the column NULL for offending rows, re-run db setup; sec extractor backfill 1-A --force refills), matching wrapRebuildError's tone.

SQLite. No DDL: the column is TEXT either way and SqliteTabularStorage already JSON-stringifies arrays, so a legacy multi-select was written as JSON and already reads back correctly under the new declared type. Only a legacy single selection is wrong — a bare string sqlToJsValue returns raw — so one idempotent UPDATE ... json_array(col) WHERE NOT (json_valid(col) AND json_type(col)='array') wraps those, logging the row count like the address rebuild.

Recovery for the 57 filings — free, via the ordinary sweep

The 57 Form 1-A filings that failed STORE_ERROR recover through the normal sec update forms sweep at no cost. 1-A is pure XML with no AI pass; ExtractorRunRepo anti-joins on successful runs, so those filings re-select themselves as soon as the column accepts arrays, and their STORE_ERROR dead letters clear via markResolved on the clean run.

They do not recover via sec extractor retry-dead-letters, which is version-gated for STORE_ERROR — the fix lives in the extractor's storage code, and no version bump happened here because the extractor was never wrong.

Rows that stored successfully need no re-extraction at all: the migration converts them in place.

Verification

bunx vitest run src/config/ src/storage/reg-a/
Test Files 18 passed | 1 skipped (19)
Tests 126 passed | 6 skipped (132)
bun run build # bun build + tsc, clean

New SQLite migration suite (RegASecuritiesOfferedTypeArrayMigration.sqlite.test.ts, 5 tests, real DB via withSqliteDb): a legacy scalar becomes an array through the repo; a pre-existing JSON array is byte-identical afterwards; a null stays null; a second run changes nothing; a value written through the repo round-trips.

The Postgres arm (postgresSchemaParity.integration.test.ts) is describe.skipIf(!SEC_PG_URL) and was not executed here — no Postgres available in this environment. It degrades the column back to varchar(100), inserts one bare scalar and one {"Debt","Other(describe)"} literal, re-runs setupAllDatabases(), and asserts data_type = 'ARRAY' plus both rows reading back as arrays, then re-runs setup to assert idempotence.

Docs

CLAUDE.md's schema-catch-up section now says this conversion is hand-rolled because the mirror declines arrays, and that both backends converge on "JSON array text on SQLite, text[] on Postgres". ⚠️ A warning sits next to the existing maintenance-window note: unlike the varchar widenings, varchar → text[] is not binary-coercible, so Postgres rewrites the heap under ACCESS EXCLUSIVE rather than only rebuilding indexes.


Generated by Claude Code

`securities_offered_type` became `string[] | null`, but five test sites still
assigned a scalar and nothing caught it: tsconfig.json excludes `**/*.test.ts`
and vitest does not typecheck.
- Fix the five scalar assignments to arrays.
- Add a multi-value round trip through `saveOfferingAsOf` (store, read back,
element-wise, and again on a same-date replay). The only multi-value coverage
was the pure `toSecuritiesOfferedTypes` helper in isolation.
- Add a real-backend round trip in `schemaRoundTrip.sqlite.test.ts` driven by a
new `ALL_SECURITIES_OFFERED_TYPES` fixture, and assert the same value through
the Postgres twin. The two backends differ in storage (JSON text vs `text[]`);
they must not differ in what the repository hands back.
- Add `tsconfig.test.json` + `bun run typecheck-tests` so a test that assigns
the wrong type to a schema field fails somewhere.
The CI step is deliberately NOT wired yet: the new gate surfaces 157
pre-existing errors across 52 unrelated test files. Weakening the config to make
that pass would defeat its purpose, so the backlog is reported instead.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Lgxtp7mQECdh7F2UT9CVwN
…place
`securities_offered_type` was re-declared `string[] | null`, but no existing
database was converted. Neither generic `db setup` catch-up pass can express a
type change — `planMissingColumns` only ADDs, and `planColumnAlignment` acts
only where `declaredStringType()` reports varchar/text, which is `other` for an
array. So an existing Postgres deployment kept `varchar(100)`, the 57 Form 1-A
filings kept failing STORE_ERROR, and `db setup` reported success; a fresh
database got `text[]`, so the two backends silently diverged.
Teaching the alignment planner about arrays would re-derive the storage
emitter's array DDL rules from memory — exactly what the `schemaTypeMirror`
allowlist exists to refuse. Hand-roll one named migration instead, alongside
`AddressRegionNullableMigration` and `backfillExtractorRunsOutcome`.
- Postgres: one schema-qualified `ALTER COLUMN ... TYPE text[]` whose `USING`
wraps a bare scalar and re-parses an existing array literal. Safe by
construction — Postgres rejects an over-length value rather than truncating
(that rejection IS the 57 failures), so no stored value is a truncated
literal. Idempotent via a `data_type = 'ARRAY'` probe.
- SQLite: no DDL. The column is TEXT either way and the storage layer already
JSON-stringifies arrays, so a legacy multi-select reads back correctly
already; one idempotent UPDATE wraps the legacy single selections.
Wired into `setupAllDatabases` immediately before the reg-a `setupDatabase()`,
mirroring the address pairing.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Lgxtp7mQECdh7F2UT9CVwN
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@sroussey@claude