From df047ef2bb7bd25e0eca5146e725600a2f61881f Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Sat, 25 Jul 2026 14:43:55 +1000 Subject: [PATCH] fix(skills,stack,scripts): correct shipped decrypt guidance, and make the package-path linter sound MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Finding 14 — decryptModel/bulkDecryptModels return an AuditableDecryptModelOperation: thenable, with .withLockContext() and .audit(). Three sites in skills/stash-encryption and four in packages/stack/README.md said the opposite ("a plain Promise>", "no .withLockContext() to chain"), steering agents away from the very .audit() chain the audit-on-decrypt changeset advertises. The skill contradicted its own reference table, which was already right — 8b744309 fixed that table and nothing else, despite a commit message claiming otherwise. Both files ship: the skill inside the stash tarball and thence into customer repos via installSkills(), the README inside the @cipherstash/stack tarball. The equivalent statement about the WASM entry is CORRECT and deliberately untouched — that client really does return a bare promise. Also `protectOps.eq` in the setup prompt stash init writes for coding agents. One occurrence repo-wide, and no such export exists; the real API is createEncryptionOperators(client), conventionally `ops`. Finding 15 — the package-path linter had a false positive and a false negative, both fixed with the fixture-driven self-tests the suite already uses: - The name capture had no right anchor, so a sentence-final `packages/stack.` swallowed the period and reported a LIVE package as dead. Uppercase was also excluded from the class, so a capitalised name was never checked at all. - livePackages came from readdirSync, i.e. the working tree. Deleting a package leaves dist/ and node_modules/ behind, so every reference to it kept passing — the exact case the linter was commissioned to catch, silently unenforced on any checkout that had built the package. Now derived from `git ls-files`. Deliberately not "has a package.json": packages/utils has none and is live. - scripts/ was not scanned, so the linters never checked themselves. Adding it immediately surfaced a dead allowlist entry in lint-no-hardcoded-runners for a path that never existed in git history. Self-test fixtures stay exempt — they must name dead packages. Severity note for the record: CI was never affected (fresh checkout, caching disabled), and main carries no required status checks — this was a local developer papercut plus a real soundness hole, not a broken build. Finally, resolves two changesets that contradicted each other in the same release: dynamodb-eql-v3 claimed "EQL v2 tables continue to work unchanged" and "no existing caller needs to change" while stack-dynamodb-v2-write-removal announced the v2 encrypt overloads as removed. The code sides with removal; the claims are now scoped to the decrypt path. --- .changeset/decrypt-chaining-docs.md | 27 +++++++++++ .changeset/dynamodb-eql-v3.md | 13 +++-- .../cli/src/commands/init/lib/setup-prompt.ts | 2 +- packages/stack/README.md | 12 +++-- .../sentence-final.md | 7 +++ .../lint-no-dead-package-paths/uppercase.md | 3 ++ .../lint-no-dead-package-paths.test.mjs | 28 +++++++++++ scripts/lint-no-dead-package-paths.mjs | 47 ++++++++++++++++--- scripts/lint-no-hardcoded-runners.mjs | 1 - skills/stash-encryption/SKILL.md | 6 +-- 10 files changed, 125 insertions(+), 21 deletions(-) create mode 100644 .changeset/decrypt-chaining-docs.md create mode 100644 scripts/__tests__/fixtures/lint-no-dead-package-paths/sentence-final.md create mode 100644 scripts/__tests__/fixtures/lint-no-dead-package-paths/uppercase.md diff --git a/.changeset/decrypt-chaining-docs.md b/.changeset/decrypt-chaining-docs.md new file mode 100644 index 00000000..f2a40d9f --- /dev/null +++ b/.changeset/decrypt-chaining-docs.md @@ -0,0 +1,27 @@ +--- +'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. +The operators come from `createEncryptionOperators(client)`, conventionally +bound to `ops`. diff --git a/.changeset/dynamodb-eql-v3.md b/.changeset/dynamodb-eql-v3.md index c6303c99..3763c2ee 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: the encrypt overloads for it are +removed in this same release (see the DynamoDB v2 write-removal entry), 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/setup-prompt.ts b/packages/cli/src/commands/init/lib/setup-prompt.ts index dd5ba0de..f7560cc5 100644 --- a/packages/cli/src/commands/init/lib/setup-prompt.ts +++ b/packages/cli/src/commands/init/lib/setup-prompt.ts @@ -280,7 +280,7 @@ export function renderImplementPrompt(ctx: SetupPromptContext): string { "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()`.", `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, query paths use the right operator (`ops.eq`, from `createEncryptionOperators(client)` — see the integration skill).', '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', diff --git a/packages/stack/README.md b/packages/stack/README.md index ae45f1ea..9e5710ed 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/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 00000000..7ae81cfb --- /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/uppercase.md b/scripts/__tests__/fixtures/lint-no-dead-package-paths/uppercase.md new file mode 100644 index 00000000..a2060a9c --- /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 bc9ea927..c0dacca7 100644 --- a/scripts/__tests__/lint-no-dead-package-paths.test.mjs +++ b/scripts/__tests__/lint-no-dead-package-paths.test.mjs @@ -43,6 +43,34 @@ 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 a `packages/drizzle` allowlist entry for a package that never existed in + // git history sat there unnoticed. Its own fixtures must stay exempt. + it('scans scripts/ but not its fixtures', () => { + const r = run() + expect(r.exitCode).toBe(0) + expect(r.output).toBe('') + }) + 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/lint-no-dead-package-paths.mjs b/scripts/lint-no-dead-package-paths.mjs index 8ca468d2..47f7497a 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,20 +37,50 @@ const SKIP_DIRS = new Set([ 'plans', 'superpowers', '.git', + // Fixtures for this linter's own self-tests deliberately reference deleted + // packages; 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 what git TRACKS, 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. const livePackages = new Set( - readdirSync(resolve(REPO_ROOT, 'packages'), { withFileTypes: true }) - .filter((e) => e.isDirectory()) - .map((e) => e.name), + execFileSync('git', ['ls-files', '-z', 'packages'], { + cwd: REPO_ROOT, + encoding: 'utf8', + maxBuffer: 64 * 1024 * 1024, + }) + .split('\0') + .filter(Boolean) + .map((file) => file.split('/')[1]) + .filter(Boolean), ) function* walk(abs) { diff --git a/scripts/lint-no-hardcoded-runners.mjs b/scripts/lint-no-hardcoded-runners.mjs index e8bbc8b4..29c453fe 100644 --- a/scripts/lint-no-hardcoded-runners.mjs +++ b/scripts/lint-no-hardcoded-runners.mjs @@ -10,7 +10,6 @@ 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 ]) diff --git a/skills/stash-encryption/SKILL.md b/skills/stash-encryption/SKILL.md index 3c780c1c..704d0ebf 100644 --- a/skills/stash-encryption/SKILL.md +++ b/skills/stash-encryption/SKILL.md @@ -375,7 +375,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. @@ -683,7 +683,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) @@ -732,7 +732,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