(TML-2397) M3 — Contract spaces: cipherstash extension as a contract space#439
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (28)
📝 WalkthroughWalkthroughThis PR adds an idempotent extension migration materialiser (public API and CLI wiring), extends invariant derivation to any op with a string ChangesIdempotent Extension Migration Materialization
CipherStash SQL Extension Package
Integration Test Updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
@prisma-next/mongo-runtime
@prisma-next/family-mongo
@prisma-next/sql-runtime
@prisma-next/family-sql
@prisma-next/extension-arktype-json
@prisma-next/extension-cipherstash
@prisma-next/middleware-telemetry
@prisma-next/mongo
@prisma-next/extension-paradedb
@prisma-next/extension-pgvector
@prisma-next/postgres
@prisma-next/sql-orm-client
@prisma-next/sqlite
@prisma-next/target-mongo
@prisma-next/adapter-mongo
@prisma-next/driver-mongo
@prisma-next/contract
@prisma-next/utils
@prisma-next/config
@prisma-next/errors
@prisma-next/framework-components
@prisma-next/operations
@prisma-next/ts-render
@prisma-next/contract-authoring
@prisma-next/ids
@prisma-next/psl-parser
@prisma-next/psl-printer
@prisma-next/cli
@prisma-next/emitter
@prisma-next/migration-tools
prisma-next
@prisma-next/vite-plugin-contract-emit
@prisma-next/mongo-codec
@prisma-next/mongo-contract
@prisma-next/mongo-value
@prisma-next/mongo-contract-psl
@prisma-next/mongo-contract-ts
@prisma-next/mongo-emitter
@prisma-next/mongo-schema-ir
@prisma-next/mongo-query-ast
@prisma-next/mongo-orm
@prisma-next/mongo-query-builder
@prisma-next/mongo-lowering
@prisma-next/mongo-wire
@prisma-next/sql-contract
@prisma-next/sql-errors
@prisma-next/sql-operations
@prisma-next/sql-schema-ir
@prisma-next/sql-contract-psl
@prisma-next/sql-contract-ts
@prisma-next/sql-contract-emitter
@prisma-next/sql-lane-query-builder
@prisma-next/sql-relational-core
@prisma-next/sql-builder
@prisma-next/target-postgres
@prisma-next/target-sqlite
@prisma-next/adapter-postgres
@prisma-next/adapter-sqlite
@prisma-next/driver-postgres
@prisma-next/driver-sqlite
commit: |
9b25e12 to
d51b446
Compare
c2bdd3d to
4448c05
Compare
d51b446 to
48437e5
Compare
a7aa92d to
f395e88
Compare
48437e5 to
54f296d
Compare
…adRefsBySpace) scenario-a.e2e.integration.test.ts called executePerSpaceDbApply (deleted by the M2.5 aggregate refactor) and am11-am12-no-descriptor.test.ts spelled VerifyContractSpacesInputs's head-ref map as pinnedHashesBySpace (renamed by M2.5b's drop-the-pinned-qualifier sweep). Migrate both: route the scenario-a apply path through executeDbInit (which encapsulates the loader → planner → runner pipeline and the additive-only policy that INIT_ADDITIVE_POLICY used to express in-line) and rename pinnedHashesBySpace → headRefsBySpace at both call sites. Resolves M3 PR #439 Type Check + Lint failures.
dcee822 to
5f131bf
Compare
…adRefsBySpace) scenario-a.e2e.integration.test.ts called executePerSpaceDbApply (deleted by the M2.5 aggregate refactor) and am11-am12-no-descriptor.test.ts spelled VerifyContractSpacesInputs's head-ref map as pinnedHashesBySpace (renamed by M2.5b's drop-the-pinned-qualifier sweep). Migrate both: route the scenario-a apply path through executeDbInit (which encapsulates the loader → planner → runner pipeline and the additive-only policy that INIT_ADDITIVE_POLICY used to express in-line) and rename pinnedHashesBySpace → headRefsBySpace at both call sites. Resolves M3 PR #439 Type Check + Lint failures.
5f131bf to
ef1ce6f
Compare
18d9e73 to
104d09a
Compare
…adRefsBySpace) scenario-a.e2e.integration.test.ts called executePerSpaceDbApply (deleted by the M2.5 aggregate refactor) and am11-am12-no-descriptor.test.ts spelled VerifyContractSpacesInputs's head-ref map as pinnedHashesBySpace (renamed by M2.5b's drop-the-pinned-qualifier sweep). Migrate both: route the scenario-a apply path through executeDbInit (which encapsulates the loader → planner → runner pipeline and the additive-only policy that INIT_ADDITIVE_POLICY used to express in-line) and rename pinnedHashesBySpace → headRefsBySpace at both call sites. Resolves M3 PR #439 Type Check + Lint failures.
ef1ce6f to
75e5941
Compare
…adRefsBySpace) scenario-a.e2e.integration.test.ts called executePerSpaceDbApply (deleted by the M2.5 aggregate refactor) and am11-am12-no-descriptor.test.ts spelled VerifyContractSpacesInputs's head-ref map as pinnedHashesBySpace (renamed by M2.5b's drop-the-pinned-qualifier sweep). Migrate both: route the scenario-a apply path through executeDbInit (which encapsulates the loader → planner → runner pipeline and the additive-only policy that INIT_ADDITIVE_POLICY used to express in-line) and rename pinnedHashesBySpace → headRefsBySpace at both call sites. Resolves M3 PR #439 Type Check + Lint failures.
75e5941 to
f27e4ee
Compare
f27e4ee to
c8e970b
Compare
…adRefsBySpace) scenario-a.e2e.integration.test.ts called executePerSpaceDbApply (deleted by the M2.5 aggregate refactor) and am11-am12-no-descriptor.test.ts spelled VerifyContractSpacesInputs's head-ref map as pinnedHashesBySpace (renamed by M2.5b's drop-the-pinned-qualifier sweep). Migrate both: route the scenario-a apply path through executeDbInit (which encapsulates the loader → planner → runner pipeline and the additive-only policy that INIT_ADDITIVE_POLICY used to express in-line) and rename pinnedHashesBySpace → headRefsBySpace at both call sites. Resolves M3 PR #439 Type Check + Lint failures.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/integration/test/cli.db-verify.aggregate-schema.test.ts (1)
191-198:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlso lock the warning count to zero.
The test says “zero schema issues”, but it only asserts
fail === 0. A regression that turns the mismatch into a warning would still pass.💚 Proposed change
const schema = parsed['schema'] as | { counts?: { fail?: number; warn?: number } } | undefined; expect(schema?.counts?.fail ?? -1).toBe(0); + expect(schema?.counts?.warn ?? -1).toBe(0);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/integration/test/cli.db-verify.aggregate-schema.test.ts` around lines 191 - 198, The test currently only asserts schema fail count is zero, leaving warn count unverified; update the assertion in the test (where parsed and schema are declared) to also assert that schema?.counts?.warn is 0 (e.g., add an expect on schema?.counts?.warn ?? -1 toBe(0)) so both fail and warn counts are locked to zero and the "zero schema issues" expectation cannot be satisfied by converting fails into warnings.packages/1-framework/3-tooling/migration/src/invariants.ts (1)
60-62:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
Object.hasOwn()to check own properties.The current property access traverses the prototype chain, which means a prototype-inherited
invariantIdcould be picked up and incorrectly included in theprovidedInvariantsset. This could affect marker bookkeeping and regeneration decisions.Suggested fix
function readInvariantId(op: MigrationPlanOperation): string | undefined { + if (!Object.hasOwn(op, 'invariantId')) { + return undefined; + } const candidate = (op as { invariantId?: unknown }).invariantId; return typeof candidate === 'string' ? candidate : undefined; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/1-framework/3-tooling/migration/src/invariants.ts` around lines 60 - 62, The readInvariantId function currently reads op.invariantId which can come from the prototype chain; change it to first check Object.hasOwn(op, 'invariantId') and only then read (op as { invariantId?: unknown }).invariantId, returning the value if it's a string and otherwise undefined; this ensures only own properties are considered for providedInvariants and prevents prototype-inherited values from being included.
🧹 Nitpick comments (7)
packages/3-extensions/cipherstash/tsconfig.json (1)
3-8: ⚡ Quick winUse the dedicated TSC output directory here.
Pointing
tscatdistwill mix compiler emit with the package’s built artifacts. This package should follow the repo’sdist-tsc/dist-tsc-prodsplit instead.♻️ Proposed change
"compilerOptions": { "rootDir": ".", - "outDir": "dist" + "outDir": "dist-tsc" }, "include": ["src/**/*.ts", "test/**/*.ts"], - "exclude": ["dist"] + "exclude": ["dist", "dist-tsc", "dist-tsc-prod"]Based on learnings, "Ensure TypeScript emit outputs are configured to dedicated output directories (e.g., dist-tsc for regular builds and dist-tsc-prod for production builds) via tsconfig settings."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/3-extensions/cipherstash/tsconfig.json` around lines 3 - 8, Update the tsconfig.json to use a dedicated TypeScript emit directory instead of "dist": change compilerOptions.outDir from "dist" to "dist-tsc" for regular builds (and add a separate tsconfig.prod that extends this file and sets outDir to "dist-tsc-prod" if you need a production emit), ensuring compilerOptions.outDir is the only field changed and keeping include/exclude as-is so emitted JS won’t be mixed with package build artifacts.packages/3-extensions/cipherstash/src/core/constants.ts (1)
80-86: ⚡ Quick winConstrain these helpers to the known type-name unions.
Accepting
stringhere gives up the typo protection the const arrays already encode. Narrowing the parameter types keeps callers aligned with the vendored bundle surface.♻️ Proposed change
export const CIPHERSTASH_INVARIANTS = { installBundle: 'cipherstash:install-eql-bundle-v1', createConfiguration: 'cipherstash:create-eql_v2_configuration-v1', createConfigurationState: 'cipherstash:create-eql_v2_configuration_state-v1', createEncrypted: 'cipherstash:create-eql_v2_encrypted-v1', - createDomain: (name: string) => `cipherstash:create-eql_v2_${name}-v1`, - createOreComposite: (name: string) => `cipherstash:create-eql_v2_${name}-v1`, + createDomain: (name: (typeof EQL_V2_DOMAIN_TYPES)[number]) => + `cipherstash:create-eql_v2_${name}-v1`, + createOreComposite: (name: (typeof EQL_V2_ORE_COMPOSITE_TYPES)[number]) => + `cipherstash:create-eql_v2_${name}-v1`, } as const;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/3-extensions/cipherstash/src/core/constants.ts` around lines 80 - 86, The helper factories createDomain and createOreComposite currently accept a plain string which loses typo-safety; change their parameter type from string to a narrow union of known type-names (e.g. type DomainName = 'nameA' | 'nameB' or derive it from the existing const arrays) and update CIPHERSTASH_INVARIANTS.createDomain(name: DomainName) and createOreComposite(name: DomainName) accordingly so callers are constrained to the vendored bundle surface; add or reuse a type alias (e.g. DomainNames or KnownTypeNames) exported alongside the constants to keep inference and autocompletion working.packages/1-framework/3-tooling/migration/src/io.ts (1)
127-150: ⚡ Quick winDrop AC/AM/spec references from the helper docs.
This block still names transient artifacts (
AC-7,AM12,T1.7, spec path). Please describe the idempotency contract directly instead.As per coding guidelines, "Source-code comments must not reference transient project artifacts including: projects//... paths, milestone-task IDs ... milestone-named acceptance criteria ... and prose attributions like ...; instead describe the constraint or behavior itself".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/1-framework/3-tooling/migration/src/io.ts` around lines 127 - 150, The docblock for the idempotent helper should drop transient spec identifiers (AC-7, AM12, T1.7, spec path) and instead state the idempotency contract plainly: explain that materialiseMigrationPackageIdempotent (or the idempotent variant of materialiseMigrationPackage) will skip writing when <targetDir>/<pkg.dirName>/ exists and return { written: false }, write the three files and return { written: true } when missing, and propagate any other I/O errors from stat; also mention its callers like runContractSpaceExtensionMigrationsPass only as contextual usage, not by spec IDs. Keep the same examples and behavior description but remove all project-specific IDs and spec path references.packages/3-extensions/cipherstash/src/core/cipherstash-codec.ts (2)
160-170: ⚡ Quick winFail fast on impossible field-event payloads.
These guards turn a planner contract violation into a silent no-op, which would under-emit CipherStash DDL instead of surfacing the bad event shape.
Suggested change
if (event === 'added') { - if (newField === undefined) return []; + if (newField === undefined) { + throw new Error('cipherstash:string@1 added event missing newField'); + } return isSearchable(newField.typeParams) ? [buildAddOp(tableName, fieldName)] : []; } if (event === 'dropped') { - if (priorField === undefined) return []; + if (priorField === undefined) { + throw new Error('cipherstash:string@1 dropped event missing priorField'); + } return isSearchable(priorField.typeParams) ? [buildRemoveOp(tableName, fieldName)] : []; } - if (priorField === undefined || newField === undefined) return []; + if (priorField === undefined || newField === undefined) { + throw new Error('cipherstash:string@1 altered event missing field payload'); + }As per coding guidelines, "Prefer assertions over defensive checks when data is guaranteed to be valid - use non-null assertions (e.g.,
columnMeta!) after validation checks instead of optional chaining".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/3-extensions/cipherstash/src/core/cipherstash-codec.ts` around lines 160 - 170, The code currently silently returns [] for impossible combinations of event and field payloads; instead, in the 'added' branch verify newField is present and throw an error (or assert) if it's undefined, and in the 'dropped' branch verify priorField is present and throw if undefined; then use non-null assertions (e.g., newField! / priorField!) when calling isSearchable and buildAddOp/buildRemoveOp so the function fails fast on bad planner payloads rather than silently emitting no-op.
1-35: ⚡ Quick winRemove spec/path references from source comments.
These comments currently embed transient artifacts like
sub-spec § 5and an internal package path. Please rewrite them in behavior terms only so they stay valid after docs/layout changes.As per coding guidelines, "Source-code comments must not reference transient project artifacts including: projects//... paths, milestone-task IDs ... and prose attributions like ... 'sub-spec § 4' ...; instead describe the constraint or behavior itself".
Also applies to: 149-153, 184-196
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/3-extensions/cipherstash/src/core/cipherstash-codec.ts` around lines 1 - 35, The file header comment for the cipherstash codec contains transient references (e.g. "sub-spec § 5", "cipherstash sub-spec § 4", and the internal package path `@prisma-next/family-sql/control`) that must be removed; edit the comment around the `cipherstash:string@1` codec and the `invariantId` template so it only describes observable behavior and trigger conditions (what events cause the hook to run, the mapping from added/dropped/altered + searchable -> which ops to emit, and that the `invariantId` is stable/deterministic), and remove any spec/section numbers and internal package path mentions while preserving the same behavioral intent and references to the codec id and invariantId template.packages/3-extensions/cipherstash/src/exports/control.ts (1)
1-27: ⚡ Quick winKeep the export-surface docs free of transient artifact references.
The new comments embed project/spec/test references (
M1 + M2,FR13,sub-spec § 5,test/descriptor.test.ts, internal package paths). Please restate those as stable behavior guarantees.As per coding guidelines, "Source-code comments must not reference transient project artifacts including: projects//... paths, milestone-task IDs ... milestone-named acceptance criteria ... and prose attributions like ... 'sub-spec § 4' ...; instead describe the constraint or behavior itself".
Also applies to: 51-57
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/3-extensions/cipherstash/src/exports/control.ts` around lines 1 - 27, Remove transient project and spec artifact references from the top-of-file documentation and replace them with stable behavioral guarantees: state that the descriptor exposes contractSpace (the contractSpace.{contractJson,migrations,headRef} surface) so the framework’s per-space planner/runner/verifier manages CipherStash database scaffolding at authoring time; state that the types.codecTypes.controlPlaneHooks[CIPHERSTASH_STRING_CODEC_ID] entry is a lifecycle hook implementing add_search_config, remove_search_config and rotation behavior for searchable Encrypted<string> columns; and state plainly that databaseDependencies is intentionally omitted (i.e. CipherStash does not rely on any legacy databaseDependencies.init mechanism). Update the prose where the comment mentions M1/M2, FR13, sub-spec § 4/5 and test/descriptor.test.ts to instead assert these concrete behaviors and guarantees (refer to symbols contractSpace, types.codecTypes.controlPlaneHooks[CIPHERSTASH_STRING_CODEC_ID], and databaseDependencies to locate the relevant documentation).packages/3-extensions/cipherstash/src/core/contract.ts (1)
1-26: ⚡ Quick winReplace milestone/spec references in source comments.
This block hardcodes transient labels like
R1,M3,FR9,AC8, and sub-spec references. Those comments will stale quickly and the repo guideline explicitly asks source comments to describe the behavior directly instead of pointing at milestone artifacts.As per coding guidelines, "Source-code comments must not reference transient project artifacts including: ... milestone-task IDs ... milestone-named acceptance criteria ...; instead describe the constraint or behavior itself".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/3-extensions/cipherstash/src/core/contract.ts` around lines 1 - 26, The header comment in contract.ts uses transient milestone/spec labels (R1, M3, FR9, AC8, etc.); replace those references with a self-contained description of the behavior and constraints: remove milestone IDs and instead state plainly that the current SqlStorage IR only models tables and parameterised type instances, that only eql_v2_configuration is represented in the IR while other typed objects are recorded under meta.cipherstashFutureIR as placeholders, and note that the missing types are still created in the DB by installEqlBundle (which contains the vendored SQL in ./eql-bundle); preserve the technical facts (StorageTable, StorageTypeInstance shape, codec limitations) but eliminate any milestone/task identifiers or sub-spec citations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/1-framework/3-tooling/migration/src/io.ts`:
- Around line 151-169: The current pathExists(p) returns true for any filesystem
entry so materialiseExtensionMigrationPackageIfMissing may skip creation when a
file/symlink exists; change pathExists to await stat(p), return
statResult.isDirectory() (and still catch ENOENT to return false) so it only
reports true for existing directories—update any references to pathExists used
by materialiseExtensionMigrationPackageIfMissing accordingly.
In `@packages/3-extensions/cipherstash/package.json`:
- Around line 42-53: The package.json advertises a root entry via
"main"/"module"/"types" but the "exports" map only exposes "./control", which
prevents bare imports; update the "exports" map to include "." pointing to
"./dist/control.mjs" (and the types entry) so the root import works, or
alternatively remove the "main", "module", and "types" fields to enforce
subpath-only usage—adjust the "exports" entry that references "./control" and
the root "." export to be consistent with the current distribution filenames
(e.g., "./dist/control.mjs" and its types).
In `@packages/3-extensions/cipherstash/README.md`:
- Around line 8-10: The README currently links to transient planning artifacts
(the link text `projects/extension-contract-spaces` and the Linear milestone
`TML-2397`) which will rot; replace those links with durable documentation
targets (e.g., a docs/ page or the package README) or remove them if no stable
target exists, and apply the same change to the other occurrences mentioned
around lines 62-67 so the cipherstash package README contains only permanent
documentation links.
In `@packages/3-extensions/cipherstash/src/core/eql-bundle.ts`:
- Around line 1-35: The top-of-file docblock in eql-bundle.ts contains transient
project references (task IDs, branch/commit hashes, repo paths) that must be
removed; edit the comment to retain only the stable behavioral/runtime
guarantees about the vendored bundle (what EQL_BUNDLE_SQL represents, that it
must be included byte-for-byte in the cipherstash:install-eql-bundle-v1
migration/execute[], and the impact on migrationHash vs headRef.hash), and
replace any detailed historical or branch-specific notes with a short pointer to
a canonical doc or issue link; ensure references to the exported identifier
EQL_BUNDLE_SQL and the migration op name installEqlBundle remain clear.
In `@packages/3-extensions/cipherstash/src/core/migrations.ts`:
- Around line 25-31: The file migrations.ts in src/core imports control-plane
packages (MigrationPackage, ContractSpaceHeadRef and computeMigrationHash) and
exports cipherstashBaselineMigration / cipherstashHeadRef, which violates the
core→control boundary; move assembly of cipherstashBaselineMigration and
cipherstashHeadRef into a control-plane module (e.g. create
src/exports/control.ts), remove imports of
`@prisma-next/framework-components/control` and `@prisma-next/migration-tools/hash`
from src/core/migrations.ts, keep only pure constants/helpers in migrations.ts,
and in the new control module import the helpers from src/core/migrations.ts
plus computeMigrationHash and the MigrationPackage/ContractSpaceHeadRef types to
construct and export cipherstashBaselineMigration and cipherstashHeadRef.
In `@packages/3-extensions/cipherstash/test/scenario-a.e2e.integration.test.ts`:
- Around line 178-205: The synthetic EQL bundle schema in
buildSyntheticEqlBundleSql currently creates public."eql_v2_configuration" with
columns id/"table"/"column"/index_name/cast_as which diverges from the pinned
contract; change the table to the published contract shape (e.g. id serial
PRIMARY KEY, state text NOT NULL, data jsonb NOT NULL) and update the two
functions ("${EQL_V2_SCHEMA}".add_search_config and
"${EQL_V2_SCHEMA}".remove_search_config) to insert/delete using the state/data
fields (storing the config JSON in data and appropriate state value) so the
synthetic bundle remains contract-compatible; apply the same schema and function
fixes in the other synthetic bundle blocks referenced (around the other ranges).
---
Outside diff comments:
In `@packages/1-framework/3-tooling/migration/src/invariants.ts`:
- Around line 60-62: The readInvariantId function currently reads op.invariantId
which can come from the prototype chain; change it to first check
Object.hasOwn(op, 'invariantId') and only then read (op as { invariantId?:
unknown }).invariantId, returning the value if it's a string and otherwise
undefined; this ensures only own properties are considered for
providedInvariants and prevents prototype-inherited values from being included.
In `@test/integration/test/cli.db-verify.aggregate-schema.test.ts`:
- Around line 191-198: The test currently only asserts schema fail count is
zero, leaving warn count unverified; update the assertion in the test (where
parsed and schema are declared) to also assert that schema?.counts?.warn is 0
(e.g., add an expect on schema?.counts?.warn ?? -1 toBe(0)) so both fail and
warn counts are locked to zero and the "zero schema issues" expectation cannot
be satisfied by converting fails into warnings.
---
Nitpick comments:
In `@packages/1-framework/3-tooling/migration/src/io.ts`:
- Around line 127-150: The docblock for the idempotent helper should drop
transient spec identifiers (AC-7, AM12, T1.7, spec path) and instead state the
idempotency contract plainly: explain that materialiseMigrationPackageIdempotent
(or the idempotent variant of materialiseMigrationPackage) will skip writing
when <targetDir>/<pkg.dirName>/ exists and return { written: false }, write the
three files and return { written: true } when missing, and propagate any other
I/O errors from stat; also mention its callers like
runContractSpaceExtensionMigrationsPass only as contextual usage, not by spec
IDs. Keep the same examples and behavior description but remove all
project-specific IDs and spec path references.
In `@packages/3-extensions/cipherstash/src/core/cipherstash-codec.ts`:
- Around line 160-170: The code currently silently returns [] for impossible
combinations of event and field payloads; instead, in the 'added' branch verify
newField is present and throw an error (or assert) if it's undefined, and in the
'dropped' branch verify priorField is present and throw if undefined; then use
non-null assertions (e.g., newField! / priorField!) when calling isSearchable
and buildAddOp/buildRemoveOp so the function fails fast on bad planner payloads
rather than silently emitting no-op.
- Around line 1-35: The file header comment for the cipherstash codec contains
transient references (e.g. "sub-spec § 5", "cipherstash sub-spec § 4", and the
internal package path `@prisma-next/family-sql/control`) that must be removed;
edit the comment around the `cipherstash:string@1` codec and the `invariantId`
template so it only describes observable behavior and trigger conditions (what
events cause the hook to run, the mapping from added/dropped/altered +
searchable -> which ops to emit, and that the `invariantId` is
stable/deterministic), and remove any spec/section numbers and internal package
path mentions while preserving the same behavioral intent and references to the
codec id and invariantId template.
In `@packages/3-extensions/cipherstash/src/core/constants.ts`:
- Around line 80-86: The helper factories createDomain and createOreComposite
currently accept a plain string which loses typo-safety; change their parameter
type from string to a narrow union of known type-names (e.g. type DomainName =
'nameA' | 'nameB' or derive it from the existing const arrays) and update
CIPHERSTASH_INVARIANTS.createDomain(name: DomainName) and
createOreComposite(name: DomainName) accordingly so callers are constrained to
the vendored bundle surface; add or reuse a type alias (e.g. DomainNames or
KnownTypeNames) exported alongside the constants to keep inference and
autocompletion working.
In `@packages/3-extensions/cipherstash/src/core/contract.ts`:
- Around line 1-26: The header comment in contract.ts uses transient
milestone/spec labels (R1, M3, FR9, AC8, etc.); replace those references with a
self-contained description of the behavior and constraints: remove milestone IDs
and instead state plainly that the current SqlStorage IR only models tables and
parameterised type instances, that only eql_v2_configuration is represented in
the IR while other typed objects are recorded under meta.cipherstashFutureIR as
placeholders, and note that the missing types are still created in the DB by
installEqlBundle (which contains the vendored SQL in ./eql-bundle); preserve the
technical facts (StorageTable, StorageTypeInstance shape, codec limitations) but
eliminate any milestone/task identifiers or sub-spec citations.
In `@packages/3-extensions/cipherstash/src/exports/control.ts`:
- Around line 1-27: Remove transient project and spec artifact references from
the top-of-file documentation and replace them with stable behavioral
guarantees: state that the descriptor exposes contractSpace (the
contractSpace.{contractJson,migrations,headRef} surface) so the framework’s
per-space planner/runner/verifier manages CipherStash database scaffolding at
authoring time; state that the
types.codecTypes.controlPlaneHooks[CIPHERSTASH_STRING_CODEC_ID] entry is a
lifecycle hook implementing add_search_config, remove_search_config and rotation
behavior for searchable Encrypted<string> columns; and state plainly that
databaseDependencies is intentionally omitted (i.e. CipherStash does not rely on
any legacy databaseDependencies.init mechanism). Update the prose where the
comment mentions M1/M2, FR13, sub-spec § 4/5 and test/descriptor.test.ts to
instead assert these concrete behaviors and guarantees (refer to symbols
contractSpace, types.codecTypes.controlPlaneHooks[CIPHERSTASH_STRING_CODEC_ID],
and databaseDependencies to locate the relevant documentation).
In `@packages/3-extensions/cipherstash/tsconfig.json`:
- Around line 3-8: Update the tsconfig.json to use a dedicated TypeScript emit
directory instead of "dist": change compilerOptions.outDir from "dist" to
"dist-tsc" for regular builds (and add a separate tsconfig.prod that extends
this file and sets outDir to "dist-tsc-prod" if you need a production emit),
ensuring compilerOptions.outDir is the only field changed and keeping
include/exclude as-is so emitted JS won’t be mixed with package build artifacts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 12e16159-33d9-4a02-8b72-e1d161e0b2c3
⛔ Files ignored due to path filters (4)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlprojects/extension-contract-spaces/plan.mdis excluded by!projects/**projects/extension-contract-spaces/specs/cipherstash-migration.spec.mdis excluded by!projects/**projects/extension-contract-spaces/specs/framework-mechanism.spec.mdis excluded by!projects/**
📒 Files selected for processing (28)
packages/1-framework/3-tooling/cli/src/utils/contract-space-extension-migrations-pass.tspackages/1-framework/3-tooling/migration/src/exports/io.tspackages/1-framework/3-tooling/migration/src/invariants.tspackages/1-framework/3-tooling/migration/src/io.tspackages/1-framework/3-tooling/migration/test/invariants.test.tspackages/1-framework/3-tooling/migration/test/materialise-extension-migration-package-if-missing.test.tspackages/3-extensions/cipherstash/README.mdpackages/3-extensions/cipherstash/biome.jsoncpackages/3-extensions/cipherstash/package.jsonpackages/3-extensions/cipherstash/src/core/cipherstash-codec.tspackages/3-extensions/cipherstash/src/core/constants.tspackages/3-extensions/cipherstash/src/core/contract.tspackages/3-extensions/cipherstash/src/core/eql-bundle.tspackages/3-extensions/cipherstash/src/core/eql-install.generated.tspackages/3-extensions/cipherstash/src/core/migrations.tspackages/3-extensions/cipherstash/src/exports/control.tspackages/3-extensions/cipherstash/test/am11-am12-no-descriptor.test.tspackages/3-extensions/cipherstash/test/cipherstash-codec.test.tspackages/3-extensions/cipherstash/test/descriptor.test.tspackages/3-extensions/cipherstash/test/scenario-a.e2e.integration.test.tspackages/3-extensions/cipherstash/test/scenario-c-bump.test.tspackages/3-extensions/cipherstash/tsconfig.jsonpackages/3-extensions/cipherstash/tsconfig.prod.jsonpackages/3-extensions/cipherstash/tsdown.config.tspackages/3-extensions/cipherstash/vitest.config.tstest/integration/package.jsontest/integration/test/cli.db-verify.aggregate-schema.test.tstest/integration/test/fixtures/cli/cli-e2e-test-app/package.json
💤 Files with no reviewable changes (2)
- test/integration/test/fixtures/cli/cli-e2e-test-app/package.json
- test/integration/package.json
|
All items from the review body addressed: Outside-diff fixes:
Nitpick fixes:
|
There was a problem hiding this comment.
♻️ Duplicate comments (2)
packages/3-extensions/cipherstash/README.md (1)
17-19:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThis README section is stale now.
The package does wire the searchable codec lifecycle hook in
src/exports/control.ts, so saying it is “not in this round” now contradicts the implementation and keeps rollout-specific wording in durable docs.As per coding guidelines: "Keep docs current (READMEs, rules, links)".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/3-extensions/cipherstash/README.md` around lines 17 - 19, The README contains a stale claim that the codec runtime and the `searchable: true` codec lifecycle hook are “not in this round”; remove or revise that sentence to reflect the current implementation by stating that the searchable codec lifecycle hook is wired up (see src/exports/control.ts) and the codec runtime handles encoding/decoding of Encrypted<string> payloads; update wording to be implementation-accurate and remove rollout-specific language so the docs match the code (reference: the searchable lifecycle hook in src/exports/control.ts and any codec runtime exports).packages/3-extensions/cipherstash/test/scenario-a.e2e.integration.test.ts (1)
179-209:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThe synthetic bundle still doesn't represent the pinned contract state.
The comment above says this stub must stay contract-compatible, but the SQL here only creates
eql_v2_encryptedpluseql_v2_configuration(state text, data jsonb). The pinned CipherStash contract surface is broader than that, and the preserved structural ops are invariant no-ops, so this test can still writeCIPHERSTASH_STORAGE_HASHfor a database that would fail a real contract-space verify.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/3-extensions/cipherstash/test/scenario-a.e2e.integration.test.ts` around lines 179 - 209, The synthetic EQL bundle SQL in buildSyntheticEqlBundleSql only creates EQL_V2_ENCRYPTED_TYPE and EQL_V2_CONFIGURATION_TABLE plus add_search_config/remove_search_config, but must mirror the full pinned CipherStash contract surface so contract-space verification cannot be bypassed; update buildSyntheticEqlBundleSql to include all contract-visible types/tables/functions present in the pinned contract (not just the configuration table) and implement preserved structural ops as harmless no-ops that keep identical schema shape and behavior. Specifically, expand the SQL to create any additional types, tables, indexes, and functions that the real contract exposes (matching names and column shapes used by the verifier), ensure functions like add_search_config and remove_search_config remain but operate as no-ops that maintain schema invariants, and preserve identifiers EQL_V2_SCHEMA, EQL_V2_ENCRYPTED_TYPE, and EQL_V2_CONFIGURATION_TABLE so the synthesized DB produces the same CIPHERSTASH_STORAGE_HASH as the real contract.
🧹 Nitpick comments (5)
test/integration/test/cli.db-verify.aggregate-schema.test.ts (3)
59-64: ⚡ Quick winRemove milestone reference from inline comment.
This comment references "M2.5b loader integrity gate" which is a transient milestone artifact. Describe the actual invariant-loading requirement instead.
♻️ Proposed fix
- // The on-disk head ref's `invariants` and the migration package's - // `providedInvariants` must both round-trip through - // `deriveProvidedInvariants` (M2.5b loader integrity gate, error - // PN-MIG-5002). The test extension's baseline op carries an - // `invariantId`, so the derivation produces `[TEST_BASELINE_INVARIANT_ID]` - // — match that on disk for both the head ref and migration metadata. + // The on-disk head ref's `invariants` and the migration package's + // `providedInvariants` must both round-trip through + // `deriveProvidedInvariants` (loader integrity gate). The test + // extension's baseline op carries an `invariantId`, so the derivation + // produces `[TEST_BASELINE_INVARIANT_ID]` — match that on disk for + // both the head ref and migration metadata.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/integration/test/cli.db-verify.aggregate-schema.test.ts` around lines 59 - 64, Update the inline comment to remove the transient milestone phrase "M2.5b loader integrity gate" and instead state the concrete requirement: both the on-disk head ref's `invariants` and the migration package's `providedInvariants` must round-trip through `deriveProvidedInvariants` and produce the same array (e.g. `[TEST_BASELINE_INVARIANT_ID]` given the test baseline op's `invariantId`), and that this derived value must match the stored values for both the head ref and the migration metadata; reference the symbols `deriveProvidedInvariants`, `invariants`, `providedInvariants`, and `TEST_BASELINE_INVARIANT_ID` in the revised comment so readers can locate the relevant logic.
88-88: ⚡ Quick winRemove acceptance criteria code from describe block.
The describe block includes "(F23)" which is a milestone-named acceptance criteria. Remove it to comply with the coding guideline.
♻️ Proposed fix
- describe('db verify command - aggregate schema verification (F23)', () => { + describe('db verify command - aggregate schema verification', () => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/integration/test/cli.db-verify.aggregate-schema.test.ts` at line 88, The describe block title "db verify command - aggregate schema verification (F23)" contains milestone/acceptance criteria "(F23)"; update the test suite declaration by removing the "(F23)" suffix so the describe call reads "db verify command - aggregate schema verification". Locate the describe invocation in test/integration/test/cli.db-verify.aggregate-schema.test.ts (the describe(...) block) and edit the string literal passed to describe accordingly.
29-47: ⚡ Quick winRemove transient project artifact references from comments.
The docstring contains milestone-task IDs, acceptance criteria codes, and prose attributions that should be replaced with descriptions of the actual constraint or behavior. Per coding guidelines, comments must not reference transient artifacts like "F23 lock", "M2 R6 R1", "M2.5", or "sub-spec § 'Commit 6'".
♻️ Proposed fix to remove transient references
/** - * F23 lock — `db verify` against a multi-member aggregate (app + - * extension, both claiming live tables) returns zero schema issues. + * Verifies that `db verify` against a multi-member aggregate (app + + * extension, both claiming live tables) returns zero schema issues. * - * Pre-aggregate (M2 R6 R1), `db verify` projected the live schema only - * through the app contract. Tables claimed by extensions surfaced as + * Previously, `db verify` projected the live schema only through the + * app contract. Tables claimed by extensions surfaced as * `extras` and tripped lenient/strict schema diffs, polluting the - * verify output. The aggregate verifier (M2.5) pre-projects the live + * verify output. The aggregate verifier pre-projects the live * schema per member before running the family's schema-verify, so each * member only sees the elements it owns. * - * Setup mirrors the spec's intent (sub-spec § "Commit 6"): + * Setup: * - app contract claims `user` * - extension `test-contract-space` claims `test_box` * - both tables exist in the live DB and both markers match the * pinned contracts * * Expected: `db verify` exits 0 with `ok: true` and zero schema issues. */As per coding guidelines: "Source-code comments must not reference transient project artifacts including: milestone-task IDs (e.g. T1.7, T2.5.3), milestone-named acceptance criteria (e.g. AM12, AC-13), and prose attributions like 'sub-spec § 4'".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/integration/test/cli.db-verify.aggregate-schema.test.ts` around lines 29 - 47, The header docstring in the test for aggregate-schema verification contains transient project artifact references (e.g. "F23 lock", "M2 R6 R1", "M2.5", "sub-spec § 'Commit 6'"); update the comment block at the top of the test (the docstring describing the "db verify against a multi-member aggregate" scenario) to remove those IDs and prose attributions and instead state the concrete behavior and constraints in plain language — e.g. that the test verifies that an app contract claiming `user` and an extension claiming `test_box` (both present in the live DB with matching markers) result in `db verify` exiting 0 with `ok: true` and zero schema issues — while keeping the setup and expected outcome details concise.packages/3-extensions/cipherstash/test/scenario-a.e2e.integration.test.ts (2)
64-66: ⚡ Quick winUse
pathefor path helpers in this TS test.This file still imports
joinfromnode:path. The repo standard for TypeScript path manipulation ispathe, including tests and tooling.Minimal change
-import { join } from 'node:path'; +import { join } from 'pathe';Based on learnings: "In the Prisma-next repository, prefer importing and using 'pathe' over the built-in 'node:path' module for path operations across the codebase (including CLI commands and tooling files)."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/3-extensions/cipherstash/test/scenario-a.e2e.integration.test.ts` around lines 64 - 66, Replace the use of node:path's join import with pathe throughout the test file: locate the top-level import that currently reads import { join } from 'node:path' and change it to import { join } from 'pathe' (removing node:path from that import list), ensuring all subsequent uses of join in scenario-a.e2e.integration.test.ts continue to work with the pathe implementation; no other logic changes are required.
1-61: ⚡ Quick winDrop the milestone/spec IDs from the test commentary.
This header still hard-codes transient artifacts (
T3.6,AC-7,AM4,TML-2373,R3/R4,sub-spec). Please describe the behavior directly so the test stays durable after the rollout docs move on.As per coding guidelines: "Source-code comments must not reference transient project artifacts including: ... milestone-task IDs ... milestone-named acceptance criteria ..."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/3-extensions/cipherstash/test/scenario-a.e2e.integration.test.ts` around lines 1 - 61, The header comment contains transient milestone/spec identifiers (e.g., "T3.6", "AC-7", "AM4", "TML-2373", "R3/R4", "sub-spec") which must be removed; edit the block comment above the scenario so it describes the test behavior and goals in plain terms (what executeDbInit covers, the three coverage layers, why a synthetic bundle is used) without referencing milestone IDs, and keep references to implementation anchors like EQL_BUNDLE_SQL, buildSyntheticEqlBundleSql, SYNTHETIC_BUNDLE_RATIONALE and executeDbInit so readers can locate the related code and rationale.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@packages/3-extensions/cipherstash/README.md`:
- Around line 17-19: The README contains a stale claim that the codec runtime
and the `searchable: true` codec lifecycle hook are “not in this round”; remove
or revise that sentence to reflect the current implementation by stating that
the searchable codec lifecycle hook is wired up (see src/exports/control.ts) and
the codec runtime handles encoding/decoding of Encrypted<string> payloads;
update wording to be implementation-accurate and remove rollout-specific
language so the docs match the code (reference: the searchable lifecycle hook in
src/exports/control.ts and any codec runtime exports).
In `@packages/3-extensions/cipherstash/test/scenario-a.e2e.integration.test.ts`:
- Around line 179-209: The synthetic EQL bundle SQL in
buildSyntheticEqlBundleSql only creates EQL_V2_ENCRYPTED_TYPE and
EQL_V2_CONFIGURATION_TABLE plus add_search_config/remove_search_config, but must
mirror the full pinned CipherStash contract surface so contract-space
verification cannot be bypassed; update buildSyntheticEqlBundleSql to include
all contract-visible types/tables/functions present in the pinned contract (not
just the configuration table) and implement preserved structural ops as harmless
no-ops that keep identical schema shape and behavior. Specifically, expand the
SQL to create any additional types, tables, indexes, and functions that the real
contract exposes (matching names and column shapes used by the verifier), ensure
functions like add_search_config and remove_search_config remain but operate as
no-ops that maintain schema invariants, and preserve identifiers EQL_V2_SCHEMA,
EQL_V2_ENCRYPTED_TYPE, and EQL_V2_CONFIGURATION_TABLE so the synthesized DB
produces the same CIPHERSTASH_STORAGE_HASH as the real contract.
---
Nitpick comments:
In `@packages/3-extensions/cipherstash/test/scenario-a.e2e.integration.test.ts`:
- Around line 64-66: Replace the use of node:path's join import with pathe
throughout the test file: locate the top-level import that currently reads
import { join } from 'node:path' and change it to import { join } from 'pathe'
(removing node:path from that import list), ensuring all subsequent uses of join
in scenario-a.e2e.integration.test.ts continue to work with the pathe
implementation; no other logic changes are required.
- Around line 1-61: The header comment contains transient milestone/spec
identifiers (e.g., "T3.6", "AC-7", "AM4", "TML-2373", "R3/R4", "sub-spec") which
must be removed; edit the block comment above the scenario so it describes the
test behavior and goals in plain terms (what executeDbInit covers, the three
coverage layers, why a synthetic bundle is used) without referencing milestone
IDs, and keep references to implementation anchors like EQL_BUNDLE_SQL,
buildSyntheticEqlBundleSql, SYNTHETIC_BUNDLE_RATIONALE and executeDbInit so
readers can locate the related code and rationale.
In `@test/integration/test/cli.db-verify.aggregate-schema.test.ts`:
- Around line 59-64: Update the inline comment to remove the transient milestone
phrase "M2.5b loader integrity gate" and instead state the concrete requirement:
both the on-disk head ref's `invariants` and the migration package's
`providedInvariants` must round-trip through `deriveProvidedInvariants` and
produce the same array (e.g. `[TEST_BASELINE_INVARIANT_ID]` given the test
baseline op's `invariantId`), and that this derived value must match the stored
values for both the head ref and the migration metadata; reference the symbols
`deriveProvidedInvariants`, `invariants`, `providedInvariants`, and
`TEST_BASELINE_INVARIANT_ID` in the revised comment so readers can locate the
relevant logic.
- Line 88: The describe block title "db verify command - aggregate schema
verification (F23)" contains milestone/acceptance criteria "(F23)"; update the
test suite declaration by removing the "(F23)" suffix so the describe call reads
"db verify command - aggregate schema verification". Locate the describe
invocation in test/integration/test/cli.db-verify.aggregate-schema.test.ts (the
describe(...) block) and edit the string literal passed to describe accordingly.
- Around line 29-47: The header docstring in the test for aggregate-schema
verification contains transient project artifact references (e.g. "F23 lock",
"M2 R6 R1", "M2.5", "sub-spec § 'Commit 6'"); update the comment block at the
top of the test (the docstring describing the "db verify against a multi-member
aggregate" scenario) to remove those IDs and prose attributions and instead
state the concrete behavior and constraints in plain language — e.g. that the
test verifies that an app contract claiming `user` and an extension claiming
`test_box` (both present in the live DB with matching markers) result in `db
verify` exiting 0 with `ok: true` and zero schema issues — while keeping the
setup and expected outcome details concise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: ae1f8708-9faf-4c55-8b81-6cb4f8031887
📒 Files selected for processing (13)
packages/1-framework/3-tooling/migration/src/invariants.tspackages/1-framework/3-tooling/migration/src/io.tspackages/3-extensions/cipherstash/README.mdpackages/3-extensions/cipherstash/package.jsonpackages/3-extensions/cipherstash/src/core/cipherstash-codec.tspackages/3-extensions/cipherstash/src/core/constants.tspackages/3-extensions/cipherstash/src/core/contract.tspackages/3-extensions/cipherstash/src/core/eql-bundle.tspackages/3-extensions/cipherstash/src/core/migrations.tspackages/3-extensions/cipherstash/src/exports/control.tspackages/3-extensions/cipherstash/test/scenario-a.e2e.integration.test.tspackages/3-extensions/cipherstash/tsconfig.jsontest/integration/test/cli.db-verify.aggregate-schema.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/3-extensions/cipherstash/src/core/eql-bundle.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/1-framework/3-tooling/migration/src/invariants.ts
- packages/1-framework/3-tooling/migration/src/io.ts
- packages/3-extensions/cipherstash/src/core/constants.ts
- packages/3-extensions/cipherstash/src/core/contract.ts
- packages/3-extensions/cipherstash/src/core/migrations.ts
- packages/3-extensions/cipherstash/src/core/cipherstash-codec.ts
…adRefsBySpace) scenario-a.e2e.integration.test.ts called executePerSpaceDbApply (deleted by the M2.5 aggregate refactor) and am11-am12-no-descriptor.test.ts spelled VerifyContractSpacesInputs's head-ref map as pinnedHashesBySpace (renamed by M2.5b's drop-the-pinned-qualifier sweep). Migrate both: route the scenario-a apply path through executeDbInit (which encapsulates the loader → planner → runner pipeline and the additive-only policy that INIT_ADDITIVE_POLICY used to express in-line) and rename pinnedHashesBySpace → headRefsBySpace at both call sites. Resolves M3 PR #439 Type Check + Lint failures.
824fa40 to
4f6226d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/3-extensions/cipherstash/test/scenario-a.e2e.integration.test.ts (1)
1-62: 💤 Low valueReplace transient project artifact references with behavior descriptions.
The file header references internal milestone/spec artifacts that the coding guidelines prohibit in source comments:
sub-spec § 6 / T3.6(line 2)sub-spec § 6(line 4)AC-7(line 13)TC-14,AM4(lines 27-28)M3 R4 / TML-2373(line 57, 60)The PR already addressed this in other files (control.ts, contract.ts). Consider rewriting this header to describe the test's behavior directly without referencing transient artifacts.
As per coding guidelines: "Source-code comments must not reference transient project artifacts including: projects//... paths, milestone-task IDs (e.g. T1.7, T2.5.3, R4 design choice), milestone-named acceptance criteria (e.g. AM12, AC-13), and prose attributions like 'M2 review', 'out of scope', 'sub-spec § 4'"
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/3-extensions/cipherstash/test/scenario-a.e2e.integration.test.ts` around lines 1 - 62, Update the file header comment (the top block starting "Scenario A end-to-end against PGlite") to remove all transient project artifact references (e.g. "sub-spec § 6 / T3.6", "AC-7", "TC-14", "AM4", "M3 R4 / TML-2373") and instead describe the test behavior and intent in plain terms: state what the test exercises (db init flow via executeDbInit, planning vs apply paths, real vs synthetic bundle behavior, codec hook wiring, and what is validated by each path) and any scope/out-of-scope notes without milestone/task IDs or acceptance-code labels. Keep the same factual details (what is tested and why) but replace the named artifacts with descriptive phrases.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/3-extensions/cipherstash/test/am11-am12-no-descriptor.test.ts`:
- Around line 2-27: The top comment block that currently labels behavior with
"AM11 + AM12 regression" and the later comment block referencing those
milestone/sub-spec IDs should be rewritten to remove any transient milestone or
sub-spec labels and instead state the behaviors directly: describe that the
verifier helpers must succeed using only pinned migrations/cipherstash files
without importing a descriptor module, that the migrate-path helper must fail
with a clear error when the descriptor is missing, and that re-running
materialisation against an already emitted migrations/cipherstash/<dirName>/
must be a no-op (existence-only check, not overwrite). Apply the same label-free
rewrite to the duplicated comment around the later section (the block referenced
in the review).
---
Nitpick comments:
In `@packages/3-extensions/cipherstash/test/scenario-a.e2e.integration.test.ts`:
- Around line 1-62: Update the file header comment (the top block starting
"Scenario A end-to-end against PGlite") to remove all transient project artifact
references (e.g. "sub-spec § 6 / T3.6", "AC-7", "TC-14", "AM4", "M3 R4 /
TML-2373") and instead describe the test behavior and intent in plain terms:
state what the test exercises (db init flow via executeDbInit, planning vs apply
paths, real vs synthetic bundle behavior, codec hook wiring, and what is
validated by each path) and any scope/out-of-scope notes without milestone/task
IDs or acceptance-code labels. Keep the same factual details (what is tested and
why) but replace the named artifacts with descriptive phrases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 7f6116f9-ea53-44d8-8355-5c8492bf3f03
⛔ Files ignored due to path filters (4)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlprojects/extension-contract-spaces/plan.mdis excluded by!projects/**projects/extension-contract-spaces/specs/cipherstash-migration.spec.mdis excluded by!projects/**projects/extension-contract-spaces/specs/framework-mechanism.spec.mdis excluded by!projects/**
📒 Files selected for processing (28)
packages/1-framework/3-tooling/cli/src/utils/contract-space-extension-migrations-pass.tspackages/1-framework/3-tooling/migration/src/exports/io.tspackages/1-framework/3-tooling/migration/src/invariants.tspackages/1-framework/3-tooling/migration/src/io.tspackages/1-framework/3-tooling/migration/test/invariants.test.tspackages/1-framework/3-tooling/migration/test/materialise-extension-migration-package-if-missing.test.tspackages/3-extensions/cipherstash/README.mdpackages/3-extensions/cipherstash/biome.jsoncpackages/3-extensions/cipherstash/package.jsonpackages/3-extensions/cipherstash/src/core/cipherstash-codec.tspackages/3-extensions/cipherstash/src/core/constants.tspackages/3-extensions/cipherstash/src/core/contract.tspackages/3-extensions/cipherstash/src/core/eql-bundle.tspackages/3-extensions/cipherstash/src/core/eql-install.generated.tspackages/3-extensions/cipherstash/src/core/migrations.tspackages/3-extensions/cipherstash/src/exports/control.tspackages/3-extensions/cipherstash/test/am11-am12-no-descriptor.test.tspackages/3-extensions/cipherstash/test/cipherstash-codec.test.tspackages/3-extensions/cipherstash/test/descriptor.test.tspackages/3-extensions/cipherstash/test/scenario-a.e2e.integration.test.tspackages/3-extensions/cipherstash/test/scenario-c-bump.test.tspackages/3-extensions/cipherstash/tsconfig.jsonpackages/3-extensions/cipherstash/tsconfig.prod.jsonpackages/3-extensions/cipherstash/tsdown.config.tspackages/3-extensions/cipherstash/vitest.config.tstest/integration/package.jsontest/integration/test/cli.db-verify.aggregate-schema.test.tstest/integration/test/fixtures/cli/cli-e2e-test-app/package.json
💤 Files with no reviewable changes (2)
- test/integration/test/fixtures/cli/cli-e2e-test-app/package.json
- test/integration/package.json
✅ Files skipped from review due to trivial changes (10)
- packages/1-framework/3-tooling/migration/src/exports/io.ts
- packages/3-extensions/cipherstash/vitest.config.ts
- packages/3-extensions/cipherstash/tsdown.config.ts
- packages/3-extensions/cipherstash/tsconfig.prod.json
- packages/3-extensions/cipherstash/src/core/eql-bundle.ts
- packages/3-extensions/cipherstash/biome.jsonc
- packages/3-extensions/cipherstash/tsconfig.json
- packages/1-framework/3-tooling/migration/src/io.ts
- packages/3-extensions/cipherstash/package.json
- packages/1-framework/3-tooling/migration/test/materialise-extension-migration-package-if-missing.test.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- packages/3-extensions/cipherstash/src/exports/control.ts
- packages/1-framework/3-tooling/migration/src/invariants.ts
- packages/1-framework/3-tooling/migration/test/invariants.test.ts
- test/integration/test/cli.db-verify.aggregate-schema.test.ts
- packages/3-extensions/cipherstash/test/cipherstash-codec.test.ts
- packages/3-extensions/cipherstash/src/core/contract.ts
- packages/3-extensions/cipherstash/src/core/constants.ts
- packages/3-extensions/cipherstash/test/descriptor.test.ts
- packages/3-extensions/cipherstash/test/scenario-c-bump.test.ts
- packages/3-extensions/cipherstash/src/core/migrations.ts
- packages/1-framework/3-tooling/cli/src/utils/contract-space-extension-migrations-pass.ts
- packages/3-extensions/cipherstash/src/core/cipherstash-codec.ts
Mirrors `packages/3-extensions/test-contract-space/` (the contract-space
fixture) and `packages/3-extensions/pgvector/` (the production reference
shape) so the new `@prisma-next/extension-cipherstash` package boots
with the standard tsdown / tsconfig / vitest / biome layout. Public
package (not `private`); single `./control` export entry; deps mirror
the closest workspace neighbour (test-contract-space) so the contract
space the next slice adds can compose `@prisma-next/family-sql/control`,
`@prisma-next/migration-tools/{hash,spaces}`, and the framework
contract types without further dependency churn.
R1 (TML-2397, project: extension-contract-spaces) is the skeleton-in
for cipherstash on the per-space planner / runner / verifier landed in
M1 + M2; the codec runtime and codec lifecycle hook are intentionally
deferred to M3 R2 (T3.4).
Refs: TML-2397
Land cipherstash as the first real consumer of the framework
contract-space mechanism shipped in M1 + M2 of the
extension-contract-spaces project. The descriptor exposes
`contractSpace: { contractJson, migrations, headRef }` so the per-space
planner / runner / verifier manage cipherstash the same way they
manage an application contract — no new framework code needed.
Contract IR (T3.1, sub-spec § 2 — TC-13 partial PASS).
Declares `eql_v2_configuration` with `(id text PK, state
eql_v2_configuration_state, data jsonb)` — the only typed object the
current SQL `StorageBase` IR can model directly. The
composite/enum/domain types the spec ultimately wants in IR
(`eql_v2_encrypted`, `eql_v2_configuration_state`, ORE composites,
`eql_v2.bloom_filter`/`hmac_256`/`blake3` domains) are recorded on
`contract.meta.cipherstashFutureIR` as a documentary placeholder so
they remain reviewable in the emitted `contract.json` but do not
perturb the planner, which has no first-class enum / composite /
domain vocabulary yet. See R1 return report for the deferral rationale
— surfacing IR-vocabulary expansion before the cipherstash skeleton
would inflate scope significantly and is best done after R1 lands the
framework wiring.
Baseline migration (T3.2, sub-spec § 3 — TC-12 structural PASS, AC7
gated on real bundle SQL).
A single migration package `20260601T0000_install_eql_bundle/` carrying:
- `cipherstash:install-eql-bundle-v1` — body = the vendored EQL
bundle SQL string, pulled from `./eql-bundle` so swapping in the
real bundle byte-for-byte is a single-file change at one seam
(the seam itself is now contract-frozen). The R1 string is a
clearly-marked placeholder; the real bundle is owned by the
cipherstash team and not yet checked into this monorepo.
- `cipherstash:create-eql_v2_configuration_state-v1`,
`cipherstash:create-eql_v2_configuration-v1`,
`cipherstash:create-eql_v2_encrypted-v1`, three
`cipherstash:create-eql_v2_<domain>-v1` ops, two
`cipherstash:create-eql_v2_<ore-composite>-v1` ops — the structural
create-* ops the M3 sub-spec § 3 op list requires. Authored as
full SQL-family runtime ops (`{ target, precheck, execute,
postcheck }`) and stored in `MigrationOps` (the framework-level
base shape) per the same pattern shipped in M2 R5
(`db-apply-per-space.cli.test.ts` SQLite fixture).
Each op carries a stable `cipherstash:*` invariantId; the ids are
centralised in `core/constants.ts` so the head-ref derivation, the
migration metadata, and any downstream consumer reference the same
literal strings (project spec FR11 — invariantIds are immutable once
published).
Head ref + descriptor wiring (T3.3, sub-spec § 4 — partial AC1 / AC8
prep).
`contractSpace.headRef.hash` is derived from the same
`computeStorageHash(...)` the assembler runs at family-create time, so
`assertDescriptorSelfConsistency` is satisfied unconditionally (locked
in the new descriptor test). `contractSpace.headRef.invariants` is the
sorted-deduped union of the baseline ops `invariantId`s. The
descriptor intentionally omits `databaseDependencies` — cipherstash is
greenfield on contract spaces and never used the legacy mechanism
(spec FR13).
Verification.
- 8 descriptor tests in `test/descriptor.test.ts` cover descriptor
identity, contract-space wiring, deferred-IR record, baseline op
set + namespace, head ref alignment, and `assertDescriptorSelfConsistency`
success.
- `pnpm typecheck` PASS.
- `pnpm lint:deps` PASS (no layering violations, no
target-* leakage in 1-framework).
- `pnpm --filter @prisma-next/extension-cipherstash build` PASS
(tsdown emits a 16 kB ESM control entry).
- `pnpm test:packages` clean except for an unrelated
`@prisma-next/mongo-runtime` flake on the first run that passed on
rerun (mongo-memory-server resource contention; pre-existing per
`wip/unattended-decisions.md`).
Refs: TML-2397
…adRefsBySpace) scenario-a.e2e.integration.test.ts called executePerSpaceDbApply (deleted by the M2.5 aggregate refactor) and am11-am12-no-descriptor.test.ts spelled VerifyContractSpacesInputs's head-ref map as pinnedHashesBySpace (renamed by M2.5b's drop-the-pinned-qualifier sweep). Migrate both: route the scenario-a apply path through executeDbInit (which encapsulates the loader → planner → runner pipeline and the additive-only policy that INIT_ADDITIVE_POLICY used to express in-line) and rename pinnedHashesBySpace → headRefsBySpace at both call sites. Resolves M3 PR #439 Type Check + Lint failures.
Resolves the lint:manifests license-declaration check that flagged @prisma-next/extension-cipherstash as a publishable package missing a license field. Aligns with the rest of the @prisma-next/* publishable surface.
… all invariant ops The F23 aggregate-schema verify test wrote the test extension's pinned migration with providedInvariants: [] and dropped the head ref's invariants, on the (stale) assumption that deriveProvidedInvariants only counts data-class ops. M2.5b's loader integrity gate (PN-MIG-5002) re-derives providedInvariants from on-disk ops and rejects mismatch; M2.5's deriveProvidedInvariants counts every op carrying an invariantId, regardless of operationClass. The test extension's baseline op is additive but carries an invariantId, so the on-disk fixture must round-trip the same set. Fixture now writes the head ref invariants and migration providedInvariants matching the in-memory metadata, recomputing only the migrationHash (the in-memory placeholder hash isn't valid).
…ny fs entry pathExists() returned true for files and symlinks, causing materialiseExtensionMigrationPackageIfMissing to skip package creation when a stray file existed at the target path. Rename to directoryExists() and use stat.isDirectory() so only an existing directory triggers the idempotent skip. Also remove transient spec identifiers (AC-7, AM12, T1.7, spec path) from the docblock in favour of a direct description of the behavior.
…ariantId Checking Object.hasOwn() before reading op.invariantId ensures only own properties contribute to providedInvariants, preventing a prototype-inherited value from being incorrectly included.
…rify test A regression converting fails to warnings would silently pass. Assert both fail and warn counts are zero to fully lock schema health.
…age.json The package is subpath-only: all consumers use @prisma-next/extension-cipherstash/control. The exports map only exposes ./control, so the legacy main/module/types fields advertise a root entry that Node resolution would block under modern exports-first resolution. Remove the redundant fields to avoid the mismatch.
The base tsconfig already configures outDir to dist-tsc to avoid mixing tsc emit with tsdown build artifacts. Remove the explicit outDir: dist override so cipherstash follows the same convention as the rest of the repo.
… remove transient refs Restrict createDomain to (typeof EQL_V2_DOMAIN_TYPES)[number] and createOreComposite to (typeof EQL_V2_ORE_COMPOSITE_TYPES)[number] so callers get typo-safety from the known type-name unions. Also remove milestone/spec identifiers from docblocks (FR11, M3 R2, M3 R4, T3.4, sub-spec § 3) in favour of direct behavior descriptions.
Replace projects/extension-contract-spaces/ links and milestone references with stable text or removal per doc-maintenance rules. Keep only the durable reference fixture and reference shape links.
…sible codec events - Remove sub-spec section citations and milestone IDs from file headers in cipherstash-codec.ts, eql-bundle.ts, and contract.ts - Throw instead of silently returning [] when the field-event planner delivers an impossible payload (added without newField, dropped without priorField, altered without either) - Remove internal package path reference from expandNativeType comment
Replace M1+M2, FR2, FR10, FR13, NFR3, sub-spec § 4/§ 5 citations and test file references with direct behavior descriptions of what contractSpace, controlPlaneHooks, and databaseDependencies expose.
…text, state, data jsonb) The synthetic EQL bundle created eql_v2_configuration with an old column layout (id serial, "table", "column", index_name, cast_as). The contract defines it as (id text PRIMARY KEY, state text NOT NULL, data jsonb NOT NULL). Update buildSyntheticEqlBundleSql to use the contract-aligned schema, update add_search_config to store config in jsonb data, and update the test query to extract values from the jsonb field. Also remove transient test labels (T3.6, AC-7, sub-spec § 3, R2) from test names and comments.
…iants docblock
… and describe blocks
7a4bd25 to
d5da911
Compare
…adRefsBySpace) scenario-a.e2e.integration.test.ts called executePerSpaceDbApply (deleted by the M2.5 aggregate refactor) and am11-am12-no-descriptor.test.ts spelled VerifyContractSpacesInputs's head-ref map as pinnedHashesBySpace (renamed by M2.5b's drop-the-pinned-qualifier sweep). Migrate both: route the scenario-a apply path through executeDbInit (which encapsulates the loader → planner → runner pipeline and the additive-only policy that INIT_ADDITIVE_POLICY used to express in-line) and rename pinnedHashesBySpace → headRefsBySpace at both call sites. Resolves M3 PR #439 Type Check + Lint failures.
…adRefsBySpace) scenario-a.e2e.integration.test.ts called executePerSpaceDbApply (deleted by the M2.5 aggregate refactor) and am11-am12-no-descriptor.test.ts spelled VerifyContractSpacesInputs's head-ref map as pinnedHashesBySpace (renamed by M2.5b's drop-the-pinned-qualifier sweep). Migrate both: route the scenario-a apply path through executeDbInit (which encapsulates the loader → planner → runner pipeline and the additive-only policy that INIT_ADDITIVE_POLICY used to express in-line) and rename pinnedHashesBySpace → headRefsBySpace at both call sites. Resolves M3 PR #439 Type Check + Lint failures.
Summary
Milestone 3 of TML-2397 — Contract spaces. Stacked on top of #438 (M2).
Authors cipherstash as a contract space — the first real consumer of the M2 codec-hook + per-space-
db init/db updatesurface. Validates the M2 framework end-to-end against a real extension's contract IR, codec hooks, and migration pipeline.What lands (4 rounds, 19 commits)
M3 R1 — skeleton + contract-space authoring (3 commits)
d51293937package skeleton30aa9d30econtract-space authoring (T3.1, T3.2, T3.3) —eql_v2_configurationtable + composite/enum/domain types undercontract.meta.cipherstashFutureIR(IR vocabulary deferral; verifier symmetrically blind so AC-1 not blocked)85c299b7dplan.md statusM3 R2 — codec hook + bundle vendoring + AM11/AM12 regression (5 commits)
09fdd6877vendor real EQL bundle (eql-2.2.1) byte-for-byte fromtml-2373-project-1-part-228dec2649cipherstash:string@1codec lifecycle hook (T3.4) — first real consumer of M2'sonFieldEventplumbinge07b52a7ethree-layerdatabaseDependenciesabsence regression (T3.5)3b683e78cAM11 + AM12 cipherstash-level regression test (descriptor deletion + on-disk pinned-readability + idempotent re-materialisation)693516470plan.md statusM3 R3 — Scenario A end-to-end against PGlite (5 commits)
78fca4e97resolve structural-ops/EQL-bundle CREATE conflict via option (c) — bundle owns DDL; structural ops carry invariantIds with no-opSELECT 1bodies8a99f205aframework widening:deriveProvidedInvariantsincludes additive ops with invariantIds (sub-spec § 7.5 already prescribed this; cipherstash was the first real consumer to expose the gap)f188eac35codec gains identityexpandNativeTypeforEncrypted<String, { searchable: true }>columns4e3363c27T3.6 Scenario A e2e against PGlite — three tests covering AC-7 byte-equivalence on disk, multi-space planning against the real bundle, and multi-space apply against a synthetic-bundle variant03ba6e771plan.md statusM3 R4 — close-out (6 commits)
efd9d1aa2syncEQL_V2ORE composites + state enum values with vendored bundle (constants drift cleanup, time-sensitive pre-publication for FR11 invariantId immutability)5005e3328extractmaterialiseExtensionMigrationPackageIfMissingprimitive into@prisma-next/migration-tools/io(consolidates CLI + cipherstash AM12 test call sites)12748a2ecT3.7 Scenario C bump-diff (pure-fixture, AC-14)8cab8563d+9beee65ee+9b25e1283close-out doc-pass (sub-spec amendments + plan.md statuses)Architectural decisions
cipherstash:create-*-v1ops carry invariantIds with no-opSELECT 1bodies. Future-extension path (realprecheckSQL once IR vocabulary expands) documented inmigrations.ts.SqlStorageonly models tables + parameterised types today. Stubbed undercontract.meta.cipherstashFutureIRpending IR vocabulary expansion (post-M3 milestone candidate). Verifier is symmetrically blind on both sides — does NOT block AC-1.pgcrypto(the EQL bundle declaresCREATE EXTENSION IF NOT EXISTS pgcrypto). Apply leg of e2e uses a synthetic-bundle variant scoped to one op's SQL body in one test; tests 1+2 (byte-equivalence on disk + planning) use the real bundle. Adding a parallel pgcrypto-equipped Postgres test path here would import a heavyweight CI dependency for a single AC where framework-side properties are already locked end-to-end.AC promotions
Verification
pnpm lint:depsgreen (753 modules / 0 violations).pnpm typecheckgreen for cipherstash + migration-tools + cli.Test plan
pnpm --filter @prisma-next/extension-cipherstash test44/44 green.Refs: TML-2397.
Summary by CodeRabbit
New Features
Improvements
Tests
Documentation