Skip to content

Commit c081ea4

Browse files
committed
fix(cli,scripts): per-integration query guidance, and linters that catch their own rot
Follow-up to the #772 review findings, addressing what the review of that review turned up. `stash init`'s setup prompt named `createEncryptionOperators` unconditionally. That symbol is exported by `@cipherstash/stack-drizzle` alone, so a Supabase or Prisma Next project was sent after a package that is not in its dependency tree — the previous `protectOps.eq` was wrong for everyone, this was wrong for three integrations out of four. Step 5 now branches through `queryOperatorGuidance()`, following the `migrationCommands()` pattern already in the file: `ops.eq` for Drizzle, the `encryptedSupabase` wrapper's own filters for Supabase, the `eql*` column operators for Prisma Next, and `client.encryptQuery(...)` for plain Postgres — which is also pointed at `stash-encryption`, since it is the default integration and installs no integration skill, making "see the integration skill" a dangling pointer. The package-path linter reported an untracked-but-present package as "does not exist" — finding 15's false alarm pointed the other way, at a directory sitting right there on disk. The live set now unions `git ls-files --others --exclude-standard`. `--directory` is deliberately not passed: it collapses an all-ignored directory to one entry and would resurrect exactly the `dist/`-and- `node_modules/` shells the linter exists to catch (verified both ways). git failing now exits 2 with an actionable message instead of a raw ENOENT stack trace and exit 1, which was indistinguishable from a genuine lint failure, and an empty live set refuses to run rather than flagging every reference at once. The `scans scripts/ but not its fixtures` test asserted the repo was clean — byte-for-byte what the suite's first test already asserted, and passing whether or not `scripts` was in TARGETS. It now plants an offender in the scanned directory. Verified by mutation: it dies when `scripts` is dropped, as the untracked-package test dies without the union and the git-failure test dies when the exit code is flipped. The runners linter now requires each allowlist entry to exist and to still contain an unexcused `npx` literal. It immediately found a second stale entry the sibling linter structurally cannot see, since it only matches the `packages/<name>` shape: `setup-prompt.ts` has had no `npx` since its switch moved to `utils.ts`. Removed. Also corrects two claims: the test comment saying `packages/drizzle` never existed in git history (it was added speculatively by c671560, became load-bearing 31 minutes later with 9d259e6, and rotted when 413ca39 deleted the package), and the `AnyEncryptedTable` doc comment still promising v3 is "purely additive and no existing caller has to change" — true of decrypt, false of encrypt since the v2 write overloads were removed.
1 parent 22b1451 commit c081ea4

10 files changed

Lines changed: 304 additions & 30 deletions

File tree

.changeset/decrypt-chaining-docs.md

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,5 +23,9 @@ with no lock-context argument.
2323

2424
Also fixes the setup prompt `stash init` writes for coding agents, which
2525
referenced `protectOps.eq` — an API that does not exist anywhere in the repo.
26-
The operators come from `createEncryptionOperators(client)`, conventionally
27-
bound to `ops`.
26+
The step now names the query API each integration can actually import:
27+
`createEncryptionOperators(client)` (conventionally `ops`) for Drizzle, the
28+
`encryptedSupabase` wrapper's own filters for Supabase, the `eql*` column
29+
operators for Prisma Next, and `client.encryptQuery(...)` for a plain Postgres
30+
project — which is also pointed at `stash-encryption`, since it is installed
31+
with no integration skill.

.changeset/dynamodb-eql-v3.md

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -13,11 +13,11 @@ the typed client from `EncryptionV3` and the nominal client from
1313

1414
EQL v2 tables continue to be **readable**`decryptModel` /
1515
`bulkDecryptModels` still accept one, so existing items stay accessible. Writing
16-
through a v2 table is a separate matter: the encrypt overloads for it are
17-
removed in this same release (see the DynamoDB v2 write-removal entry), so a
18-
caller that still encrypts through a v2 table does need to change. The table
19-
decides which wire format is used, so a DynamoDB table populated under one
20-
version must keep being read with that version.
16+
through a v2 table is a separate matter: `encryptModel` / `bulkEncryptModels`
17+
narrowed to EQL v3 tables in this same release, so a caller that still encrypts
18+
through a v2 table does need to change. The table decides which wire format is
19+
used, so a DynamoDB table populated under one version must keep being read with
20+
that version.
2121

2222
This fixes a latent bug that made v3 unusable: the write path detected an
2323
encrypted value by its `k: 'ct'` tag, but EQL v3 scalars carry no `k`

packages/cli/src/commands/init/lib/__tests__/setup-prompt.test.ts

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,45 @@ describe('renderSetupPrompt — orient + route (implement mode)', () => {
105105
expect(out).toContain('pnpm exec drizzle-kit migrate')
106106
})
107107

108+
// The "wire the column through" step names the query API by hand. It used to
109+
// name `protectOps.eq`, which exists nowhere; naming `createEncryptionOperators`
110+
// unconditionally is the same mistake one step smaller — that symbol is
111+
// exported only by `@cipherstash/stack-drizzle`, which a Supabase or Prisma
112+
// project does not even have in its dependency tree. Each integration gets
113+
// the API it can actually import.
114+
describe('query-operator guidance is per-integration', () => {
115+
const render = (integration: SetupPromptContext['integration']) =>
116+
renderSetupPrompt({ ...baseCtx, integration })
117+
118+
it('names the Drizzle operators for drizzle', () => {
119+
const out = render('drizzle')
120+
expect(out).toContain('createEncryptionOperators(client)')
121+
expect(out).toContain('ops.eq')
122+
})
123+
124+
it('names the wrapper builder for supabase, not the Drizzle operators', () => {
125+
const out = render('supabase')
126+
expect(out).not.toContain('createEncryptionOperators')
127+
expect(out).toContain('encryptedSupabase')
128+
})
129+
130+
it('names the eql* column operators for prisma-next', () => {
131+
const out = render('prisma-next')
132+
expect(out).not.toContain('createEncryptionOperators')
133+
expect(out).toContain('eqlEq')
134+
})
135+
136+
// `postgresql` is the default when nothing is detected, and it gets no
137+
// integration skill at all (install-skills.ts) — so "see the integration
138+
// skill" is a dangling pointer for it too.
139+
it('names the core encryptQuery path for plain postgresql', () => {
140+
const out = render('postgresql')
141+
expect(out).not.toContain('createEncryptionOperators')
142+
expect(out).toContain('encryptQuery')
143+
expect(out).toContain('stash-encryption')
144+
})
145+
})
146+
108147
it('emits supabase migration commands for supabase integration', () => {
109148
const out = renderSetupPrompt({
110149
...baseCtx,

packages/cli/src/commands/init/lib/setup-prompt.ts

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,30 @@ function migrationCommands(
6868
return undefined
6969
}
7070

71+
/**
72+
* How this integration filters on an encrypted column. Named per-integration
73+
* rather than generically because the APIs are not interchangeable and only one
74+
* of them is importable from any given project: `createEncryptionOperators` is
75+
* exported by `@cipherstash/stack-drizzle` alone, so naming it for a Supabase
76+
* or Prisma project sends the agent after a package that is not installed.
77+
*
78+
* `postgresql` is the fallback integration and is installed with no integration
79+
* skill (see `SKILL_MAP` in `install-skills.ts`), so it is pointed at
80+
* `stash-encryption` instead of "the integration skill".
81+
*/
82+
function queryOperatorGuidance(integration: Integration): string {
83+
if (integration === 'supabase') {
84+
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'
85+
}
86+
if (integration === 'prisma-next') {
87+
return 'query paths use the `eql*` operators on the column inside `.where()` (`u.email.eqlEq(value)`, `eqlMatch`, `eqlGt`, …) — see the integration skill'
88+
}
89+
if (integration === 'postgresql') {
90+
return 'query paths encrypt the search term first with `client.encryptQuery(value, { table, column, queryType })` and compare against the encrypted column — see the `stash-encryption` skill (a plain Postgres project gets no integration skill)'
91+
}
92+
return 'query paths use the right operator (`ops.eq`, from `createEncryptionOperators(client)`) — see the integration skill'
93+
}
94+
7195
function bullet(line: string): string {
7296
return `- ${line}`
7397
}
@@ -280,7 +304,7 @@ export function renderImplementPrompt(ctx: SetupPromptContext): string {
280304
"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()`.",
281305
`3. Generate the schema migration${migration ? ` — \`${migration.generate}\` (${migration.tool})` : " using the project's existing migration tooling"}.`,
282306
`4. Show the user the generated SQL before applying${migration ? ` — \`${migration.apply}\`` : ''}.`,
283-
'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).',
307+
`5. Wire the column through the application code: insert paths encrypt before write, select paths decrypt after read, ${queryOperatorGuidance(ctx.integration)}.`,
284308
'6. Verify with a round-trip: insert a record, select it back, confirm the value decrypts and the search ops work.',
285309
'',
286310
'### Migrate an existing column to encrypted',

packages/stack/src/dynamodb/types.ts

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,9 +21,14 @@ import type { EncryptModelOperation } from './operations/encrypt-model'
2121
* `encryptedColumn`/`encryptedField` from `@cipherstash/stack/schema`) or an
2222
* EQL v3 one (`encryptedTable` + `types.*` from `@cipherstash/stack/eql/v3`).
2323
*
24-
* Both are supported deliberately. DynamoDB shares none of the v2 Postgres
25-
* machinery — there is no EQL extension to install and no migration to run —
26-
* so accepting v3 is purely additive and no existing caller has to change.
24+
* This union is the adapter's widest input type — the erased view the internal
25+
* `CallableEncryptionClient` is declared against. It is NOT the public contract:
26+
* the surface split the two versions apart. `encryptModel` /
27+
* `bulkEncryptModels` narrowed to `AnyV3Table` (the v2 write overloads were
28+
* removed, so a v2 encrypt call site does have to change); `decryptModel` /
29+
* `bulkDecryptModels` still take either, so items stored under v2 stay
30+
* readable. See {@link EncryptedDynamoDBInstance} for the overloads that decide
31+
* this per method.
2732
*/
2833
export type AnyEncryptedTable =
2934
| EncryptedTable<EncryptedTableColumn>
Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
# A package that exists on disk but is not yet tracked
2+
3+
The new thing lives in packages/lint-untracked-probe, scaffolded but not staged.

scripts/__tests__/lint-no-dead-package-paths.test.mjs

Lines changed: 63 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { execFileSync } from 'node:child_process'
2+
import { mkdirSync, rmSync, writeFileSync } from 'node:fs'
23
import { resolve } from 'node:path'
34
import { fileURLToPath } from 'node:url'
45
import { describe, expect, it } from 'vitest'
@@ -8,9 +9,18 @@ const SCRIPT = resolve(
89
'../../lint-no-dead-package-paths.mjs',
910
)
1011

12+
const REPO_ROOT = resolve(fileURLToPath(import.meta.url), '../../..')
13+
1114
function run(...targets) {
15+
return runWith({}, ...targets)
16+
}
17+
18+
function runWith(opts, ...targets) {
1219
try {
13-
execFileSync('node', [SCRIPT, ...targets], { encoding: 'utf8' })
20+
execFileSync(process.execPath, [SCRIPT, ...targets], {
21+
encoding: 'utf8',
22+
...opts,
23+
})
1424
return { exitCode: 0, output: '' }
1525
} catch (err) {
1626
return {
@@ -63,12 +73,59 @@ describe('lint-no-dead-package-paths', () => {
6373
})
6474

6575
// The linters carry package paths of their own; `scripts/` was not scanned,
66-
// so a `packages/drizzle` allowlist entry for a package that never existed in
67-
// git history sat there unnoticed. Its own fixtures must stay exempt.
76+
// so lint-no-hardcoded-runners' `packages/drizzle/src/bin/runner.ts` allowlist
77+
// entry — added speculatively by c6715608, load-bearing 31 minutes later once
78+
// 9d259e6e created the file — sat dead for the two months after 413ca396
79+
// deleted the package. Its sibling entry for `packages/protect` was removed
80+
// with its package; this one was simply missed.
81+
//
82+
// Asserting the repo is clean would NOT pin this: the suite's first test
83+
// already does that, and both pass whether or not `scripts` is in TARGETS.
84+
// Plant an offender in the scanned directory instead.
6885
it('scans scripts/ but not its fixtures', () => {
69-
const r = run()
70-
expect(r.exitCode).toBe(0)
71-
expect(r.output).toBe('')
86+
const probe = resolve(REPO_ROOT, 'scripts/dead-path-probe.md')
87+
try {
88+
writeFileSync(probe, 'A reference to `packages/protect`, long gone.\n')
89+
const r = run()
90+
expect(r.exitCode).toBe(1)
91+
expect(r.output).toMatch(/scripts\/dead-path-probe\.md:1/)
92+
// `__tests__` stays skipped — the fixtures name dead packages on purpose,
93+
// and so do the comments in this very file.
94+
expect(r.output).not.toMatch(/__tests__/)
95+
} finally {
96+
rmSync(probe, { force: true })
97+
}
98+
})
99+
100+
// A package scaffolded a minute ago is live, but nothing about it is tracked
101+
// yet. Deriving the live set from `git ls-files` alone reported it as "does
102+
// not exist" — the same species of false alarm as the sentence-final one
103+
// above, pointed the other way, at a directory sitting right there on disk.
104+
it('treats a package that exists but is not yet tracked as live', () => {
105+
const pkg = resolve(REPO_ROOT, 'packages/lint-untracked-probe')
106+
try {
107+
mkdirSync(resolve(pkg, 'src'), { recursive: true })
108+
writeFileSync(resolve(pkg, 'src/index.ts'), 'export const x = 1\n')
109+
const r = run(fx('untracked-package.md'))
110+
expect(r.output).toBe('')
111+
expect(r.exitCode).toBe(0)
112+
} finally {
113+
rmSync(pkg, { recursive: true, force: true })
114+
}
115+
})
116+
117+
// The live set is derived by shelling out to git, so git failing is a mode
118+
// this linter has to own. Exiting 1 with a raw ENOENT stack trace would be
119+
// indistinguishable from a genuine lint failure; exit 2 says "the linter
120+
// could not run", not "your docs are wrong".
121+
it('exits 2 with an actionable message when git is unavailable', () => {
122+
const r = runWith({
123+
env: { PATH: resolve(REPO_ROOT, 'scripts/nonexistent') },
124+
})
125+
expect(r.exitCode).toBe(2)
126+
expect(r.output).toMatch(/git/)
127+
expect(r.output).toMatch(/checkout/)
128+
expect(r.output).not.toMatch(/at ModuleJob/)
72129
})
73130

74131
it('names the file and line of each offender', () => {

scripts/__tests__/lint-no-hardcoded-runners.test.mjs

Lines changed: 55 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { execFileSync } from 'node:child_process'
2+
import { readFileSync, rmSync, writeFileSync } from 'node:fs'
23
import { resolve } from 'node:path'
34
import { fileURLToPath } from 'node:url'
45
import { describe, expect, it } from 'vitest'
@@ -8,9 +9,9 @@ const SCRIPT = resolve(
89
'../../lint-no-hardcoded-runners.mjs',
910
)
1011

11-
function run(target) {
12+
function runScript(script, ...targets) {
1213
try {
13-
execFileSync('node', [SCRIPT, target], { encoding: 'utf8' })
14+
execFileSync(process.execPath, [script, ...targets], { encoding: 'utf8' })
1415
return { exitCode: 0, output: '' }
1516
} catch (err) {
1617
return {
@@ -20,6 +21,10 @@ function run(target) {
2021
}
2122
}
2223

24+
function run(target) {
25+
return runScript(SCRIPT, target)
26+
}
27+
2328
describe('lint-no-hardcoded-runners', () => {
2429
const fx = (name) =>
2530
resolve(fileURLToPath(import.meta.url), `../fixtures/${name}`)
@@ -70,3 +75,51 @@ describe('lint-no-hardcoded-runners', () => {
7075
expect(run(fx('identifier.ts')).exitCode).toBe(0)
7176
})
7277
})
78+
79+
// An allowlist entry is a standing exemption. When the file it names is deleted
80+
// — or stops carrying the `npx` literal it was excused for — the entry becomes
81+
// silent dead weight, and the next reader takes it as evidence that the file
82+
// still needs an exemption. `packages/drizzle/src/bin/runner.ts` sat here for
83+
// the two months after 413ca396 deleted its package, and surfaced only because
84+
// a sibling linter happened to start scanning `scripts/` (#772 review, finding
85+
// 15). That sibling can only ever catch the `packages/<name>` shape; this check
86+
// covers every entry, including a stale path inside a live package.
87+
describe('lint-no-hardcoded-runners — allowlist hygiene', () => {
88+
// A copy alongside the original so `REPO_ROOT` still resolves to the repo.
89+
const PROBE = resolve(
90+
fileURLToPath(import.meta.url),
91+
'../../allowlist-probe.mjs',
92+
)
93+
94+
function runWithExtraEntry(entry) {
95+
const src = readFileSync(SCRIPT, 'utf8').replace(
96+
'const ALLOWLISTED_PATHS = new Set([',
97+
`const ALLOWLISTED_PATHS = new Set([\n '${entry}',`,
98+
)
99+
try {
100+
writeFileSync(PROBE, src)
101+
return runScript(PROBE)
102+
} finally {
103+
rmSync(PROBE, { force: true })
104+
}
105+
}
106+
107+
it('rejects an entry whose file no longer exists', () => {
108+
const r = runWithExtraEntry('scripts/deleted-helper.mjs')
109+
expect(r.exitCode).toBe(2)
110+
expect(r.output).toMatch(/scripts\/deleted-helper\.mjs/)
111+
expect(r.output).toMatch(/no such file/)
112+
})
113+
114+
it('rejects an entry whose file no longer needs the exemption', () => {
115+
const r = runWithExtraEntry('scripts/vitest.config.mjs')
116+
expect(r.exitCode).toBe(2)
117+
expect(r.output).toMatch(/scripts\/vitest\.config\.mjs/)
118+
expect(r.output).toMatch(/no longer contains/)
119+
})
120+
121+
it('accepts the allowlist as it stands', () => {
122+
const r = runScript(SCRIPT)
123+
expect(r.exitCode).toBe(0)
124+
})
125+
})

scripts/lint-no-dead-package-paths.mjs

Lines changed: 54 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -37,8 +37,9 @@ const SKIP_DIRS = new Set([
3737
'plans',
3838
'superpowers',
3939
'.git',
40-
// Fixtures for this linter's own self-tests deliberately reference deleted
41-
// packages; scanning them would make the suite unrunnable.
40+
// This linter's own self-tests deliberately reference deleted packages —
41+
// in the fixtures, and in the prose comments of the test files themselves.
42+
// Scanning them would make the suite unrunnable.
4243
'__tests__',
4344
])
4445
const SKIP_FILES = new Set(['CHANGELOG.md'])
@@ -58,7 +59,7 @@ const TEXT_EXT = /\.(md|ya?ml|json|mjs|ts|txt)$/
5859
// (#772 review, finding 15).
5960
const PACKAGE_REF = /packages\/([a-zA-Z0-9](?:[a-zA-Z0-9._-]*[a-zA-Z0-9])?)/g
6061

61-
// Live packages come from what git TRACKS, not from what is on disk.
62+
// Live packages come from git, not from what is on disk.
6263
//
6364
// `readdirSync` was wrong in the direction that matters: deleting a package
6465
// leaves its `dist/` and `node_modules/` behind, so the directory still exists
@@ -71,18 +72,61 @@ const PACKAGE_REF = /packages\/([a-zA-Z0-9](?:[a-zA-Z0-9._-]*[a-zA-Z0-9])?)/g
7172
// Note this deliberately does NOT require a `package.json`: `packages/utils` has
7273
// none (it is two loose files consumed by relative path from `packages/nextjs`)
7374
// yet is tracked, live, and referenced from AGENTS.md.
75+
//
76+
// Shelling out to git is a dependency this linter has to own: git missing, or a
77+
// tree with no `.git`, must not read as "every package is dead".
78+
function gitPackagePaths(...args) {
79+
try {
80+
return execFileSync('git', args, {
81+
cwd: REPO_ROOT,
82+
encoding: 'utf8',
83+
maxBuffer: 64 * 1024 * 1024,
84+
})
85+
.split('\0')
86+
.filter(Boolean)
87+
} catch (err) {
88+
const detail = String(err.stderr || err.message || '').trim()
89+
console.error(
90+
`Could not list packages via \`git ${args.join(' ')}\`:\n\n ${detail}\n\n` +
91+
'This linter derives the live package set from git, so it cannot run\n' +
92+
'without git on PATH or outside a git checkout.',
93+
)
94+
// Exit 2, not 1: the linter failed to run. Exit 1 means it ran and found
95+
// dead references, which is a different thing to go and fix.
96+
process.exit(2)
97+
}
98+
}
99+
74100
const livePackages = new Set(
75-
execFileSync('git', ['ls-files', '-z', 'packages'], {
76-
cwd: REPO_ROOT,
77-
encoding: 'utf8',
78-
maxBuffer: 64 * 1024 * 1024,
79-
})
80-
.split('\0')
81-
.filter(Boolean)
101+
[
102+
...gitPackagePaths('ls-files', '-z', 'packages'),
103+
// Untracked but not ignored. A package scaffolded a minute ago is live
104+
// even though nothing about it is staged yet, and reporting it as "does
105+
// not exist" is the sentence-final false alarm all over again, pointed the
106+
// other way. `--directory` is deliberately NOT passed: it collapses an
107+
// all-ignored directory to a single entry, which would resurrect exactly
108+
// the `dist/`-and-`node_modules/` shells this linter exists to catch.
109+
...gitPackagePaths(
110+
'ls-files',
111+
'--others',
112+
'--exclude-standard',
113+
'-z',
114+
'packages',
115+
),
116+
]
82117
.map((file) => file.split('/')[1])
83118
.filter(Boolean),
84119
)
85120

121+
if (livePackages.size === 0) {
122+
console.error(
123+
'git reported no packages at all under `packages/`. Refusing to run —\n' +
124+
'every reference would be flagged. Check that `packages/` is present and\n' +
125+
'not wholly ignored.',
126+
)
127+
process.exit(2)
128+
}
129+
86130
function* walk(abs) {
87131
const stat = statSync(abs)
88132
if (stat.isFile()) {

0 commit comments

Comments
 (0)