diff --git a/.changeset/decrypt-chaining-docs.md b/.changeset/decrypt-chaining-docs.md new file mode 100644 index 000000000..19bcbb8ea --- /dev/null +++ b/.changeset/decrypt-chaining-docs.md @@ -0,0 +1,72 @@ +--- +'stash': patch +'@cipherstash/stack': patch +--- + +Correct the shipped documentation for `decryptModel` / `bulkDecryptModels`. + +Three places in `skills/stash-encryption` and four in `packages/stack/README.md` +said these return "a plain `Promise>` (not a chainable operation)" +and that there is therefore "no `.withLockContext()` to chain". They return an +`AuditableDecryptModelOperation`, which is thenable and carries both +`.withLockContext()` and `.audit()` — the same `.audit()` chain the +audit-on-decrypt work advertises. The skill contradicted itself: its own +reference table already listed the correct return type. + +The skill ships inside the `stash` tarball and `installSkills()` copies it into +customer repos, so this was steering agents away from an API that exists. The +README ships in the `@cipherstash/stack` tarball. + +The equivalent statement about the **WASM entry** is correct and unchanged — +`@cipherstash/stack/wasm-inline` really does return a plain promise from decrypt, +with no lock-context argument. + +Also fixes the setup prompt `stash init` writes for coding agents, which +referenced `protectOps.eq` — an API that does not exist anywhere in the repo. +Every step naming an integration-specific API now branches on the project's +actual integration, instead of naming Drizzle's and Supabase's and leaving the +other two to guess: + +- **Query paths.** `createEncryptionOperators(client)` (conventionally `ops`) + for Drizzle, the `encryptedSupabase` wrapper's own filters for Supabase, the + `eql*` column operators for Prisma Next, and `client.encryptQuery(...)` for a + plain Postgres project. +- **Schema authoring.** The `types.*` column factories for Drizzle, the + `eql_v3_encrypted` domain in migration SQL for Supabase, the `cipherstash.*` + field constructors in `schema.prisma` for Prisma Next, and `encryptedTable` + for plain Postgres. Prisma Next was previously sent at `types.*` / + `encryptedTable` — the client `stash schema build` explicitly refuses to + scaffold for that integration. +- **Read paths.** `decryptModel(row, usersSchema)` where that applies, and the + wrapper's transparent decryption where it does not. +- **Skill pointers.** A plain Postgres project installs no integration-specific + skill, so each "see the integration skill" was a pointer at a file that was + never written. Those now point at `stash-encryption`, which it does get. + +`client.encryptQuery` is also shown taking the schema objects themselves +(`{ table: usersSchema, column: usersSchema.email }`) rather than an +object-shorthand that read as three required strings — `queryType` is inferred +from the column's configured indexes. + +The cutover and complete-rollout **plan templates** now split EQL v3 from v2. +Both described the v2 rename swap (`` → `_plaintext`, twin → ``) +as the only cutover path, so on the default v3 install `stash plan` drafted a +plan built around `stash encrypt cutover` — a command that refuses v3 columns +outright ("there is no rename step") and refuses entirely on a v3-only +database, where `eql_v2_configuration` does not exist. The implement prompt +already carried this split; the plan templates did not. The version is +per-column, so the templates tell the agent to establish it per column rather +than deciding once for the whole plan. + +The "already encrypted" stop-and-ask now recognises `eql_v3_*` domains +alongside the legacy `eql_v2_encrypted` udt, so it can fire on the default +path at all. + +**`stash init` now detects already-encrypted columns on EQL v3.** Database +introspection marked a column as CipherStash-managed only when its udt was +exactly `eql_v2_encrypted`. v3 columns carry per-domain types +(`eql_v3_text_search`, `eql_v3_integer_ord`, …), so on the default path every +encrypted column was reported as plaintext — shown with its `dataType` and left +unticked in the column picker, inviting a re-run to encrypt it a second time. +The picker also labelled any encrypted column with the literal string +`eql_v2_encrypted`; it now shows the column's real domain. diff --git a/.changeset/dynamodb-eql-v3.md b/.changeset/dynamodb-eql-v3.md index c6303c997..54b960c92 100644 --- a/.changeset/dynamodb-eql-v3.md +++ b/.changeset/dynamodb-eql-v3.md @@ -11,10 +11,13 @@ Pass a table built with `encryptedTable` + the `types.*` domains from the typed client from `EncryptionV3` and the nominal client from `Encryption({ config: { eqlVersion: 3 } })` are accepted. -EQL v2 tables continue to work unchanged — this is additive, and no existing -caller needs to change. The table decides which wire format is used, so a -DynamoDB table populated under one version must keep being read with that -version. +EQL v2 tables continue to be **readable** — `decryptModel` / +`bulkDecryptModels` still accept one, so existing items stay accessible. Writing +through a v2 table is a separate matter: `encryptModel` / `bulkEncryptModels` +narrowed to EQL v3 tables in this same release, so a caller that still encrypts +through a v2 table does need to change. The table decides which wire format is +used, so a DynamoDB table populated under one version must keep being read with +that version. This fixes a latent bug that made v3 unusable: the write path detected an encrypted value by its `k: 'ct'` tag, but EQL v3 scalars carry no `k` @@ -71,4 +74,4 @@ type, where a declared column `email` becomes `email__source` (plus `email`. `decryptModel` / `bulkDecryptModels` invert it via `DecryptedAttributes`. `AnyEncryptedTable`, `DynamoDBEncryptionClient` and `AuditConfig` are now exported from `@cipherstash/stack/dynamodb` so these signatures can be named. -The EQL v2 overloads are unchanged. +The EQL v2 **decrypt** overloads are unchanged; the v2 encrypt overloads are removed in this release. diff --git a/packages/cli/src/commands/init/lib/__tests__/introspect.test.ts b/packages/cli/src/commands/init/lib/__tests__/introspect.test.ts index 40a011e95..1efca50e3 100644 --- a/packages/cli/src/commands/init/lib/__tests__/introspect.test.ts +++ b/packages/cli/src/commands/init/lib/__tests__/introspect.test.ts @@ -5,6 +5,7 @@ import type { DataType } from '../../types.js' import { candidateDomains, defaultDomain, + isEqlEncryptedDomain, pgTypeToDataType, selectTableColumns, } from '../introspect.js' @@ -103,6 +104,44 @@ describe('defaultDomain', () => { }) }) +// The marker used to be `udt_name === 'eql_v2_encrypted'` alone. v3 is the +// default generation and its columns carry `eql_v3_*` domains, so on the +// default path an already-encrypted column was reported as plaintext: shown +// with its `dataType` hint and left unticked. `packages/wizard` already made +// this call, and its comment names the consequence — misreporting encrypted +// columns as plaintext lets an agent clobber real ciphertext. +describe('isEqlEncryptedDomain', () => { + it('recognises every eql_v3_* domain', () => { + for (const udt of [ + 'eql_v3_encrypted', + 'eql_v3_text_search', + 'eql_v3_integer_ord', + 'eql_v3_date_ord', + 'eql_v3_json', + ]) { + expect(isEqlEncryptedDomain(udt)).toBe(true) + } + }) + + it('still recognises the legacy v2 udt', () => { + expect(isEqlEncryptedDomain('eql_v2_encrypted')).toBe(true) + }) + + it('does not claim plaintext types', () => { + for (const udt of ['text', 'int4', 'jsonb', 'bool', 'timestamptz']) { + expect(isEqlEncryptedDomain(udt)).toBe(false) + } + }) + + // The trailing underscore is load-bearing: a bare `startsWith('eql_v3')` + // would also claim a hypothetical future `eql_v30_*` generation. Same + // reasoning as `classifyEqlDomain` in `@cipherstash/migrate`. + it('does not claim a future generation by prefix', () => { + expect(isEqlEncryptedDomain('eql_v30_text_search')).toBe(false) + expect(isEqlEncryptedDomain('eql_v3')).toBe(false) + }) +}) + describe('selectTableColumns', () => { beforeEach(() => { selectMock.mockReset() @@ -214,6 +253,38 @@ describe('selectTableColumns', () => { expect(schema?.columns).toEqual([{ name: 'ssn', domain: 'TextSearch' }]) }) + // The per-column hint was the literal string 'eql_v2_encrypted' for any + // encrypted column, so a v3 column was labelled with a domain it does not + // have. Report the column's actual udt. + it('hints an encrypted column with its real domain, not a hardcoded v2 udt', async () => { + const withV3 = [ + { + tableName: 'accounts', + columns: [ + { + columnName: 'ssn', + dataType: 'jsonb', + udtName: 'eql_v3_text_search', + isEqlEncrypted: true, + }, + ], + }, + ] + selectMock + .mockResolvedValueOnce('accounts') + .mockResolvedValueOnce('TextSearch') + multiselectMock.mockResolvedValueOnce(['ssn']) + + await selectTableColumns(withV3) + + const opts = multiselectMock.mock.calls[0][0].options + expect(opts).toEqual([ + expect.objectContaining({ value: 'ssn', hint: 'eql_v3_text_search' }), + ]) + // And it starts ticked, so a re-run does not offer to re-encrypt it. + expect(multiselectMock.mock.calls[0][0].initialValues).toEqual(['ssn']) + }) + it('pre-selects the widest searchable domain as the per-column default', async () => { // Asserts the ARGS to the domain prompt, not just its return: the picker // must pass initialValue: defaultDomain(options) so the widest searchable diff --git a/packages/cli/src/commands/init/lib/__tests__/setup-prompt.test.ts b/packages/cli/src/commands/init/lib/__tests__/setup-prompt.test.ts index b1b684910..ee1c302b7 100644 --- a/packages/cli/src/commands/init/lib/__tests__/setup-prompt.test.ts +++ b/packages/cli/src/commands/init/lib/__tests__/setup-prompt.test.ts @@ -105,6 +105,117 @@ describe('renderSetupPrompt — orient + route (implement mode)', () => { expect(out).toContain('pnpm exec drizzle-kit migrate') }) + // The "wire the column through" step names the query API by hand. It used to + // name `protectOps.eq`, which exists nowhere; naming `createEncryptionOperators` + // unconditionally is the same mistake one step smaller — that symbol is + // exported only by `@cipherstash/stack-drizzle`, which a Supabase or Prisma + // project does not even have in its dependency tree. Each integration gets + // the API it can actually import. + describe('query-operator guidance is per-integration', () => { + const render = (integration: SetupPromptContext['integration']) => + renderSetupPrompt({ ...baseCtx, integration }) + + it('names the Drizzle operators for drizzle', () => { + const out = render('drizzle') + expect(out).toContain('createEncryptionOperators(client)') + expect(out).toContain('ops.eq') + }) + + it('names the wrapper builder for supabase, not the Drizzle operators', () => { + const out = render('supabase') + expect(out).not.toContain('createEncryptionOperators') + expect(out).toContain('encryptedSupabase') + }) + + it('names the eql* column operators for prisma-next', () => { + const out = render('prisma-next') + expect(out).not.toContain('createEncryptionOperators') + expect(out).toContain('eqlEq') + }) + + // `postgresql` is the default when nothing is detected, and it gets no + // integration skill at all (install-skills.ts) — so "see the integration + // skill" is a dangling pointer for it too. + it('names the core encryptQuery path for plain postgresql', () => { + const out = render('postgresql') + expect(out).not.toContain('createEncryptionOperators') + expect(out).toContain('encryptQuery') + expect(out).toContain('stash-encryption') + }) + + // `EncryptQueryOptions` takes the schema OBJECTS — `usersSchema` and + // `usersSchema.email` — not their names as strings, and `queryType` is + // inferred from the column's indexes when omitted. The object-shorthand + // `{ table, column, queryType }` reads as three required string fields: + // the same species of plausible-but-wrong API as the `protectOps.eq` this + // PR exists to have removed. + it('shows encryptQuery taking schema objects, not string names', () => { + const out = render('postgresql') + expect(out).toContain('column: usersSchema.email') + expect(out).not.toContain('{ table, column, queryType }') + }) + + // `packages/cli` builds with tsup, which transpiles without type-checking, + // and has no `typecheck` script — so `Record` buys nothing + // at build time here. An if-chain ending in a bare Drizzle `return` hands a + // future fifth integration the exact string this helper exists to stop it + // getting. Degrade to neutral guidance, as `skillsFor()` degrades to + // BASE_SKILLS. + it('degrades to neutral guidance for an unrecognised integration', () => { + const out = renderSetupPrompt({ + ...baseCtx, + integration: 'mystery-orm' as SetupPromptContext['integration'], + }) + expect(out).not.toContain('createEncryptionOperators') + expect(out).not.toContain('encryptedSupabase') + expect(out).toContain('encryptQuery') + }) + }) + + // Step 2 named the Drizzle and Supabase schema APIs unconditionally — the + // identical defect step 5 above was just fixed for. A prisma-next project was + // sent at `types.*` / `encryptedTable`, which `stash schema build` explicitly + // refuses to scaffold for that integration; a plain-Postgres project was + // given no named path at all. + describe('schema-authoring guidance is per-integration', () => { + const render = (integration: SetupPromptContext['integration']) => + renderSetupPrompt({ ...baseCtx, integration }) + + it('names the Drizzle column factories for drizzle', () => { + const out = render('drizzle') + expect(out).toContain('`types.*` column factories') + expect(out).toContain('@cipherstash/stack-drizzle') + }) + + it('names the eql_v3 domain for supabase', () => { + expect(render('supabase')).toContain('eql_v3_encrypted') + }) + + it('names the cipherstash.* field constructors for prisma-next', () => { + const out = render('prisma-next') + expect(out).toContain('cipherstash.TextSearch()') + expect(out).toContain('prisma/schema.prisma') + // The `types.*` client is precisely what `stash schema build` refuses to + // emit for prisma-next. + expect(out).not.toContain('`types.*` column factories') + }) + + it('names encryptedTable for plain postgresql', () => { + const out = render('postgresql') + expect(out).toContain('encryptedTable') + expect(out).toContain('@cipherstash/stack/v3') + }) + }) + + // `postgresql` installs no integration-specific skill (SKILL_MAP), so every + // unconditional "see the integration skill" is a pointer at a file that was + // never written. Step 5's was fixed; two more survived elsewhere. + it('never points plain postgresql at "the integration skill"', () => { + const out = renderSetupPrompt({ ...baseCtx, integration: 'postgresql' }) + expect(out).not.toMatch(/the integration skill/i) + expect(out).toContain('stash-encryption') + }) + it('emits supabase migration commands for supabase integration', () => { const out = renderSetupPrompt({ ...baseCtx, @@ -438,7 +549,12 @@ describe('renderSetupPrompt — no db push recommendations', () => { expect(out).toMatch(/1\.\s*\*\*Schema-add/) expect(out).toMatch(/2\.\s*\*\*Dual-write/) // Cutover is still covered, just without a db push workaround note. - const cutoverSection = out.substring(out.indexOf('#### Encryption cutover')) + // The heading has to be one that exists: `indexOf` returning -1 makes + // `substring(-1)` the whole document, so this scoped assertion was + // silently asserting nothing at all. + const heading = '#### Backfill and switch' + expect(out).toContain(heading) + const cutoverSection = out.substring(out.indexOf(heading)) expect(cutoverSection).toMatch(/encrypt cutover/) }) @@ -450,6 +566,66 @@ describe('renderSetupPrompt — no db push recommendations', () => { }) }) +// The rename swap is an EQL v2 operation. `stash encrypt cutover` refuses a v3 +// column outright ("Cut-over is not applicable to EQL v3 columns … there is no +// rename step", `encrypt/cutover.ts`) and refuses entirely on a v3-only +// install, where `eql_v2_configuration` does not exist. v3 is the default, so +// a template presenting the rename as the only path drafts a plan around a +// command that will decline to run. The implement prompt already splits the +// two; these templates did not. +describe('renderSetupPrompt — plan templates split EQL v3 from v2', () => { + const plan = (planStep: 'cutover' | 'complete') => + renderSetupPrompt({ ...baseCtx, mode: 'plan', planStep }) + + for (const planStep of ['cutover', 'complete'] as const) { + describe(`${planStep} plan`, () => { + it('names both versions and states v3 has no rename', () => { + const out = plan(planStep) + expect(out).toContain('EQL v3') + expect(out).toContain('EQL v2') + expect(out).toMatch(/no rename/i) + }) + + it('does not present the rename swap unconditionally', () => { + const out = plan(planStep) + // Every mention of the rename swap has to sit next to the v2 label. + for (const line of out.split('\n')) { + if (/_plaintext/.test(line)) { + expect(line).toMatch(/v2|v3/) + } + } + }) + + it('tells the agent how to determine the version per column', () => { + // The version is per-column — a database can hold both — so this + // cannot be decided once for the whole plan. + expect(plan(planStep)).toMatch(/encrypt status|encrypt backfill/) + }) + }) + } + + // The stop-and-ask trigger for "already encrypted" named the v2 UDT alone. + // v3 is the default and its columns carry `eql_v3_*` domains, so on the + // default path the check silently never fired. The agent reads the schema + // itself, so this half is fixable here; `introspect.ts`'s `isEqlEncrypted` + // has the same gap and is a separate behavioural change. + it('recognises v3 domains in the already-encrypted stop-and-ask', () => { + for (const mode of ['implement', 'plan'] as const) { + const out = renderSetupPrompt({ ...baseCtx, mode }) + expect(out).toMatch(/eql_v3_/) + } + }) + + it('keeps the cutover invocation scoped to v2', () => { + const out = plan('cutover') + const cutoverLine = out + .split('\n') + .find((l) => l.includes('encrypt cutover --table')) + expect(cutoverLine).toBeDefined() + expect(cutoverLine).toMatch(/v2/) + }) +}) + describe('renderSetupPrompt — honours what the handoff actually wrote', () => { for (const mode of ['implement', 'plan'] as const) { it(`claude-code with no skills points at neither a skills dir nor AGENTS.md (${mode})`, () => { diff --git a/packages/cli/src/commands/init/lib/introspect.ts b/packages/cli/src/commands/init/lib/introspect.ts index 2e4a4c732..31b7bb398 100644 --- a/packages/cli/src/commands/init/lib/introspect.ts +++ b/packages/cli/src/commands/init/lib/introspect.ts @@ -42,12 +42,36 @@ export function pgTypeToDataType(udtName: string): DataType { } } +/** + * Is this column already managed by CipherStash? + * + * Both generations count. v3 is the sole generation this workspace authors, + * and its columns carry per-domain types (`eql_v3_text_search`, + * `eql_v3_integer_ord`, …) rather than v2's single `eql_v2_encrypted` udt — + * so keying on the v2 name alone, as this did, reported every column on the + * default path as plaintext. That is the dangerous direction to be wrong in: + * an encrypted column shown as plaintext invites the caller to encrypt it a + * second time. `packages/wizard` already carries this predicate; the two + * should agree. + * + * Deliberately not `classifyEqlDomain` from `@cipherstash/migrate`, despite + * the dependency being present: that answers "which generation authors this", + * and returns `null` for `eql_v2_encrypted` because v2 is no longer authorable. + * The question here is "is this encrypted at all", which v2 answers yes to. + * + * The trailing underscore matters — a bare `eql_v3` prefix would also claim a + * hypothetical future `eql_v30_*`. + */ +export function isEqlEncryptedDomain(udtName: string): boolean { + return udtName === 'eql_v2_encrypted' || udtName.startsWith('eql_v3_') +} + /** * Read every base table in the `public` schema along with its columns. * - * The `eql_v2_encrypted` UDT marker tells us a column is already managed by - * CipherStash — useful for re-runs against a partially set up DB so we can - * pre-select those columns rather than asking the user to reconfirm. + * The EQL domain markers tell us a column is already managed by CipherStash — + * useful for re-runs against a partially set up DB so we can pre-select those + * columns rather than asking the user to reconfirm. */ export async function introspectDatabase( databaseUrl: string, @@ -85,7 +109,7 @@ export async function introspectDatabase( columnName: row.column_name, dataType: row.data_type, udtName: row.udt_name, - isEqlEncrypted: row.udt_name === 'eql_v2_encrypted', + isEqlEncrypted: isEqlEncryptedDomain(row.udt_name), }) tableMap.set(row.table_name, cols) } @@ -203,7 +227,7 @@ export function defaultDomain( * Returns `undefined` if the user cancels at any prompt — callers should * propagate the cancellation rather than treating it as "no columns selected". * - * Pre-selects columns that are already `eql_v2_encrypted` so re-running on a + * Pre-selects columns that already carry an EQL domain so re-running on a * partially encrypted DB is a no-op by default. */ export async function selectTableColumns( @@ -230,7 +254,7 @@ export async function selectTableColumns( if (eqlColumns.length > 0) { p.log.info( - `Detected ${eqlColumns.length} column${eqlColumns.length !== 1 ? 's' : ''} with eql_v2_encrypted type — pre-selected for you.`, + `Detected ${eqlColumns.length} already-encrypted column${eqlColumns.length !== 1 ? 's' : ''} (${[...new Set(eqlColumns.map((c) => c.udtName))].join(', ')}) — pre-selected for you.`, ) } @@ -239,7 +263,7 @@ export async function selectTableColumns( options: table.columns.map((col) => ({ value: col.columnName, label: col.columnName, - hint: col.isEqlEncrypted ? 'eql_v2_encrypted' : col.dataType, + hint: col.isEqlEncrypted ? col.udtName : col.dataType, })), required: true, initialValues: eqlColumns.map((c) => c.columnName), diff --git a/packages/cli/src/commands/init/lib/setup-prompt.ts b/packages/cli/src/commands/init/lib/setup-prompt.ts index c0e22ba21..ac97a3687 100644 --- a/packages/cli/src/commands/init/lib/setup-prompt.ts +++ b/packages/cli/src/commands/init/lib/setup-prompt.ts @@ -68,6 +68,94 @@ function migrationCommands( return undefined } +/** + * Where this integration's rules were actually written. + * + * `postgresql` is the fallback integration and installs no integration-specific + * skill (see `SKILL_MAP` in `install-skills.ts`), so "the integration skill" is + * a pointer at a file that was never created. Point it at `stash-encryption`, + * which it does get. + */ +function integrationSkillRef(integration: Integration): string { + switch (integration) { + case 'drizzle': + case 'supabase': + case 'prisma-next': + return 'the integration skill' + default: + return 'the `stash-encryption` skill' + } +} + +/** + * How this integration declares an encrypted column in the schema. + * + * Per-integration for the same reason the query operators are: the APIs are not + * interchangeable, and `stash schema build` (`utils.ts`) refuses outright to + * scaffold a `types.*` client for prisma-next — that integration authors its + * columns with `cipherstash.*` constructors in `schema.prisma` instead. + */ +function schemaAuthoringGuidance(integration: Integration): string { + switch (integration) { + case 'drizzle': + return 'Declare it with the `types.*` column factories from `@cipherstash/stack-drizzle` on the existing `pgTable`' + case 'supabase': + return 'Declare the column with the `eql_v3_encrypted` domain in the migration SQL (`encryptedSupabase` derives its encryption config by introspecting those domains)' + case 'prisma-next': + return 'Declare the field with the `cipherstash.*` constructors in `prisma/schema.prisma` (`cipherstash.TextSearch()`, `cipherstash.DoubleOrd()`, …), with the `cipherstash` extension pack wired up per `@cipherstash/prisma-next/control`' + default: + return 'Declare the table with `encryptedTable` and the `types.*` domain factories from `@cipherstash/stack/v3`, then pass it to `Encryption({ schemas })`' + } +} + +/** + * How this integration filters on an encrypted column. Named per-integration + * rather than generically because the APIs are not interchangeable and only one + * of them is importable from any given project: `createEncryptionOperators` is + * exported by `@cipherstash/stack-drizzle` alone, so naming it for a Supabase + * or Prisma project sends the agent after a package that is not installed. + * + * A `switch` with a neutral `default`, not an if-chain ending in the Drizzle + * string: `packages/cli` is built by tsup, which transpiles without + * type-checking, and the package has no `typecheck` script — so nothing would + * catch a fifth `Integration` variant silently inheriting Drizzle's answer. + * `skillsFor()` in `install-skills.ts` degrades the same way, for the same + * reason. + */ +function queryOperatorGuidance(integration: Integration): string { + switch (integration) { + case 'supabase': + return 'query paths filter through the `encryptedSupabase` wrapper (`es.from("users").select(...).eq("email", value)`) — it encrypts filter operands for encrypted columns automatically; see the integration skill' + case 'prisma-next': + return 'query paths use the `eql*` operators on the column inside `.where()` (`u.email.eqlEq(value)`, `eqlMatch`, `eqlGt`, …) — see the integration skill' + case 'drizzle': + return 'query paths use the right operator (`ops.eq`, from `createEncryptionOperators(client)`) — see the integration skill' + default: + // `table` and `column` are the schema OBJECTS, not their names as + // strings, and `queryType` is inferred from the column's configured + // indexes unless it is passed to override the inference. + return 'query paths encrypt the search term first — `client.encryptQuery(value, { table: usersSchema, column: usersSchema.email })`, passing the schema objects themselves rather than their names — and compare that against the encrypted column; see the `stash-encryption` skill (a plain Postgres project gets no integration-specific skill)' + } +} + +/** + * How this integration turns ciphertext back into values on the read path. + * + * Named per-integration for the third time in this file, and for the third + * time because the answer is not portable: `decryptModel` is the typed + * client's, transparent decryption is the Supabase wrapper's. + */ +function readPathGuidance(integration: Integration): string { + switch (integration) { + case 'supabase': + return 'selects through the `encryptedSupabase` wrapper decrypt transparently' + case 'prisma-next': + return 'the encrypted fields decrypt through the Prisma Next client' + default: + return 'call `decryptModel(row, usersSchema)` — or `bulkDecryptModels` for a set — before returning the value to callers' + } +} + function bullet(line: string): string { return `- ${line}` } @@ -274,13 +362,13 @@ export function renderImplementPrompt(ctx: SetupPromptContext): string { '', '### Add a new encrypted column', '', - 'Use when the column **does not yet exist** in the database (no plaintext predecessor to preserve). This is normal Drizzle / Supabase work plus the encryption client patterns from the integration skill.', + `Use when the column **does not yet exist** in the database (no plaintext predecessor to preserve). This is normal schema work in the project's own ORM or migration tooling, plus the encryption client patterns from ${integrationSkillRef(ctx.integration)}.`, '', "1. **If this is the first encrypted column in the project, configure the bundler exclusion first.** `@cipherstash/stack` cannot be bundled (it wraps a native FFI module). Next.js: add `serverExternalPackages: ['@cipherstash/stack', '@cipherstash/protect-ffi']` to `next.config.*`. Webpack: `externals`. esbuild: `external`. Vite SSR: `ssr.external`. Without this, the encryption client crashes at runtime with `Cannot find module '@cipherstash/protect-ffi-*'`. See the `stash-encryption` skill's Installation section for the full snippets.", - "2. Edit the user's real schema file (`src/db/schema.ts` or wherever they keep it) to declare the new encrypted column. Use the patterns in the integration skill — the `types.*` domain factories from `@cipherstash/stack-drizzle` for Drizzle, and the `types.*` factories from `@cipherstash/stack/eql/v3` (via `encryptedTable`, passed as `schemas`) for Supabase. Encrypted columns must be **nullable `jsonb`** at creation time. Never `.notNull()`.", + `2. Edit the user's real schema file (\`src/db/schema.ts\` or wherever they keep it) to declare the new encrypted column. ${schemaAuthoringGuidance(ctx.integration)} — the patterns are in ${integrationSkillRef(ctx.integration)}. Encrypted columns must be **nullable \`jsonb\`** at creation time (the \`eql_v3_*\` domains are over \`jsonb\`). Never \`.notNull()\`.`, `3. Generate the schema migration${migration ? ` — \`${migration.generate}\` (${migration.tool})` : " using the project's existing migration tooling"}.`, `4. Show the user the generated SQL before applying${migration ? ` — \`${migration.apply}\`` : ''}.`, - '5. Wire the column through the application code: insert paths encrypt before write, select paths decrypt after read, query paths use the right operator (`protectOps.eq`, etc. — see the integration skill).', + `5. Wire the column through the application code: insert paths encrypt before write, select paths decrypt after read, ${queryOperatorGuidance(ctx.integration)}.`, '6. Verify with a round-trip: insert a record, select it back, confirm the value decrypts and the search ops work.', '', '### Migrate an existing column to encrypted', @@ -304,7 +392,7 @@ export function renderImplementPrompt(ctx: SetupPromptContext): string { '', `3. **Backfill.** Run \`${cli} encrypt backfill --table --column \`. The CLI prompts the user (or accepts \`--confirm-dual-writes-deployed\` non-interactively) to confirm dual-writes are live, then chunks through the existing rows. Resumable; checkpoints to \`cs_migrations\` after every chunk. SIGINT-safe.`, `4. **Switch reads to the encrypted column.** The step depends on the EQL version (\`${cli} encrypt backfill\` prints it; \`${cli} encrypt status\` shows it). **EQL v3 (the default):** there is no rename — update the schema and queries to read/write the encrypted column by its own name, and wire decryption through the encryption client. **EQL v2 (legacy data only):** update the schema file to declare the encrypted column under its final name (drop the twin suffix), then \`${cli} encrypt cutover --table --column \` runs the rename in one transaction (\`\` → \`_plaintext\`, twin → \`\`). Do **not** declare a v2 column with a \`types.*\` domain — those are EQL v3 only. The adapters no longer author v2 (\`@cipherstash/stack-drizzle\` removed \`encryptedType\`), so a v2 column is a read path: declare it with the deprecated \`@cipherstash/stack/schema\` builders and decrypt through \`@cipherstash/stack\`.`, - '5. **Wire the read path through the encryption client.** The read column now holds ciphertext. Read code paths must decrypt before returning the value to callers — `decryptModel(row, table)` for Drizzle, the `encryptedSupabase` wrapper for Supabase, or the equivalent `decrypt`/`bulkDecryptModels` calls. Without this step, your read paths return raw encrypted payloads to end users. The integration skill has the exact API.', + `5. **Wire the read path through the encryption client.** The read column now holds ciphertext — ${readPathGuidance(ctx.integration)}. Without this step, your read paths return raw encrypted payloads to end users. See ${integrationSkillRef(ctx.integration)} for the exact API.`, '6. **Remove the dual-write code.** The plaintext column (still `` on v3; renamed `_plaintext` on v2) is no longer authoritative. Delete the dual-write logic from the persistence layer.', `7. **Drop.** Run \`${cli} encrypt drop --table --column \`. Generates a migration that removes the now-unused plaintext column (on v3 it first verifies no rows are still plaintext-only). Apply with the project's normal migration tooling.`, '', @@ -326,7 +414,7 @@ export function renderImplementPrompt(ctx: SetupPromptContext): string { "The user asks to convert a populated column in place. Explain why it doesn't work and offer the migrate-existing-column flow instead.", ), bullet( - "A column the user names is already encrypted (`eql_v2_encrypted` udt) but with a different EQL config than they've described. This is the post-cutover re-encryption case (`stash encrypt update`, not yet shipped) — surface it instead of guessing.", + "A column the user names is already encrypted — an `eql_v3_*` domain (`eql_v3_text_search`, `eql_v3_integer_ord`, …) on the default path, or the legacy `eql_v2_encrypted` udt — but with a different EQL config than they've described. This is the post-cutover re-encryption case (`stash encrypt update`, not yet shipped) — surface it instead of guessing.", ), bullet( 'The schema migration would change the data type of a column the user has already filled.', @@ -428,7 +516,7 @@ function planSharedStopAndAsk(): string[] { "The user asks to convert a populated column in place. Explain why it doesn't work and offer the migrate-existing-column flow instead.", ), bullet( - "A column the user names is already encrypted (`eql_v2_encrypted` udt) but with a different EQL config than they've described. This is the post-cutover re-encryption case (`stash encrypt update`, not yet shipped) — surface it in the plan as a flagged risk.", + "A column the user names is already encrypted — an `eql_v3_*` domain (`eql_v3_text_search`, `eql_v3_integer_ord`, …) on the default path, or the legacy `eql_v2_encrypted` udt — but with a different EQL config than they've described. This is the post-cutover re-encryption case (`stash encrypt update`, not yet shipped) — surface it in the plan as a flagged risk.", ), bullet( 'You discover existing partial CipherStash setup that disagrees with what the user is describing — someone else may have run `stash init` earlier with different choices. Note this in the plan and ask the user to clarify before writing prescriptive steps.', @@ -556,19 +644,16 @@ function renderCutoverPlanPrompt(ctx: SetupPromptContext): string { '**Backfill.** Encrypt the historical rows that pre-date the rollout deploy. Resumable; chunked; SIGINT-safe.', ), bullet( - '**Schema rename.** Update the schema declaration so the original column points at the encrypted type.', - ), - bullet( - '**Cutover.** A single transaction renames `` → `_plaintext` and `_encrypted` → ``.', + `**Switch reads to the encrypted column.** This step depends on the column's EQL version, so establish it per column first — \`${cli} encrypt backfill\` prints it and \`${cli} encrypt status\` shows it. **EQL v3 (the default):** there is no rename. Update the schema declaration and the queries to read and write \`_encrypted\` under its own name. **EQL v2 (legacy data only):** declare the encrypted column under its final name (drop the twin suffix), then a single \`${cli} encrypt cutover\` transaction renames \`\` → \`_plaintext\` and \`_encrypted\` → \`\`.`, ), bullet( - '**Read path.** Application reads of `` now return ciphertext until the read path decrypts via the encryption client. The plan must specify what changes per read site.', + '**Read path.** Reads of the encrypted column return ciphertext until the read path decrypts via the encryption client. The plan must specify what changes per read site.', ), bullet( - '**Remove dual-writes.** The plaintext column is now `_plaintext` and is no longer authoritative. Delete the dual-write code paths.', + '**Remove dual-writes.** The plaintext column — still `` on v3, renamed `_plaintext` on v2 — is no longer authoritative. Delete the dual-write code paths.', ), bullet( - "**Drop plaintext.** `stash encrypt drop` emits a migration that removes `_plaintext`. Apply with the project's normal migration tooling.", + `**Drop plaintext.** \`${cli} encrypt drop\` emits a migration that removes the now-unused plaintext column; on v3 it first verifies no rows are still plaintext-only. Apply with the project's normal migration tooling.`, ), '', '## Your task: produce the cutover plan file', @@ -591,12 +676,12 @@ function renderCutoverPlanPrompt(ctx: SetupPromptContext): string { ' encrypt backfill` invocation with concrete `--table` / `--column` values.', ), bullet( - 'The schema-edit step, with the exact rename pattern (drop `_encrypted` suffix on the encrypted column, switch the original column declaration off `text`/`varchar` and onto the encrypted type).', + 'The schema-edit step, stated per column against its EQL version. **v3:** point the declaration and queries at `_encrypted` under its own name — there is no rename, and no `_encrypted` suffix to drop. **v2:** drop the `_encrypted` suffix and switch the original column declaration off `text`/`varchar` and onto the encrypted type.', ), bullet( - 'The cutover invocation per column: `' + + 'For EQL v2 columns only, the cutover invocation per column: `' + cli + - ' encrypt cutover --table --column `.', + ' encrypt cutover --table --column `. It refuses v3 columns — there is nothing to rename — so do not schedule it for them.', ), bullet( 'Read-path code changes: every site that reads `` from this table must decrypt via the encryption client. Enumerate the sites you can find via grep so the user can verify nothing was missed.', @@ -608,7 +693,7 @@ function renderCutoverPlanPrompt(ctx: SetupPromptContext): string { ' encrypt drop --table --column `, plus the schema-migration apply step that follows.', ), bullet( - `Risks specific to cutover: row-count for the backfill (use \`${cli} eql status\` to estimate if helpful), tables under heavy write load (cutover holds a brief lock on the rename), application code that constructs SQL by string (those reads won't transparently decrypt).`, + `Risks specific to cutover: row-count for the backfill (use \`${cli} eql status\` to estimate if helpful), tables under heavy write load (on v2, cutover holds a brief lock for the rename), application code that constructs SQL by string (those reads won't transparently decrypt).`, ), bullet( "Open questions for the user — anything you can't determine from the schema, context.json, or the skills.", @@ -653,7 +738,7 @@ function renderCompletePlanPrompt(ctx: SetupPromptContext): string { '**Add new encrypted columns** — declared encrypted from the start; single-deploy.', ), bullet( - '**Migrate existing columns** — schema-add → dual-write code → backfill → schema rename → cutover → read-path switch → remove dual-write code → drop plaintext. No deploy gate between rollout and cutover steps because there is no deployed application to gate on.', + `**Migrate existing columns** — schema-add → dual-write code → backfill → switch reads to the encrypted column → remove dual-write code → drop plaintext. The switch step depends on the column's EQL version (\`${cli} encrypt status\` shows it): on **EQL v3**, the default, there is no rename — point the schema and queries at \`_encrypted\` by name; on **EQL v2** only, \`${cli} encrypt cutover\` renames the twin into \`\` first. No deploy gate between rollout and cutover steps because there is no deployed application to gate on.`, ), '', '## Your task: produce the complete-rollout plan file', diff --git a/packages/stack/README.md b/packages/stack/README.md index ae45f1ea6..9e5710eda 100644 --- a/packages/stack/README.md +++ b/packages/stack/README.md @@ -528,8 +528,10 @@ same claim must be supplied to encrypt and decrypt. Lock contexts work with all operations: `encrypt`, `decrypt`, `encryptModel`, `decryptModel`, `bulkEncryptModels`, `bulkDecryptModels`, `bulkEncrypt`, `bulkDecrypt`, `encryptQuery`. `.withLockContext()` also accepts a `LockContext` instance. -On the typed client, `decryptModel` / `bulkDecryptModels` take the lock -context as an optional third argument instead of chaining. +On the typed client, `decryptModel` / `bulkDecryptModels` additionally accept +the lock context as an optional third argument. Use that or `.withLockContext()`, +not both — chaining onto a decrypt that already took a positional lock context +throws. > **Deprecated: `LockContext.identify()`.** Per-operation CTS tokens were removed > in `protect-ffi` 0.25; the token `identify()` fetches is no longer used by @@ -707,14 +709,14 @@ Method signatures are derived from your schemas: plaintext arguments are pinned `returnType` controls the encrypted query term's shape: `'eql'` (default, the EQL JSON payload for the ORM adapters), `'composite-literal'` (a Postgres composite string for `.eq()`/string-based APIs), or `'escaped-composite-literal'` (the same, escaped for embedding). Most users take the default; the adapters set it as needed. | `encryptQuery` | `(terms: ScalarQueryTerm[])` | `BatchEncryptQueryOperation` (thenable) | | `encryptModel` | `(model, table)` | `EncryptModelOperation` (thenable) | -| `decryptModel` | `(encryptedModel, table, lockContext?)` | `Promise>` | +| `decryptModel` | `(encryptedModel, table, lockContext?)` | `AuditableDecryptModelOperation` (thenable) | | `bulkEncryptModels` | `(models, table)` | `BulkEncryptModelsOperation` (thenable) | -| `bulkDecryptModels` | `(encryptedModels, table, lockContext?)` | `Promise>` | +| `bulkDecryptModels` | `(encryptedModels, table, lockContext?)` | `AuditableDecryptModelOperation` (thenable) | | `bulkEncrypt` | `(plaintexts, { column, table })` | `BulkEncryptOperation` (thenable) | | `bulkDecrypt` | `(encryptedPayloads)` | `BulkDecryptOperation` (thenable) | | `getEncryptConfig` | `()` | The resolved encrypt config | -The thenable operations support `.withLockContext(lockContext)` for identity-aware encryption. `decryptModel` / `bulkDecryptModels` return a plain `Promise` instead — pass the lock context as the optional third argument. `decrypt` of a single value cannot be strongly typed (a lone ciphertext carries no column identity), and `encryptQuery` rejects storage-only columns at compile time. +The thenable operations support `.withLockContext(lockContext)` for identity-aware encryption, and `decryptModel` / `bulkDecryptModels` also support `.audit({ metadata })`. Those two additionally accept the lock context as an optional third argument — use one form or the other. `decrypt` of a single value cannot be strongly typed (a lone ciphertext carries no column identity), and `encryptQuery` rejects storage-only columns at compile time. ### `LockContext` (legacy) diff --git a/packages/stack/src/dynamodb/types.ts b/packages/stack/src/dynamodb/types.ts index 83aac0414..c84af7b2c 100644 --- a/packages/stack/src/dynamodb/types.ts +++ b/packages/stack/src/dynamodb/types.ts @@ -21,9 +21,14 @@ import type { EncryptModelOperation } from './operations/encrypt-model' * `encryptedColumn`/`encryptedField` from `@cipherstash/stack/schema`) or an * EQL v3 one (`encryptedTable` + `types.*` from `@cipherstash/stack/eql/v3`). * - * Both are supported deliberately. DynamoDB shares none of the v2 Postgres - * machinery — there is no EQL extension to install and no migration to run — - * so accepting v3 is purely additive and no existing caller has to change. + * This union is the adapter's widest input type — the erased view the internal + * `CallableEncryptionClient` is declared against. It is NOT the public contract: + * the surface split the two versions apart. `encryptModel` / + * `bulkEncryptModels` narrowed to `AnyV3Table` (the v2 write overloads were + * removed, so a v2 encrypt call site does have to change); `decryptModel` / + * `bulkDecryptModels` still take either, so items stored under v2 stay + * readable. See {@link EncryptedDynamoDBInstance} for the overloads that decide + * this per method. */ export type AnyEncryptedTable = | EncryptedTable diff --git a/scripts/__tests__/fixtures/lint-no-dead-package-paths/dead-shell.md b/scripts/__tests__/fixtures/lint-no-dead-package-paths/dead-shell.md new file mode 100644 index 000000000..9b25c6550 --- /dev/null +++ b/scripts/__tests__/fixtures/lint-no-dead-package-paths/dead-shell.md @@ -0,0 +1,4 @@ +# A deleted package whose build output survives on disk + +The old thing lived in packages/lint-dead-shell-probe, deleted from git but +still sitting there as a `dist/` shell on any checkout that once built it. diff --git a/scripts/__tests__/fixtures/lint-no-dead-package-paths/sentence-final.md b/scripts/__tests__/fixtures/lint-no-dead-package-paths/sentence-final.md new file mode 100644 index 000000000..7ae81cfb4 --- /dev/null +++ b/scripts/__tests__/fixtures/lint-no-dead-package-paths/sentence-final.md @@ -0,0 +1,7 @@ +# Sentence-final package paths + +The client lives in packages/stack. +A hyphen at a wrap: packages/migrate- +An underscore: packages/wizard_ +An ellipsis: packages/cli... +Parenthesised (packages/stack) and comma'd packages/cli, both fine. diff --git a/scripts/__tests__/fixtures/lint-no-dead-package-paths/untracked-package.md b/scripts/__tests__/fixtures/lint-no-dead-package-paths/untracked-package.md new file mode 100644 index 000000000..4f808752e --- /dev/null +++ b/scripts/__tests__/fixtures/lint-no-dead-package-paths/untracked-package.md @@ -0,0 +1,3 @@ +# A package that exists on disk but is not yet tracked + +The new thing lives in packages/lint-untracked-probe, scaffolded but not staged. diff --git a/scripts/__tests__/fixtures/lint-no-dead-package-paths/uppercase.md b/scripts/__tests__/fixtures/lint-no-dead-package-paths/uppercase.md new file mode 100644 index 000000000..a2060a9c9 --- /dev/null +++ b/scripts/__tests__/fixtures/lint-no-dead-package-paths/uppercase.md @@ -0,0 +1,3 @@ +# Uppercase package names must still be checked + +See packages/Foo/does/not/exist for details. diff --git a/scripts/__tests__/lint-no-dead-package-paths.test.mjs b/scripts/__tests__/lint-no-dead-package-paths.test.mjs index bc9ea927f..92f9d7aff 100644 --- a/scripts/__tests__/lint-no-dead-package-paths.test.mjs +++ b/scripts/__tests__/lint-no-dead-package-paths.test.mjs @@ -1,5 +1,7 @@ import { execFileSync } from 'node:child_process' -import { resolve } from 'node:path' +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join, resolve } from 'node:path' import { fileURLToPath } from 'node:url' import { describe, expect, it } from 'vitest' @@ -8,9 +10,18 @@ const SCRIPT = resolve( '../../lint-no-dead-package-paths.mjs', ) +const REPO_ROOT = resolve(fileURLToPath(import.meta.url), '../../..') + function run(...targets) { + return runWith({}, ...targets) +} + +function runWith(opts, ...targets) { try { - execFileSync('node', [SCRIPT, ...targets], { encoding: 'utf8' }) + execFileSync(process.execPath, [SCRIPT, ...targets], { + encoding: 'utf8', + ...opts, + }) return { exitCode: 0, output: '' } } catch (err) { return { @@ -43,6 +54,159 @@ describe('lint-no-dead-package-paths', () => { expect(r.output).toMatch(/packages\/protect/) }) + // #772 review, finding 15. The name capture had no right anchor, so a + // sentence-final `packages/stack.` swallowed the period and the linter + // reported a LIVE package as dead — failing the build with a message naming a + // directory that plainly exists. Never fired in 400 commits only because the + // repo's backtick convention happened to dodge it. + it('does not flag a live package followed by sentence punctuation', () => { + const r = run(fx('sentence-final.md')) + expect(r.output).toBe('') + expect(r.exitCode).toBe(0) + }) + + // The character class excluded uppercase, so `packages/Foo` was never checked + // at all — a silent hole rather than a false alarm. + it('checks a package name containing uppercase', () => { + const r = run(fx('uppercase.md')) + expect(r.exitCode).toBe(1) + expect(r.output).toMatch(/packages\/Foo/) + }) + + // The linters carry package paths of their own; `scripts/` was not scanned, + // so lint-no-hardcoded-runners' `packages/drizzle/src/bin/runner.ts` allowlist + // entry — added speculatively by c6715608, load-bearing 31 minutes later once + // 9d259e6e created the file — sat dead for the two months after 413ca396 + // deleted the package. Its sibling entry for `packages/protect` was removed + // with its package; this one was simply missed. + // + // Asserting the repo is clean would NOT pin this: the suite's first test + // already does that, and both pass whether or not `scripts` is in TARGETS. + // Plant an offender in the scanned directory instead. + it('scans scripts/ but not its fixtures', () => { + const probe = resolve(REPO_ROOT, 'scripts/dead-path-probe.md') + try { + writeFileSync(probe, 'A reference to `packages/protect`, long gone.\n') + const r = run() + expect(r.exitCode).toBe(1) + expect(r.output).toMatch(/scripts\/dead-path-probe\.md:1/) + // `__tests__` stays skipped — the fixtures name dead packages on purpose, + // and so do the comments in this very file. + expect(r.output).not.toMatch(/__tests__/) + } finally { + rmSync(probe, { force: true }) + } + }) + + // A package scaffolded a minute ago is live, but nothing about it is tracked + // yet. Deriving the live set from `git ls-files` alone reported it as "does + // not exist" — the same species of false alarm as the sentence-final one + // above, pointed the other way, at a directory sitting right there on disk. + it('treats a package that exists but is not yet tracked as live', () => { + const pkg = resolve(REPO_ROOT, 'packages/lint-untracked-probe') + try { + mkdirSync(resolve(pkg, 'src'), { recursive: true }) + writeFileSync(resolve(pkg, 'src/index.ts'), 'export const x = 1\n') + const r = run(fx('untracked-package.md')) + expect(r.output).toBe('') + expect(r.exitCode).toBe(0) + } finally { + rmSync(pkg, { recursive: true, force: true }) + } + }) + + // The whole reason `livePackages` comes from git rather than from + // `readdirSync`, pinned in the discriminating direction (#772 review, + // finding 15). + // + // Deleting a package leaves its gitignored `dist/` and `node_modules/` + // behind, so the directory is still on disk and any filesystem-derived live + // set calls it live — silently excusing every reference to it. The test + // above, `treats a package that exists but is not yet tracked as live`, does + // NOT pin this: it builds a divergence that `readdirSync` and git agree on, + // so it passes under a revert. This one is the only test that fails under + // both a plain `readdirSync` revert and the more plausible + // `readdirSync`-unioned-with-git hybrid. + // + // The shell's contents MUST be gitignored. If they aren't, the + // `--others --exclude-standard` arm legitimately counts the package as live + // and this test would assert the opposite of what it claims — so guard on a + // clean `git status` and fail loudly rather than silently degrading. + it('flags a deleted package whose gitignored dist/ shell survives on disk', () => { + const shell = resolve(REPO_ROOT, 'packages/lint-dead-shell-probe') + try { + mkdirSync(resolve(shell, 'dist'), { recursive: true }) + writeFileSync(resolve(shell, 'dist/index.js'), 'exports.x = 1\n') + mkdirSync(resolve(shell, 'node_modules'), { recursive: true }) + writeFileSync(resolve(shell, 'node_modules/.keep'), '') + + // Scoped to the probe alone: asserting the whole `packages/` tree is + // clean would couple this to whatever else is uncommitted in the + // working copy, which is nobody's business but the developer's. + const visible = execFileSync( + 'git', + ['status', '--porcelain', 'packages/lint-dead-shell-probe'], + { cwd: REPO_ROOT, encoding: 'utf8' }, + ) + expect(visible).toBe('') + + const r = run(fx('dead-shell.md')) + expect(r.exitCode).toBe(1) + expect(r.output).toMatch(/packages\/lint-dead-shell-probe/) + } finally { + rmSync(shell, { recursive: true, force: true }) + } + }) + + // The live set is derived by shelling out to git, so git failing is a mode + // this linter has to own. Exiting 1 with a raw ENOENT stack trace would be + // indistinguishable from a genuine lint failure; exit 2 says "the linter + // could not run", not "your docs are wrong". + it('exits 2 with an actionable message when git is unavailable', () => { + const r = runWith({ + env: { PATH: resolve(REPO_ROOT, 'scripts/nonexistent') }, + }) + expect(r.exitCode).toBe(2) + expect(r.output).toMatch(/git/) + expect(r.output).toMatch(/checkout/) + expect(r.output).not.toMatch(/at ModuleJob/) + }) + + // A linter whose whole job is catching dead paths in configuration silently + // ignored dead paths in its OWN configuration: a target that did not exist + // was skipped, so a renamed or moved entry dropped out of coverage forever + // with a green build. Its sibling `lint-no-hardcoded-runners.mjs` exits 2 on + // a stale allowlist entry — same rot, opposite handling, same PR. + // + // Exit 2, not 1: the linter could not check what it was asked to check, + // which is not the same as "your docs are wrong". + it('exits 2 when a target does not exist rather than skipping it', () => { + const r = run(fx('no-such-fixture.md')) + expect(r.exitCode).toBe(2) + expect(r.output).toMatch(/no-such-fixture\.md/) + expect(r.output).not.toMatch(/at ModuleJob/) + }) + + // `relative(REPO_ROOT, file)` renders a target outside the repo as a + // `../../../../..` chain climbing out of the root — technically resolvable, + // unreadable in practice, and it buries the one thing the line is for. + // Unreachable via the default target list, which is repo-relative + // throughout; reachable the moment anyone passes an argv override. + it('renders an absolute path for an offender outside the repo root', () => { + const dir = mkdtempSync(join(tmpdir(), 'lint-dead-package-paths-')) + try { + const file = join(dir, 'outside.md') + writeFileSync(file, 'Points at packages/lint-dead-shell-probe, gone.\n') + const r = run(file) + expect(r.exitCode).toBe(1) + expect(r.output).toMatch(/packages\/lint-dead-shell-probe/) + expect(r.output).toContain(file) + expect(r.output).not.toMatch(/\.\.\//) + } finally { + rmSync(dir, { recursive: true, force: true }) + } + }) + it('names the file and line of each offender', () => { const r = run(fx('dead-ref.md')) expect(r.output).toMatch(/dead-ref\.md:3/) diff --git a/scripts/__tests__/lint-no-hardcoded-runners.test.mjs b/scripts/__tests__/lint-no-hardcoded-runners.test.mjs index 7bb1202dc..709ac63ed 100644 --- a/scripts/__tests__/lint-no-hardcoded-runners.test.mjs +++ b/scripts/__tests__/lint-no-hardcoded-runners.test.mjs @@ -1,5 +1,7 @@ import { execFileSync } from 'node:child_process' -import { resolve } from 'node:path' +import { mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join, resolve } from 'node:path' import { fileURLToPath } from 'node:url' import { describe, expect, it } from 'vitest' @@ -8,9 +10,9 @@ const SCRIPT = resolve( '../../lint-no-hardcoded-runners.mjs', ) -function run(target) { +function runScript(script, ...targets) { try { - execFileSync('node', [SCRIPT, target], { encoding: 'utf8' }) + execFileSync(process.execPath, [script, ...targets], { encoding: 'utf8' }) return { exitCode: 0, output: '' } } catch (err) { return { @@ -20,6 +22,10 @@ function run(target) { } } +function run(target) { + return runScript(SCRIPT, target) +} + describe('lint-no-hardcoded-runners', () => { const fx = (name) => resolve(fileURLToPath(import.meta.url), `../fixtures/${name}`) @@ -35,6 +41,32 @@ describe('lint-no-hardcoded-runners', () => { expect(r.output).toMatch(/\bnpx\b/) }) + // `statSync` on a missing target threw uncaught: exit 1 plus a raw stack + // trace, indistinguishable from "found a hardcoded npx" to anything reading + // the exit code. Exit 2 means the linter could not run. + it('exits 2 when a target does not exist', () => { + const r = run(fx('no-such-fixture.ts')) + expect(r.exitCode).toBe(2) + expect(r.output).toContain('no-such-fixture.ts') + expect(r.output).not.toMatch(/at ModuleJob/) + }) + + // Matches the sibling linter: a target outside the repo rendered as a + // `../../../../..` chain out of the root instead of naming the file. + it('renders an absolute path for an offender outside the repo root', () => { + const dir = mkdtempSync(join(tmpdir(), 'lint-hardcoded-runners-')) + try { + const file = join(dir, 'outside.ts') + writeFileSync(file, "export const cmd = 'npx drizzle-kit generate'\n") + const r = run(file) + expect(r.exitCode).toBe(1) + expect(r.output).toContain(file) + expect(r.output).not.toMatch(/\.\.\//) + } finally { + rmSync(dir, { recursive: true, force: true }) + } + }) + it("ignores `?? 'npx'` fallback expressions", () => { expect(run(fx('allowed-fallback.ts')).exitCode).toBe(0) }) @@ -70,3 +102,51 @@ describe('lint-no-hardcoded-runners', () => { expect(run(fx('identifier.ts')).exitCode).toBe(0) }) }) + +// An allowlist entry is a standing exemption. When the file it names is deleted +// — or stops carrying the `npx` literal it was excused for — the entry becomes +// silent dead weight, and the next reader takes it as evidence that the file +// still needs an exemption. `packages/drizzle/src/bin/runner.ts` sat here for +// the two months after 413ca396 deleted its package, and surfaced only because +// a sibling linter happened to start scanning `scripts/` (#772 review, finding +// 15). That sibling can only ever catch the `packages/` shape; this check +// covers every entry, including a stale path inside a live package. +describe('lint-no-hardcoded-runners — allowlist hygiene', () => { + // A copy alongside the original so `REPO_ROOT` still resolves to the repo. + const PROBE = resolve( + fileURLToPath(import.meta.url), + '../../allowlist-probe.mjs', + ) + + function runWithExtraEntry(entry) { + const src = readFileSync(SCRIPT, 'utf8').replace( + 'const ALLOWLISTED_PATHS = new Set([', + `const ALLOWLISTED_PATHS = new Set([\n '${entry}',`, + ) + try { + writeFileSync(PROBE, src) + return runScript(PROBE) + } finally { + rmSync(PROBE, { force: true }) + } + } + + it('rejects an entry whose file no longer exists', () => { + const r = runWithExtraEntry('scripts/deleted-helper.mjs') + expect(r.exitCode).toBe(2) + expect(r.output).toMatch(/scripts\/deleted-helper\.mjs/) + expect(r.output).toMatch(/no such file/) + }) + + it('rejects an entry whose file no longer needs the exemption', () => { + const r = runWithExtraEntry('scripts/vitest.config.mjs') + expect(r.exitCode).toBe(2) + expect(r.output).toMatch(/scripts\/vitest\.config\.mjs/) + expect(r.output).toMatch(/no longer contains/) + }) + + it('accepts the allowlist as it stands', () => { + const r = runScript(SCRIPT) + expect(r.exitCode).toBe(0) + }) +}) diff --git a/scripts/lint-no-dead-package-paths.mjs b/scripts/lint-no-dead-package-paths.mjs index 8ca468d27..321b30e04 100644 --- a/scripts/lint-no-dead-package-paths.mjs +++ b/scripts/lint-no-dead-package-paths.mjs @@ -1,3 +1,4 @@ +import { execFileSync } from 'node:child_process' import { readdirSync, readFileSync, statSync } from 'node:fs' import { join, relative, resolve } from 'node:path' @@ -24,6 +25,10 @@ const TARGETS = process.argv.slice(2).length 'skills', 'e2e/README.md', 'packages/cli/AGENTS.md', + // The linters themselves carry package paths — an allowlist entry for a + // deleted package sat here unnoticed because `scripts/` was not scanned. + // `__tests__` is excluded below: its fixtures MUST name dead packages. + 'scripts', ] const SKIP_DIRS = new Set([ @@ -32,22 +37,96 @@ const SKIP_DIRS = new Set([ 'plans', 'superpowers', '.git', + // This linter's own self-tests deliberately reference deleted packages — + // in the fixtures, and in the prose comments of the test files themselves. + // Scanning them would make the suite unrunnable. + '__tests__', ]) const SKIP_FILES = new Set(['CHANGELOG.md']) const TEXT_EXT = /\.(md|ya?ml|json|mjs|ts|txt)$/ // `packages/` where `` is a real directory name. The character // class excludes `*`, so workspace globs (`packages/*`, `./packages/*`) are -// left alone, and `+` is greedy so `packages/stack-forge` is never excused by -// the live `packages/stack`. -const PACKAGE_REF = /packages\/([a-z0-9][a-z0-9._-]*)/g +// left alone, and it is greedy so a longer directory name is never excused by +// a live package whose name is a prefix of it. +// +// The name must END on an alphanumeric. Without that anchor a sentence-final +// `packages/stack.` — or a hyphen at a line wrap, or a trailing underscore — +// captured the punctuation too and reported a LIVE package as dead, failing +// the build with a message naming a directory that plainly exists. Uppercase +// is admitted so a capitalised directory name is checked rather than silently +// skipped; no package uses one today, which is exactly why nothing noticed +// (#772 review, finding 15). +const PACKAGE_REF = /packages\/([a-zA-Z0-9](?:[a-zA-Z0-9._-]*[a-zA-Z0-9])?)/g + +// Live packages come from git, not from what is on disk. +// +// `readdirSync` was wrong in the direction that matters: deleting a package +// leaves its `dist/` and `node_modules/` behind, so the directory still exists +// and every reference to the deleted package passed. That is the exact failure +// this linter was written to catch, and it silently stopped catching it on any +// checkout where the package had previously been built — two packages deleted +// by this very stack are sitting on `main` right now as exactly such shells +// (#772 review, finding 15). +// +// Note this deliberately does NOT require a `package.json`: `packages/utils` has +// none (it is two loose files consumed by relative path from `packages/nextjs`) +// yet is tracked, live, and referenced from AGENTS.md. +// +// Shelling out to git is a dependency this linter has to own: git missing, or a +// tree with no `.git`, must not read as "every package is dead". +function gitPackagePaths(...args) { + try { + return execFileSync('git', args, { + cwd: REPO_ROOT, + encoding: 'utf8', + maxBuffer: 64 * 1024 * 1024, + }) + .split('\0') + .filter(Boolean) + } catch (err) { + const detail = String(err.stderr || err.message || '').trim() + console.error( + `Could not list packages via \`git ${args.join(' ')}\`:\n\n ${detail}\n\n` + + 'This linter derives the live package set from git, so it cannot run\n' + + 'without git on PATH or outside a git checkout.', + ) + // Exit 2, not 1: the linter failed to run. Exit 1 means it ran and found + // dead references, which is a different thing to go and fix. + process.exit(2) + } +} const livePackages = new Set( - readdirSync(resolve(REPO_ROOT, 'packages'), { withFileTypes: true }) - .filter((e) => e.isDirectory()) - .map((e) => e.name), + [ + ...gitPackagePaths('ls-files', '-z', 'packages'), + // Untracked but not ignored. A package scaffolded a minute ago is live + // even though nothing about it is staged yet, and reporting it as "does + // not exist" is the sentence-final false alarm all over again, pointed the + // other way. `--directory` is deliberately NOT passed: it collapses an + // all-ignored directory to a single entry, which would resurrect exactly + // the `dist/`-and-`node_modules/` shells this linter exists to catch. + ...gitPackagePaths( + 'ls-files', + '--others', + '--exclude-standard', + '-z', + 'packages', + ), + ] + .map((file) => file.split('/')[1]) + .filter(Boolean), ) +if (livePackages.size === 0) { + console.error( + 'git reported no packages at all under `packages/`. Refusing to run —\n' + + 'every reference would be flagged. Check that `packages/` is present and\n' + + 'not wholly ignored.', + ) + process.exit(2) +} + function* walk(abs) { const stat = statSync(abs) if (stat.isFile()) { @@ -73,19 +152,39 @@ for (const target of TARGETS) { } catch { exists = false } - // A default target that doesn't exist is not an error — the list covers - // optional files (CONTRIBUTE.md, per-package AGENTS.md). - if (!exists) continue + // A target that isn't there used to be skipped in silence, on the theory + // that the default list covered optional files. It doesn't — all of TARGETS + // is tracked and present — and the silence was the same rot this linter + // exists to catch, one level up: rename a target and it drops out of + // coverage forever with a green build. `lint-no-hardcoded-runners.mjs` + // already exits 2 on a stale allowlist entry; this is that rule applied to + // this linter's own configuration. + if (!exists) { + console.error( + `Target \`${target}\` does not exist.\n\n` + + 'Either it was renamed or removed — in which case update TARGETS in\n' + + 'this script, since a target that is silently skipped is coverage\n' + + 'quietly lost — or it was mistyped on the command line.', + ) + // Exit 2, not 1: the linter could not check what it was asked to check. + process.exit(2) + } for (const file of walk(abs)) { + // A target outside the repo (only reachable via an argv override — every + // default is repo-relative) renders as a `../../../../..` chain climbing + // out of the root, which buries the filename it exists to point at. const rel = relative(REPO_ROOT, file) + const shown = rel.startsWith('..') ? file : rel if (SKIP_FILES.has(rel.split('/').pop())) continue const lines = readFileSync(file, 'utf8').split('\n') lines.forEach((line, idx) => { PACKAGE_REF.lastIndex = 0 for (const m of line.matchAll(PACKAGE_REF)) { if (livePackages.has(m[1])) continue - offenders.push(`${rel}:${idx + 1}: \`packages/${m[1]}\` does not exist`) + offenders.push( + `${shown}:${idx + 1}: \`packages/${m[1]}\` does not exist`, + ) } }) } diff --git a/scripts/lint-no-hardcoded-runners.mjs b/scripts/lint-no-hardcoded-runners.mjs index e8bbc8b49..3070963c8 100644 --- a/scripts/lint-no-hardcoded-runners.mjs +++ b/scripts/lint-no-hardcoded-runners.mjs @@ -9,8 +9,6 @@ const REPO_ROOT = resolve(import.meta.dirname, '..') const ALLOWLISTED_PATHS = new Set([ 'packages/wizard/src/lib/detect.ts', // npm row of the PM table 'packages/cli/src/commands/init/utils.ts', // runnerCommand `case 'npm'` - 'packages/cli/src/commands/init/lib/setup-prompt.ts', // execCommand `case 'npm':` switch - 'packages/drizzle/src/bin/runner.ts', // Pre-allowlisted: helper for Task 13 'scripts/lint-no-hardcoded-runners.mjs', // this script's own docs ]) @@ -69,13 +67,77 @@ function isAllowedRunnerSwitch(line) { return /\bcase\s+['"]npm['"]/.test(line) || /name:\s*['"]npm['"]/.test(line) } +// An allowlist entry is a standing exemption, so it has to keep earning its +// place. Two ways it rots: the file is deleted (which is how the legacy Drizzle +// package's `src/bin/runner.ts` entry outlived its package by two months), or +// the file survives but the `npx` literal it was excused for moves elsewhere — +// leaving an entry that reads as evidence the exemption is still needed. Check +// both before scanning anything. +const staleAllowlist = [] +for (const rel of ALLOWLISTED_PATHS) { + let source + try { + source = readFileSync(resolve(REPO_ROOT, rel), 'utf8') + } catch { + staleAllowlist.push(`${rel}: no such file`) + continue + } + const stillNeeded = source + .split('\n') + .some( + (line) => + NPX_TOKEN.test(line) && + !isCommentLine(line) && + !isAllowedFallback(line) && + !isAllowedRunnerSwitch(line), + ) + if (!stillNeeded) { + staleAllowlist.push( + `${rel}: no longer contains an unexcused \`npx\` literal`, + ) + } +} + +if (staleAllowlist.length > 0) { + console.error( + `Found ${staleAllowlist.length} stale allowlist entr(ies) in this script:\n`, + ) + for (const s of staleAllowlist) console.error(` ${s}`) + console.error( + '\nDrop the entry. An exemption for a file that no longer exists, or that\n' + + 'no longer contains the literal it was excused for, is dead weight that\n' + + 'reads as deliberate.', + ) + // Exit 2, not 1: the linter's own configuration is wrong, which is a + // different thing to fix than an `npx` literal in the codebase. + process.exit(2) +} + const offenders = [] for (const target of TARGETS) { const abs = resolve(REPO_ROOT, target) - const stat = statSync(abs) + let stat + try { + stat = statSync(abs) + } catch { + // Was an uncaught throw: exit 1 plus a raw stack trace, which reads to + // anything checking the exit code exactly like a genuine `npx` finding. + // Exit 2 says the linter could not run — the same contract the allowlist + // self-check above already uses. + console.error( + `Target \`${target}\` does not exist.\n\n` + + 'Update the scan roots in this script if it was renamed or removed,\n' + + 'or check the path passed on the command line.', + ) + process.exit(2) + } const files = stat.isDirectory() ? walk(abs) : [abs] for await (const file of files) { + // `rel` stays repo-relative for the allowlist lookup; only the rendering + // falls back to the absolute path, for a target outside the repo root that + // would otherwise print a `../../../../..` chain instead of a filename. const rel = relative(REPO_ROOT, file) + const shown = rel.startsWith('..') ? file : rel if (ALLOWLISTED_PATHS.has(rel)) continue if (/\.(test|spec)\.(ts|tsx|mts|cts)$/.test(file)) continue const lines = readFileSync(file, 'utf8').split('\n') @@ -85,7 +147,7 @@ for (const target of TARGETS) { if (isCommentLine(line)) return if (isAllowedFallback(line)) return if (isAllowedRunnerSwitch(line)) return - offenders.push(`${rel}:${idx + 1}: ${line.trim()}`) + offenders.push(`${shown}:${idx + 1}: ${line.trim()}`) }) } } diff --git a/skills/stash-encryption/SKILL.md b/skills/stash-encryption/SKILL.md index 010e63d4d..6865ebd2f 100644 --- a/skills/stash-encryption/SKILL.md +++ b/skills/stash-encryption/SKILL.md @@ -390,7 +390,7 @@ if (!enc.failure) { Typed-client model notes: -- `decryptModel` / `bulkDecryptModels` take the **table as a second argument** and return a plain `Promise>` (not a chainable operation) — pass a lock context as the optional third argument instead of chaining `.withLockContext()`. +- `decryptModel` / `bulkDecryptModels` take the **table as a second argument** and return a chainable `AuditableDecryptModelOperation` — await it for the `Result`, or chain `.audit({ metadata })` / `.withLockContext(lockContext)` first. A lock context may instead be passed as the optional third argument; use one form or the other, not both (chaining `.withLockContext()` onto a decrypt that already took a positional lock context throws). - `Date` columns are reconstructed to real `Date` instances on decrypt; `bigint` columns round-trip as native `bigint`. - Nullable schema fields stay nullable through the round trip. @@ -698,7 +698,7 @@ Every operation returns a `Result`. Narrow on `.failure` before touching `.data` `identityClaim` is an array of JWT claim *names*, not values: `["sub"]` (the default) or `["sub", "org_id"]`. ZeroKMS resolves each claim's value from the JWT the strategy federated. **The same claim must be supplied to encrypt and decrypt** — it is baked into the data key's tag, so decrypting without it fails with `Failed to retrieve key`. -Lock contexts work with every operation: `encrypt`, `decrypt`, `encryptModel`, `decryptModel`, `bulkEncrypt`, `bulkDecrypt`, `bulkEncryptModels`, `bulkDecryptModels`, `encryptQuery`. On the typed client, `decryptModel` and `bulkDecryptModels` take the lock context as their optional **third argument** (they return a plain `Promise>`, so there is no `.withLockContext()` to chain): +Lock contexts work with every operation: `encrypt`, `decrypt`, `encryptModel`, `decryptModel`, `bulkEncrypt`, `bulkDecrypt`, `bulkEncryptModels`, `bulkDecryptModels`, `encryptQuery`. On the typed client, `decryptModel` and `bulkDecryptModels` additionally accept the lock context as an optional **third argument**, which is the form shown here — `.withLockContext()` chains on them too, but use one or the other, not both: ```typescript const dec = await client.decryptModel(enc.data, users, IDENTITY) @@ -747,7 +747,7 @@ const result = await client .audit({ metadata: { action: "create" } }) // optional: audit trail ``` -(The typed client's `decryptModel` / `bulkDecryptModels` are the exception: they return a plain `Promise>` and take the lock context as an argument — see "Model Operations".) +(The typed client's `decryptModel` / `bulkDecryptModels` may also take the lock context as a positional argument instead of chaining — see "Model Operations".) ## Error Handling