merge dev to main (v3.0.0-beta.4) #249
Conversation
* feat: "create" access policy implementation * fix test * update
* feat(policy): support comparing `auth()` with auth model * fix file name
* chore(policy): more test cases and update * fix update regression * optimize nested relation manipulation
Co-authored-by: ymc9 <[email protected]>
* fix: add regression project and optimize ts schema generation * address review comments
WalkthroughBumps versions to 3.0.0-beta.4 across the monorepo, updates README/TODO, adds a regression test workspace, and introduces substantial runtime/policy engine changes: dialect import/visibility tweaks, CRUD nested-relation context handling, policy transformation/handler refactors, executor/plugin hook updates, and language validator/test enhancements. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Possibly related PRs
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Claude finished @ymc9's task —— View job Pull Request Review - v3.0.0-beta.4I've completed a comprehensive review of this release PR. Here's my detailed analysis: Review Checklist
✅ Code Quality & ArchitectureStrengths:
Minor Observations:
🔧 Key Improvements in This Release1. Policy Expression Handling -
|
There was a problem hiding this comment.
Pull Request Overview
This PR merges development branch dev to main, bringing a comprehensive set of improvements and optimizations to the ZenStack codebase. The changes focus on performance optimizations, bug fixes, and enhanced type safety across the entire system.
Key Changes:
- Performance optimizations for schema generation and runtime operations
- Enhanced error handling and validation improvements
- Type safety improvements and better code organization
- Version bump from 3.0.0-beta.3 to 3.0.0-beta.4
Reviewed Changes
Copilot reviewed 60 out of 61 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/runtime/src/client/crud/operations/base.ts | Enhanced relation handling and fixed parent update logic for improved data consistency |
| packages/runtime/src/plugins/policy/policy-handler.ts | Optimized policy evaluation for create operations by generating single SQL query for validation |
| packages/sdk/src/ts-schema-generator.ts | Implemented conditional ExpressionUtils import to reduce bundle size and improve performance |
| tests/regression/ | Added comprehensive regression test suite for issue 204 with generated schema and test files |
| packages/runtime/src/utils/type-utils.ts | Fixed WrapType utility to properly handle optional array types |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
packages/runtime/src/client/crud/dialects/postgresql.ts (2)
355-361: Bug: array_length requires a dimension (or use cardinality())Current SQL is invalid on Postgres. Provide dim 1 for 1-D arrays or switch to cardinality().
Apply:
- override buildArrayLength( - eb: ExpressionBuilder<any, any>, - array: Expression<unknown>, - ): ExpressionWrapper<any, any, number> { - return eb.fn('array_length', [array]); - } + override buildArrayLength( + eb: ExpressionBuilder<any, any>, + array: Expression<unknown>, + ): ExpressionWrapper<any, any, number> { + // Postgres needs a dimension; for 1-D arrays use 1 + return eb.fn('array_length', [array, sql.val(1)]); + // Alternatively: return eb.fn('cardinality', [array]); + }
362-368: Potential SQL injection/quoting bug in array literal constructionString elements aren’t escaped; single quotes in values will break SQL (and are injection-prone). At minimum, escape quotes; ideally, parameterize.
Safer minimal fix:
- override buildArrayLiteralSQL(values: unknown[]): string { - if (values.length === 0) { - return '{}'; - } else { - return `ARRAY[${values.map((v) => (typeof v === 'string' ? `'${v}'` : v))}]`; - } - } + override buildArrayLiteralSQL(values: unknown[]): string { + if (values.length === 0) return '{}'; + const toSql = (v: unknown) => + typeof v === 'string' ? `'${v.replace(/'/g, "''")}'` : v === null ? 'NULL' : String(v); + return `ARRAY[${values.map(toSql).join(', ')}]`; + }Optional follow-up (preferred): change this API to return a parameterized RawBuilder using
sql.join(values.map(sql.val)).packages/runtime/src/client/query-utils.ts (1)
10-14: MakehasModelconsistent with the new case-insensitivegetModel.Current implementation checks keys only;
getModelchecksm.name. Unify to avoid false negatives.export function hasModel(schema: SchemaDef, model: string) { - return Object.keys(schema.models) - .map((k) => k.toLowerCase()) - .includes(model.toLowerCase()); + return !!getModel(schema, model); }packages/runtime/src/client/crud/operations/base.ts (1)
824-860: Wrap JSON.parse for JSON defaults and audit other parsersWrap the JSON.parse calls in fillGeneratedAndDefaultValues to avoid throwing on malformed defaults; audit other JSON.parse sites for consistency.
Files to change:
- packages/runtime/src/client/crud/operations/base.ts — fillGeneratedAndDefaultValues (lines ~846–848)
- packages/runtime/src/client/result-processor.ts — review lines 38, 102, 199
Suggested minimal change:
if (fieldDef.type === 'Json') { // Schema uses JSON string for default value of Json fields if (fieldDef.array && Array.isArray(value)) { - value = value.map((v) => (typeof v === 'string' ? JSON.parse(v) : v)); + value = value.map((v) => { + if (typeof v === 'string') { + try { + return JSON.parse(v); + } catch { + return v; + } + } + return v; + }); } else if (typeof value === 'string') { - value = JSON.parse(value); + try { + value = JSON.parse(value); + } catch { + // keep original value on parse error + } } }packages/runtime/src/plugins/policy/expression-transformer.ts (1)
198-250: Bug: '^' (every) predicate currently behaves like 'none'You invert the predicate for '!' but not for '^', yet both branches compare count == 0. This makes '^' identical to '!', which is incorrect. For "every", the subquery should count rows NOT matching the predicate (i.e., invert inside the subquery).
Apply this minimal fix:
- if (expr.op === '!') { - predicateFilter = logicalNot(this.dialect, predicateFilter); - } + // For '^' (every): count of non-matching rows should be 0 -> invert predicate inside the subquery + if (expr.op === '^') { + predicateFilter = logicalNot(this.dialect, predicateFilter); + } const count = FunctionNode.create('count', [ValueNode.createImmediate(1)]); const predicateResult = match(expr.op) .with('?', () => BinaryOperationNode.create(count, OperatorNode.create('>'), ValueNode.createImmediate(0))) .with('!', () => BinaryOperationNode.create(count, OperatorNode.create('='), ValueNode.createImmediate(0))) .with('^', () => BinaryOperationNode.create(count, OperatorNode.create('='), ValueNode.createImmediate(0))) .exhaustive();
🧹 Nitpick comments (31)
packages/runtime/src/utils/type-utils.ts (1)
19-25: Rename generics to avoid shadowing and clarify intent; simplify conditional shape
- The generic names
OptionalandArrayshadow the exportedOptionaltype (Line 3) and the globalArraytype, which is confusing. PreferIsOptional/IsArrayand constrain them toboolean.- Restructure for readability while preserving behavior. Also makes precedence explicit.
Confirm that when both flags are true you intend “nullable container” (
T[] | null) rather than “nullable elements” ((T | null)[]). If element-nullability is needed in some call sites, consider adding a separate helper.-export type WrapType<T, Optional = false, Array = false> = Array extends true - ? Optional extends true - ? T[] | null - : T[] - : Optional extends true - ? T | null - : T; +export type WrapType< + T, + IsOptional extends boolean = false, + IsArray extends boolean = false +> = + IsOptional extends true + ? (IsArray extends true ? T[] : T) | null + : IsArray extends true + ? T[] + : T;packages/runtime/src/client/executor/name-mapper.ts (2)
251-253: Redundant alias truthiness inside an alias-present branchYou already checked scope.alias; keep only the IdentifierNode.name comparison (and after narrowing the type, even that guard is unnecessary).
- if (scope.alias && IdentifierNode.is(scope.alias) && scope.alias.name === qualifier) { + if (IdentifierNode.is(scope.alias) && scope.alias.name === qualifier) {Optionally, after typing Scope.alias as IdentifierNode:
- if (IdentifierNode.is(scope.alias) && scope.alias.name === qualifier) { + if (scope.alias?.name === qualifier) {
123-126: Qualifier match guard is correct; minor simplification possible after alias narrowingLogic is sound. Once Scope.alias is IdentifierNode, you can simplify the condition.
- if (scope.alias && IdentifierNode.is(scope.alias) && scope.alias.name === mappedTableName) { + if (scope.alias?.name === mappedTableName) {README.md (1)
24-24: Use a public-friendly Discord link and fix capitalizationConsider linking to the public server invite (as used in the badges above) instead of a channel URL that 404s for non-members, and capitalize “Discord”.
-> V3 is currently in beta phase and not ready for production use. Feedback and bug reports are greatly appreciated. Please visit this dedicated [discord channel](https://discord.com/channels/1035538056146595961/1352359627525718056) for chat and support. +> V3 is currently in beta and not ready for production use. Feedback and bug reports are greatly appreciated. Please visit our [Discord](https://discord.gg/Ykhr738dUe) for chat and support.TODO.md (1)
86-90: Track TODOs with issue links/owners to avoid drift.Consider adding GitHub issue links and DRI/acceptance notes for “Global omit” and “@default validation” to keep scope clear and auditable.
packages/sdk/src/model-utils.ts (1)
104-112: Clarify return type of getAuthDecl (now returns DataModel | TypeDef | undefined)Callers may have assumed DataModel only. Consider annotating the return type to make the contract explicit and ease adoption.
-export function getAuthDecl(model: Model) { +export function getAuthDecl(model: Model): DataModel | TypeDef | undefined {packages/runtime/src/client/plugin.ts (1)
182-187: Breaking change: onKyselyQuery now receives { client } instead of { kysely }Public plugin arg shape changed. Please ensure docs/CHANGELOG and all internal usages/tests are updated.
Also consider tightening the type of schema to the generic parameter:
-export type OnKyselyQueryArgs<Schema extends SchemaDef> = { - schema: SchemaDef; +export type OnKyselyQueryArgs<Schema extends SchemaDef> = { + schema: Schema; client: ClientContract<Schema>; query: RootOperationNode; proceed: ProceedKyselyQueryFunction; };packages/runtime/test/plugin/on-kysely-query.test.ts (1)
95-95: Avoid potential recursive re-entry/infinite post creationSpawning an insert inside onKyselyQuery for every InsertQueryNode can recurse for the Post insert too. Guard by table name so the follow-up insert only happens for User.
- async onKyselyQuery({ client, proceed, query }) { + async onKyselyQuery({ client, proceed, query }) { if (query.kind !== 'InsertQueryNode') { return proceed(query); } const result = await proceed(query); // create a post for the user - await proceed(createPost(client.$qb, result)); + const table = + (query as InsertQueryNode).into?.table.table ?? + (query as any).table?.table; // fallback for older nodes + if (table === 'User') { + await proceed(createPost(client.$qb, result)); + } return result; },Confirm executor semantics: do nested proceed calls re-enter the same interceptor? If yes, the guard is required; if no, documenting that behavior would help future contributors.
packages/runtime/test/policy/crud/dumb-rules.test.ts (2)
5-41: Close the client to avoid handle leaks.Add a disconnect at the end of the test to free DB connections.
await expect(db.c.create({ data: { x: 0 } })).resolves.toMatchObject({ x: 0 }); await expect(db.d.create({ data: { x: 0 } })).toBeRejectedByPolicy(); }); }); + +// inside the test body, after last assertion: +await db?.$disconnect?.();
11-34: Optionally assert read-path to reflect declared read rules.Since each model also declares @@Allow('read', true), add a quick read assertion to prove read-path isn’t unintentionally filtered.
await expect(db.a.create({ data: { x: 0 } })).resolves.toMatchObject({ x: 0 }); +await expect(db.a.findMany()).resolves.toEqual( + expect.arrayContaining([expect.objectContaining({ x: 0 })]) +);tests/regression/test/issue-204/regression.zmodel (1)
1-4: Use a test-local DB filename to avoid cross-regression interference.“dev.db” is generic and may clash if tests run in parallel. Prefer a unique file per regression or use env().
datasource db { provider = "sqlite" - url = "file:./dev.db" + url = "file:./issue-204.db" }packages/runtime/test/policy/auth-equality.test.ts (2)
17-24: Add an explicit read-allow rule for Post for symmetry.The equality case allows all on Post but doesn’t explicitly allow reads; adding it avoids surprises in read-backs and mirrors the inequality case.
model Post { id Int @id @default(autoincrement()) title String authorId1 Int authorId2 Int author User @relation(fields: [authorId1, authorId2], references: [id1, id2]) @@allow('all', auth() == author) + @@allow('read', true) }
28-61: Disconnect clients at test end to release resources.Wrap each test body with a finally block or add a trailing disconnect.
const db = await createPolicyTestClient(`...`); -// test ops... +try { + // test ops... +} finally { + await db?.$disconnect?.(); +}packages/runtime/test/policy/deep-nested.test.ts (1)
10-12: Confirm intended cascade direction after ownership flip.With M1.m2 referencing M2.id (onDelete: Cascade), deleting M2 will cascade to M1, not vice versa. The added tests later validate M2→M4/M3 cascades; consider also asserting M2→M1 cascade to lock in this contract.
await db.m2.delete({ where: { id: 1 } }); await expect(db.m4.findMany()).resolves.toHaveLength(0); +// M2 deletion should also cascade to M1 via onDelete: Cascade +await expect(db.m1.findUnique({ where: { myId: '1' } })).toResolveNull();Also applies to: 23-23
packages/language/src/validators/attribute-application-validator.ts (1)
178-237: Tighten owner-side detection to avoid false negativesCurrently a
@relationwith afieldsarg is treated as owner-side even iffieldsis malformed/empty. Guard against empty arrays to reduce misclassification.Apply:
- if (!relationAttr.args.some((arg) => arg.name === 'fields')) { + const fieldsArg = relationAttr.args.find((arg) => arg.name === 'fields'); + if (!fieldsArg || !isArrayExpr(fieldsArg.value) || fieldsArg.value.items.length === 0) { // no "fields" argument, can't be owner side of the relation, match return true; }Optional: extract an
isOwnerSideRelation(field: DataField)helper inutilsso runtime/lang stay consistent.tests/regression/tsconfig.json (1)
1-7: Add Vitest type definitions to avoid implicit any on globalsPrevents TS errors on
describe/it/expectin editors/CI.{ "extends": "@zenstackhq/typescript-config/base.json", "compilerOptions": { - "noEmit": true + "noEmit": true, + "types": ["vitest", "node"] }, - "include": ["src/**/*.ts", "test/**/*.ts"] + "include": ["src/**/*.ts", "test/**/*.ts", "vitest.config.ts"] }tests/regression/generate.ts (2)
4-4: Unused import: fsRemove to keep the file clean.
-import fs from 'node:fs';
21-21: Unused variable: tsPathIt’s never read. Drop it.
- const tsPath = path.join(outputDir, 'schema.ts');tests/regression/test/issue-204/regression.test.ts (1)
6-9: Add minimal assertions so the test actually verifies behaviorTurn logs into expectations.
-import { describe, it } from 'vitest'; +import { describe, it, expect } from 'vitest'; @@ - console.log(config.teamColors?.[0]); + expect(config.teamColors?.[0]).toBe(ShirtColor.Black); @@ - console.log(config1); + expect(config1).toEqual({});packages/runtime/src/client/errors.ts (1)
28-30: Set error name for better diagnostics; sanitize details upstreamSetting
this.nameimproves stack traces and filtering. Ensuredetailsnever contains sensitive data.export class NotFoundError extends Error { constructor(model: string, details?: string) { - super(`Entity not found for model "${model}"${details ? `: ${details}` : ''}`); + super(`Entity not found for model "${model}"${details ? `: ${details}` : ''}`); + this.name = 'NotFoundError'; } }tests/regression/package.json (1)
3-3: Version field on a private test workspaceOptional: set to 0.0.0 to avoid confusion with package versions.
- "version": "3.0.0-beta.3", + "version": "0.0.0",packages/language/test/utils.ts (1)
11-13: Clean up temp files to avoid leaking artifacts in CI.Ensure created schema files are removed even on failure.
export async function loadSchema(schema: string) { // create a temp file const tempFile = path.join(os.tmpdir(), `zenstack-schema-${randomUUID()}.zmodel`); - fs.writeFileSync(tempFile, schema); - const r = await loadDocument(tempFile, getPluginModels()); + try { + fs.writeFileSync(tempFile, schema); + const r = await loadDocument(tempFile, getPluginModels()); ... - invariant(r.success); - return r.model; + invariant(r.success); + return r.model; + } finally { + fs.rmSync(tempFile, { force: true }); + } } ... export async function loadSchemaWithError(schema: string, error: string | RegExp) { // create a temp file const tempFile = path.join(os.tmpdir(), `zenstack-schema-${randomUUID()}.zmodel`); - fs.writeFileSync(tempFile, schema); - const r = await loadDocument(tempFile, getPluginModels()); + try { + fs.writeFileSync(tempFile, schema); + const r = await loadDocument(tempFile, getPluginModels()); ... - if (typeof error === 'string') { + if (typeof error === 'string') { ... - } else { + } else { ... - } + } + } finally { + fs.rmSync(tempFile, { force: true }); + } }Also applies to: 24-26
packages/language/test/expression-validation.test.ts (2)
25-52: Differentiate test names to aid diagnostics.Two tests share the same description. Make them unique.
- it('should reject model comparison', async () => { + it('should reject model comparison (profile vs this)', async () => {
21-21: Stabilize string matching for error messages.Error text may change punctuation/casing. Prefer a regex with optional period to reduce brittleness.
- 'comparison between models is not supported', + /comparison between models is not supported\.?/i, ... - 'comparison between models is not supported', + /comparison between models is not supported\.?/i, ... - 'incompatible operand types', + /incompatible operand types/i,Also applies to: 50-51, 97-98
packages/runtime/src/client/query-utils.ts (1)
32-35: Optional: add a non-throwing field getter for types to mirrorrequireField.Many helpers use
getField(model-only). Consider agetFieldFromModelOrTypeto avoid sprinkling try/catch where type fields are acceptable.export function getField(schema: SchemaDef, model: string, field: string) { const modelDef = getModel(schema, model); return modelDef?.fields[field]; } +export function getFieldFromModelOrType(schema: SchemaDef, modelOrType: string, field: string) { + const m = getModel(schema, modelOrType); + if (m) return m.fields[field]; + const t = getTypeDef(schema, modelOrType) as any; + return t?.fields?.[field]; +}Update call sites that can accept either source accordingly.
Also applies to: 118-136
packages/runtime/src/plugins/policy/utils.ts (2)
53-58: Consider simplifying empty array handling.When the input array is empty, the function returns
trueNode(dialect)for conjunction, which is correct. However, the logic could be clearer by combining the empty and single-element cases.- if (nodes.length === 0) { - return trueNode(dialect); - } - if (nodes.length === 1) { - return nodes[0]!; - } + if (nodes.length <= 1) { + return nodes[0] ?? trueNode(dialect); + }
73-78: Consider simplifying empty array handling.Similar to the conjunction function, this could be simplified by combining the empty and single-element cases.
- if (nodes.length === 0) { - return falseNode(dialect); - } - if (nodes.length === 1) { - return nodes[0]!; - } + if (nodes.length <= 1) { + return nodes[0] ?? falseNode(dialect); + }packages/runtime/src/plugins/policy/expression-transformer.ts (2)
351-353: Qualify function arg field refs to avoid ambiguityUnqualified eb.ref(arg.field) can become ambiguous in nested/aliased contexts. Prefer qualifying with the current alias/model.
- if (ExpressionUtils.isField(arg)) { - return eb.ref(arg.field); - } + if (ExpressionUtils.isField(arg)) { + return eb.ref(`${context.alias ?? context.model}.${arg.field}`); + }
371-465: Member/this traversal — LGTM with one noteThe new “this as root” handling and backward relation folding are solid. Note: plain field must be the last segment — invariant is good.
Consider supporting multi-level auth member access (e.g., auth().profile.id). valueMemberAccess currently restricts to a single member; extending it would unlock more policies.
packages/runtime/src/plugins/policy/policy-handler.ts (2)
180-195: VALUES-derived const table: portability concernReferencing column1/column2 from a VALUES(...) source depends on dialect defaults. Some dialects require explicit column aliases (e.g., VALUES (...) AS t(c1, c2, ...)).
- If Kysely’s AliasNode supports column aliases, supply them to avoid relying on dialect-specific defaults.
- Otherwise, consider selecting from a projection with explicit aliasing per column.
Please run the policy create-path tests against all supported dialects to confirm portability.
196-215: COUNT case/style consistencyYou use FunctionNode.create('COUNT', ...) here but 'count' elsewhere. While most dialects are case-insensitive, keep function casing consistent.
- FunctionNode.create('COUNT', [ValueNode.createImmediate(1)]), + FunctionNode.create('count', [ValueNode.createImmediate(1)]),
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (60)
README.md(1 hunks)TODO.md(1 hunks)package.json(1 hunks)packages/cli/package.json(1 hunks)packages/common-helpers/package.json(1 hunks)packages/create-zenstack/package.json(1 hunks)packages/dialects/sql.js/package.json(1 hunks)packages/eslint-config/package.json(1 hunks)packages/language/package.json(2 hunks)packages/language/src/validators/attribute-application-validator.ts(3 hunks)packages/language/src/validators/expression-validator.ts(1 hunks)packages/language/test/expression-validation.test.ts(1 hunks)packages/language/test/utils.ts(2 hunks)packages/runtime/package.json(1 hunks)packages/runtime/src/client/crud/dialects/base-dialect.ts(3 hunks)packages/runtime/src/client/crud/dialects/index.ts(1 hunks)packages/runtime/src/client/crud/dialects/postgresql.ts(1 hunks)packages/runtime/src/client/crud/dialects/sqlite.ts(1 hunks)packages/runtime/src/client/crud/operations/base.ts(25 hunks)packages/runtime/src/client/crud/validator.ts(2 hunks)packages/runtime/src/client/errors.ts(1 hunks)packages/runtime/src/client/executor/kysely-utils.ts(1 hunks)packages/runtime/src/client/executor/name-mapper.ts(9 hunks)packages/runtime/src/client/executor/zenstack-query-executor.ts(0 hunks)packages/runtime/src/client/options.ts(1 hunks)packages/runtime/src/client/plugin.ts(1 hunks)packages/runtime/src/client/query-utils.ts(1 hunks)packages/runtime/src/plugins/policy/expression-transformer.ts(12 hunks)packages/runtime/src/plugins/policy/policy-handler.ts(6 hunks)packages/runtime/src/plugins/policy/utils.ts(2 hunks)packages/runtime/src/schema/expression.ts(1 hunks)packages/runtime/src/utils/type-utils.ts(1 hunks)packages/runtime/test/client-api/update.test.ts(2 hunks)packages/runtime/test/plugin/on-kysely-query.test.ts(1 hunks)packages/runtime/test/policy/auth-equality.test.ts(1 hunks)packages/runtime/test/policy/crud/create.test.ts(1 hunks)packages/runtime/test/policy/crud/dumb-rules.test.ts(1 hunks)packages/runtime/test/policy/crud/update.test.ts(1 hunks)packages/runtime/test/policy/deep-nested.test.ts(4 hunks)packages/runtime/test/scripts/generate.ts(0 hunks)packages/runtime/test/utils.ts(2 hunks)packages/sdk/package.json(1 hunks)packages/sdk/src/model-utils.ts(2 hunks)packages/sdk/src/ts-schema-generator.ts(10 hunks)packages/tanstack-query/package.json(1 hunks)packages/testtools/package.json(1 hunks)packages/typescript-config/package.json(1 hunks)packages/vitest-config/package.json(1 hunks)packages/zod/package.json(1 hunks)samples/blog/package.json(1 hunks)tests/e2e/package.json(1 hunks)tests/regression/generate.ts(1 hunks)tests/regression/package.json(1 hunks)tests/regression/test/issue-204/input.ts(1 hunks)tests/regression/test/issue-204/models.ts(1 hunks)tests/regression/test/issue-204/regression.test.ts(1 hunks)tests/regression/test/issue-204/regression.zmodel(1 hunks)tests/regression/test/issue-204/schema.ts(1 hunks)tests/regression/tsconfig.json(1 hunks)tests/regression/vitest.config.ts(1 hunks)
💤 Files with no reviewable changes (2)
- packages/runtime/test/scripts/generate.ts
- packages/runtime/src/client/executor/zenstack-query-executor.ts
🧰 Additional context used
📓 Path-based instructions (4)
package.json
📄 CodeRabbit inference engine (CLAUDE.md)
Pin the repository package manager to
[email protected]via thepackageManagerfield
Files:
package.json
{packages,samples,tests}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place packages only under
packages/,samples/, ortests/
Files:
packages/testtools/package.jsonpackages/dialects/sql.js/package.jsonpackages/runtime/test/plugin/on-kysely-query.test.tspackages/typescript-config/package.jsonpackages/runtime/test/policy/crud/dumb-rules.test.tstests/e2e/package.jsonpackages/sdk/package.jsontests/regression/tsconfig.jsonpackages/runtime/test/utils.tspackages/language/src/validators/expression-validator.tstests/regression/test/issue-204/regression.test.tspackages/cli/package.jsonpackages/eslint-config/package.jsonpackages/runtime/src/schema/expression.tspackages/runtime/test/policy/auth-equality.test.tspackages/runtime/src/client/plugin.tspackages/sdk/src/ts-schema-generator.tstests/regression/test/issue-204/input.tstests/regression/vitest.config.tstests/regression/package.jsonpackages/runtime/src/client/crud/dialects/sqlite.tspackages/runtime/test/policy/crud/update.test.tspackages/sdk/src/model-utils.tspackages/runtime/src/client/errors.tspackages/tanstack-query/package.jsonpackages/runtime/src/client/executor/kysely-utils.tspackages/runtime/src/client/crud/dialects/index.tspackages/zod/package.jsonpackages/create-zenstack/package.jsonpackages/runtime/src/client/crud/dialects/postgresql.tspackages/language/test/expression-validation.test.tspackages/runtime/src/client/options.tstests/regression/test/issue-204/schema.tspackages/vitest-config/package.jsontests/regression/test/issue-204/regression.zmodelpackages/runtime/src/client/crud/validator.tstests/regression/test/issue-204/models.tspackages/runtime/package.jsonpackages/runtime/src/utils/type-utils.tspackages/language/src/validators/attribute-application-validator.tspackages/common-helpers/package.jsonpackages/runtime/test/client-api/update.test.tssamples/blog/package.jsonpackages/language/test/utils.tspackages/runtime/src/client/crud/dialects/base-dialect.tspackages/runtime/src/plugins/policy/expression-transformer.tstests/regression/generate.tspackages/runtime/src/plugins/policy/utils.tspackages/runtime/src/client/query-utils.tspackages/language/package.jsonpackages/runtime/test/policy/crud/create.test.tspackages/runtime/src/client/executor/name-mapper.tspackages/runtime/src/client/crud/operations/base.tspackages/runtime/test/policy/deep-nested.test.tspackages/runtime/src/plugins/policy/policy-handler.ts
tests/e2e/**
📄 CodeRabbit inference engine (CLAUDE.md)
End-to-end tests must live under
tests/e2e/
Files:
tests/e2e/package.json
**/schema.ts
📄 CodeRabbit inference engine (CLAUDE.md)
The generated TypeScript schema should be named
schema.ts
Files:
tests/regression/test/issue-204/schema.ts
🧠 Learnings (1)
📚 Learning: 2025-09-04T12:38:14.150Z
Learnt from: CR
PR: zenstackhq/zenstack-v3#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-04T12:38:14.150Z
Learning: Applies to **/schema.ts : The generated TypeScript schema should be named `schema.ts`
Applied to files:
packages/sdk/src/ts-schema-generator.tstests/regression/test/issue-204/schema.tstests/regression/generate.ts
🧬 Code graph analysis (21)
packages/runtime/test/policy/crud/dumb-rules.test.ts (1)
packages/runtime/test/policy/utils.ts (1)
createPolicyTestClient(14-25)
packages/language/src/validators/expression-validator.ts (2)
packages/language/src/utils.ts (1)
isDataFieldReference(138-140)packages/language/src/generated/ast.ts (1)
isThisExpr(721-723)
tests/regression/test/issue-204/regression.test.ts (1)
tests/regression/test/issue-204/models.ts (3)
Configuration(11-11)ShirtColor(12-12)ShirtColor(13-13)
packages/runtime/src/schema/expression.ts (1)
packages/sdk/src/schema/expression.ts (1)
Expression(1-10)
packages/runtime/test/policy/auth-equality.test.ts (1)
packages/runtime/test/policy/utils.ts (1)
createPolicyTestClient(14-25)
packages/sdk/src/ts-schema-generator.ts (2)
packages/language/src/generated/ast.ts (11)
Expression(115-115)Expression(117-117)BinaryExpr(256-262)BinaryExpr(264-264)UnaryExpr(741-746)UnaryExpr(748-748)ArrayExpr(185-189)ArrayExpr(191-191)ReferenceExpr(688-693)ReferenceExpr(695-695)isDataField(344-346)packages/sdk/src/schema/expression.ts (2)
Expression(1-10)CallExpression(46-50)
packages/runtime/test/policy/crud/update.test.ts (1)
packages/runtime/test/policy/utils.ts (1)
createPolicyTestClient(14-25)
packages/sdk/src/model-utils.ts (1)
packages/language/src/generated/ast.ts (2)
isDataModel(391-393)isTypeDef(737-739)
packages/language/test/expression-validation.test.ts (1)
packages/language/test/utils.ts (2)
loadSchemaWithError(22-40)loadSchema(9-20)
tests/regression/test/issue-204/schema.ts (1)
packages/sdk/src/schema/schema.ts (1)
SchemaDef(10-18)
packages/runtime/src/client/crud/validator.ts (2)
packages/runtime/src/client/crud/operations/base.ts (1)
requireModel(114-116)packages/runtime/src/client/query-utils.ts (1)
requireModel(24-30)
packages/language/src/validators/attribute-application-validator.ts (2)
packages/language/src/generated/ast.ts (7)
Expression(115-115)Expression(117-117)isDataModel(391-393)DataField(333-340)DataField(342-342)BinaryExpr(256-262)BinaryExpr(264-264)packages/language/src/utils.ts (4)
isDataFieldReference(138-140)isRelationshipField(145-147)isAuthOrAuthMemberAccess(130-132)isCollectionPredicate(454-456)
packages/language/test/utils.ts (2)
packages/language/src/index.ts (1)
loadDocument(21-132)packages/common-helpers/src/tiny-invariant.ts (1)
invariant(4-14)
packages/runtime/src/plugins/policy/expression-transformer.ts (5)
packages/runtime/src/plugins/policy/utils.ts (2)
logicalNot(92-106)conjunction(49-67)packages/sdk/src/schema/expression.ts (1)
Expression(1-10)packages/runtime/src/client/errors.ts (1)
QueryError(13-17)packages/runtime/src/client/query-utils.ts (2)
getModel(16-18)requireField(37-55)packages/common-helpers/src/tiny-invariant.ts (1)
invariant(4-14)
tests/regression/generate.ts (2)
packages/sdk/src/ts-schema-generator.ts (1)
generate(57-71)packages/language/src/index.ts (1)
loadDocument(21-132)
packages/runtime/src/plugins/policy/utils.ts (1)
packages/sdk/src/schema/schema.ts (1)
SchemaDef(10-18)
packages/runtime/src/client/query-utils.ts (3)
packages/runtime/src/client/crud/operations/base.ts (5)
schema(91-93)requireModel(114-116)getModel(118-120)getField(126-128)requireField(122-124)packages/sdk/src/schema/schema.ts (1)
SchemaDef(10-18)packages/runtime/src/client/errors.ts (1)
QueryError(13-17)
packages/runtime/test/policy/crud/create.test.ts (1)
packages/runtime/test/policy/utils.ts (1)
createPolicyTestClient(14-25)
packages/runtime/src/client/executor/name-mapper.ts (2)
packages/runtime/src/client/crud/operations/base.ts (1)
requireModel(114-116)packages/runtime/src/client/query-utils.ts (1)
requireModel(24-30)
packages/runtime/src/client/crud/operations/base.ts (5)
packages/sdk/src/schema/schema.ts (4)
SchemaDef(10-18)GetModels(108-108)ModelDef(20-36)BuiltinType(84-94)packages/runtime/src/client/errors.ts (1)
NotFoundError(27-31)packages/common-helpers/src/tiny-invariant.ts (1)
invariant(4-14)packages/runtime/src/utils/object-utils.ts (1)
fieldsToSelectObject(11-13)packages/runtime/src/client/query-utils.ts (1)
getManyToManyRelation(238-274)
packages/runtime/src/plugins/policy/policy-handler.ts (4)
packages/runtime/src/client/query-utils.ts (1)
requireModel(24-30)packages/sdk/src/schema/schema.ts (1)
GetModels(108-108)packages/runtime/src/client/contract.ts (1)
CRUD(214-214)packages/runtime/src/plugins/policy/utils.ts (1)
falseNode(28-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-test (20.x)
- GitHub Check: claude-review
🔇 Additional comments (66)
packages/runtime/src/client/executor/name-mapper.ts (3)
361-366: LGTM: preserve model name as alias when mapped table name differsThis keeps user-facing field names stable while mapping physical table names. Good.
431-432: LGTM: preserve original field name via IdentifierNode aliasWrapping post-transform with the original field name avoids breaking consumers. Nice.
448-451: LGTM: selective aliasing during select-all expansionOnly alias when a mapped column name differs from the field name. Correct.
packages/zod/package.json (1)
3-3: Version bump looks goodAligned to 3.0.0-beta.4; no other changes.
packages/testtools/package.json (1)
3-3: Version bump looks goodConsistent with workspace release.
tests/e2e/package.json (1)
3-3: Version bump looks goodE2E package remains under tests/e2e as required.
packages/sdk/package.json (1)
3-3: Version bump looks goodNo API/exports changes; metadata-only.
packages/common-helpers/package.json (1)
3-3: Version bump looks goodMatches monorepo version alignment.
package.json (1)
3-3: Root version bump looks goodAlso confirms packageManager is pinned to [email protected] per guidelines.
packages/eslint-config/package.json (1)
3-3: Version bump looks goodPrivate config package metadata-only change.
packages/create-zenstack/package.json (1)
3-3: LGTM — metadata-only change.packages/cli/package.json (1)
6-6: LGTM — version bump only; no functional changes here.packages/runtime/test/utils.ts (1)
6-6: Type-only import for LogEvent is appropriate.No runtime cost added; matches usage.
packages/typescript-config/package.json (1)
3-3: LGTM — metadata-only bump.samples/blog/package.json (1)
3-3: LGTM — sample version aligned with release.packages/tanstack-query/package.json (1)
3-3: LGTM — version bump only.packages/dialects/sql.js/package.json (1)
3-3: Version bump looks goodManifest-only change under packages/ is aligned with repo guidelines. No concerns.
packages/vitest-config/package.json (1)
4-4: Version bump OKMatches the monorepo-wide beta.4 update. Nothing else changed.
packages/sdk/src/model-utils.ts (1)
5-5: Importing isTypeDef is appropriateNeeded for updated @@auth lookup across DataModel and TypeDef. Looks correct.
tests/regression/vitest.config.ts (1)
1-4: Minimal Vitest config is fineMerges base without overrides; good for a new regression workspace. No action.
packages/runtime/src/client/plugin.ts (1)
2-2: Type-only import of ClientContractType-only usage avoids runtime cycles. Looks good.
packages/runtime/test/plugin/on-kysely-query.test.ts (1)
87-87: Test updated to use client in hook argsSignature change reflected correctly.
packages/runtime/src/client/crud/validator.ts (2)
25-33: Import cleanup and centralization via requireModelRemoving QueryError import and relying on requireModel/requireField/contracts simplifies error handling. Good direction.
281-283: Model resolution switched to requireModelBehavior now depends on requireModel (likely case-insensitive lookup and standardized error). Please verify error messages/types expected by callers/tests; slight wording changes can break assertions.
packages/language/package.json (2)
4-4: Version bump OKAligns with monorepo beta.4.
63-70: New dev dependencies look appropriate@types/tmp, glob, and common-helpers are dev-only; no runtime surface impact. Ensure glob usage remains in tests/tooling only.
tests/regression/test/issue-204/regression.zmodel (1)
14-16: Confirm optional list support in composite types.
ShirtColor[]?(optional list) inside a compositetypeserialized via@jsoncan be dialect/tooling-sensitive. Ensure your generator/runtime version supports optional lists for composite fields; otherwise model as a required list with default empty array in code.Would you like me to cross-check your current generator/runtime version for this specific syntax?
packages/runtime/test/client-api/update.test.ts (2)
1818-1831: LGTM: correct no-op semantics when disconnect filter doesn’t match.The assertion that the existing connection remains intact is precise and guards against accidental disconnects.
1847-1856: LGTM: clarified test name matches behavior.Renaming to “not connected, no-op” improves intent without altering behavior.
packages/runtime/test/policy/deep-nested.test.ts (2)
619-633: Good: setting M2.value = 3 ensures delete is permitted by policy.This aligns with
@@allow('delete', value > 2)and makes the subsequent cascade checks meaningful.
636-641: Nice coverage: verifies no unintended cascades on deleting M1.Asserting M2/M4 counts after M1 deletion helps prevent regressions on cascade direction.
packages/runtime/test/policy/crud/create.test.ts (2)
4-25: Good coverage of create policy basicsSolid positive/negative cases for scalar checks and raw QB paths.
115-135: Error message assertion aligns with new validatorExpectation matches the new “non-owned relation” check; helps prevent regressions.
packages/language/src/validators/attribute-application-validator.ts (1)
171-176: Create/all policy hooks: new non-owned relation guard is correctly integratedThe check runs only for
create/alland when an expression exists. Good placement and scope.packages/runtime/src/client/crud/dialects/index.ts (1)
4-15: Re-export BaseCrudDialect and verify no remaining './base' importsAdd a re-export to simplify downstream imports; confirm there are no stale
./baseimports in the repo.import type { BaseCrudDialect } from './base-dialect'; @@ export function getCrudDialect<Schema extends SchemaDef>( @@ } + +export type { BaseCrudDialect } from './base-dialect';packages/runtime/src/client/crud/dialects/sqlite.ts (1)
23-26: Import path update is consistent with the refactorNo functional changes; consistent with Postgres dialect changes.
packages/runtime/src/client/crud/dialects/postgresql.ts (1)
23-23: Import path rename to base-dialect looks goodNo behavior change; consistent with other dialects.
packages/runtime/src/client/options.ts (1)
5-5: Import path update aligns with dialect refactorType-only import to base-dialect is correct; no runtime impact.
packages/language/src/validators/expression-validator.ts (1)
210-216: Clearer, consistent error textMessage change to “comparison between models is not supported” matches test intent; logic unchanged.
packages/runtime/src/schema/expression.ts (1)
91-94: New ExpressionUtils.not helper — good additionKeeps builder API symmetrical with and/or/unary.
tests/regression/test/issue-204/schema.ts (1)
1-60: LGTM: generated schema shape matchesSchemaDef, file name complies with guideline.Provider, models, typeDefs, and enums align with runtime expectations;
schema.tsnaming meets the rule for generated schemas.packages/runtime/src/client/query-utils.ts (1)
16-18: Add unit tests for case-sensitivity and model/type precedence (query-utils)getModel lowercases names (case-insensitive); getTypeDef is a direct lookup (case-sensitive); requireField prefers model over type. Add tests for:
- model exists under a different-cased name -> returns the model
- type exists under a different-cased name -> does NOT return the type (assert current, case-sensitive behavior)
- both absent -> requireModel/requireField throws QueryError
- both present with same name -> requireField resolves to the model (model precedence)
Target: packages/runtime/src/client/query-utils.ts (getModel/getTypeDef/requireField). I can draft focused tests if none exist.
tests/regression/test/issue-204/models.ts (1)
8-13: LGTM: generated model/type wrappers and enum export look correct.Type/value dual export for
ShirtColoris idiomatic and matches the generated schema.packages/runtime/src/plugins/policy/utils.ts (1)
108-110: LGTM! Clean utility function.The
wrapParensIfhelper is well-designed - it encapsulates the conditional parentheses logic nicely and improves code readability throughout the file.packages/runtime/src/client/crud/dialects/base-dialect.ts (1)
1107-1131: Visibility change looks good.Making
and,or, andnotmethods public is a sensible API change that allows external consumers to use these logical combinators. The implementation remains unchanged and well-tested.packages/sdk/src/ts-schema-generator.ts (4)
55-61: Good practice: Resetting the flag for each generation.Resetting
usedExpressionUtilsat the start of eachgenerate()call ensures clean state between generations, preventing cross-contamination.
110-118: Efficient conditional import generation.The conditional inclusion of
ExpressionUtilsin the import statement based on actual usage is a good optimization that reduces unnecessary imports in the generated code.
154-161: Well-structured helper method.The
createExpressionUtilsCallhelper centralizes the creation of ExpressionUtils calls and properly manages the usage flag. This is a clean abstraction.
841-855: Good handling of JSON defaults.The special handling for JSON field defaults, including parsing stringified JSON values and arrays, ensures proper data transformation.
packages/runtime/test/policy/crud/update.test.ts (1)
1-585: Comprehensive test coverage for update policies!The test suite thoroughly covers various update scenarios including scalar conditions, relation conditions, nested updates, and relation manipulations. The tests are well-structured with clear separation of concerns and good use of the testing harness.
packages/runtime/src/client/crud/operations/base.ts (6)
68-78: Clean context type definition.The
FromRelationContexttype is well-structured with clear field documentation. TheparentUpdatesfield provides a clean way to accumulate parent entity updates during nested relation processing.
289-294: Good refactor: Context-driven parent updates.The shift from immediate parent updates to recording them in
parentUpdatesfor later application is a cleaner approach that better handles complex nested operations.
560-563: Improved error messages for relation operations.The enhanced error message that includes relation context helps developers quickly identify which relation operation failed.
1034-1037: Clean propagation of parent updates.The merging of
parentUpdatesfrom nested relation processing into the main update fields is handled cleanly.
1589-1595: Good validation of connect operations.The validation ensures that all requested entities were successfully connected, providing proper error handling for missing entities.
1683-1716: Complex but necessary disconnect logic.The disconnect logic for owned relations properly handles both unconditional disconnects and filtered disconnects, with appropriate parent update recording.
packages/runtime/src/plugins/policy/expression-transformer.ts (5)
83-88: Safer authType access — LGTMRuntime guard + non-null assertion is appropriate here.
113-128: Field access simplification — LGTMRouting non-relational fields through a qualified column ref and delegating relations to transformRelationAccess reads cleaner and avoids stale thisEntity caches.
252-296: Auth comparisons over model IDs — LGTMClearer error messages, proper handling for model auth types, and composite-id support via AND folding look correct.
482-522: Correlated relation subquery join condition — LGTMFK/PK qualification via context.alias/fromModel is correct for both ownership directions.
Please validate a deep nested path (this.a.b.c.d) on composite FKs across all supported dialects to ensure alias resolution remains correct when composing multiple SelectQueryNodes.
540-552: Helper combinators — LGTMCentralizing boolean expression construction reduces duplication and improves readability.
packages/runtime/src/plugins/policy/policy-handler.ts (4)
147-158: Graceful handling of missing columns/values — LGTMDeriving fields/rows when node.values is absent makes pre-create checks robust for INSERT DEFAULT VALUES and similar cases.
167-178: Field/value alignment relies on object key orderallFields/order is derived from Object.keys(requireModel(...).fields). JS preserves insertion order, but ensure schema.field emission is deterministic across codegen/runtime; otherwise columnN-to-field mapping might drift.
Add a unit test that randomizes field insertion order in the schema object and asserts pre-create policy evaluation still maps values to the correct fields.
364-398: Policy combination logic — LGTMor(allows) && and(!denies) with false fallback when no allows matches the intended semantics and aligns with the new transformer.
484-499: Transformer invocation simplification — LGTMRemoval of thisEntity/thisEntityRaw and model/alias-centric invocation matches the refactored transformer API.
tests/regression/test/issue-204/input.ts (1)
1-30: Auto-generated types for Foo — LGTMType exports look complete and consistent with runtime generics.
| id Int @id | ||
| title String | ||
| author User @relation(fields: [authorId], references: [id]) | ||
| @@allow('all', author == this) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Relation requires the scalar FK field authorId.
Without authorId Int, schema validation may fail before reaching the expression validator, making the test flaky for the intended error.
model Post {
id Int @id
title String
+ authorId Int
author User @relation(fields: [authorId], references: [id])
@@allow('all', author == this)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| id Int @id | |
| title String | |
| author User @relation(fields: [authorId], references: [id]) | |
| @@allow('all', author == this) | |
| } | |
| model Post { | |
| id Int @id | |
| title String | |
| authorId Int | |
| author User @relation(fields: [authorId], references: [id]) | |
| @@allow('all', author == this) | |
| } |
🤖 Prompt for AI Agents
In packages/language/test/expression-validation.test.ts around lines 15 to 19,
the User relation is defined without the scalar foreign-key field authorId,
which causes schema validation to fail before the expression validator runs; add
a scalar field authorId Int to the model (and ensure it's included in the model
definition used by the test) and update any references if needed so the relation
uses fields: [authorId], references: [id] and the test will reliably reach the
expression validator.
|
|
||
| export async function loadSchema(schema: string) { | ||
| // create a temp file | ||
| const tempFile = path.join(os.tmpdir(), `zenstack-schema-${crypto.randomUUID()}.zmodel`); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid relying on global crypto; import randomUUID explicitly.
Using the global crypto can break TS builds unless DOM lib is enabled. Import from node:crypto and call it directly.
+import { randomUUID } from 'node:crypto';
...
- const tempFile = path.join(os.tmpdir(), `zenstack-schema-${crypto.randomUUID()}.zmodel`);
+ const tempFile = path.join(os.tmpdir(), `zenstack-schema-${randomUUID()}.zmodel`);
...
- const tempFile = path.join(os.tmpdir(), `zenstack-schema-${crypto.randomUUID()}.zmodel`);
+ const tempFile = path.join(os.tmpdir(), `zenstack-schema-${randomUUID()}.zmodel`);Also applies to: 24-24
🤖 Prompt for AI Agents
In packages/language/test/utils.ts around lines 11 and 24, the code uses the
global crypto.randomUUID which can break TS builds without the DOM lib; import
the function explicitly from Node and replace uses accordingly: add an import
like `import { randomUUID } from 'node:crypto';` at the top of the file and
change `crypto.randomUUID()` to `randomUUID()` at both locations (lines 11 and
24).
| expect(r).toSatisfy( | ||
| (r) => r.success, | ||
| `Failed to load schema: ${(r as any).errors?.map((e) => e.toString()).join(', ')}`, | ||
| ); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
toSatisfy is not a built-in Vitest matcher—verify setup or use a portable assertion.
Unless you’ve configured extended matchers, these will error at runtime. Either ensure the matcher is registered or switch to a portable pattern.
Option A (keep): Confirm that a matcher like vitest-extended is installed and registered in test setup.
Option B (portable):
- expect(r).toSatisfy(
- (r) => r.success,
- `Failed to load schema: ${(r as any).errors?.map((e) => e.toString()).join(', ')}`,
- );
+ if (!r.success) {
+ expect.fail(`Failed to load schema: ${r.errors.map((e) => e.toString()).join(', ')}`);
+ }
...
- expect(r).toSatisfy(
- (r) => r.errors.some((e) => e.toString().toLowerCase().includes(error.toLowerCase())),
- `Expected error message to include "${error}" but got: ${r.errors.map((e) => e.toString()).join(', ')}`,
- );
+ expect(
+ r.errors.some((e) => e.toString().toLowerCase().includes(error.toLowerCase())),
+ `Expected error message to include "${error}" but got: ${r.errors.map((e) => e.toString()).join(', ')}`
+ ).toBe(true);
...
- expect(r).toSatisfy(
- (r) => r.errors.some((e) => error.test(e)),
- `Expected error message to match "${error}" but got: ${r.errors.map((e) => e.toString()).join(', ')}`,
- );
+ expect(
+ r.errors.some((e) => error.test(e)),
+ `Expected error message to match "${error}" but got: ${r.errors.map((e) => e.toString()).join(', ')}`
+ ).toBe(true);Run to verify matcher availability and setup:
Also applies to: 30-38
🏁 Script executed:
#!/bin/bash
# Check for extended matcher deps and setup
rg -n "toSatisfy\\(" -g "packages/**/test/**.ts"
fd package.json | xargs -I{} jq -r '.devDependencies // {} | keys[]' {} | rg -n "vitest-extended|jest-extended" -n
fd "*.setup.*" -a | xargs rg -n "expect\\.extend|toSatisfy"Length of output: 11855
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- packages/runtime/test/vitest-ext.ts (first 240 lines) ----"
if [ -f packages/runtime/test/vitest-ext.ts ]; then sed -n '1,240p' packages/runtime/test/vitest-ext.ts || true; else echo "MISSING: packages/runtime/test/vitest-ext.ts"; fi
echo
echo "---- packages/language/test/utils.ts (first 240 lines) ----"
if [ -f packages/language/test/utils.ts ]; then sed -n '1,240p' packages/language/test/utils.ts || true; else echo "MISSING: packages/language/test/utils.ts"; fi
echo
echo "---- packages/language/package.json ----"
if [ -f packages/language/package.json ]; then sed -n '1,240p' packages/language/package.json || true; else echo "MISSING: packages/language/package.json"; fi
echo
echo "---- packages/vitest-config/package.json ----"
if [ -f packages/vitest-config/package.json ]; then sed -n '1,240p' packages/vitest-config/package.json || true; else echo "MISSING: packages/vitest-config/package.json"; fi
echo
echo "---- repo: files that register expect.extend ----"
rg -n --hidden --glob '!node_modules' "expect\\.extend" || true
echo
echo "---- repo: occurrences of toSatisfy ----"
rg -n --hidden --glob '!node_modules' "toSatisfy\\(" || true
echo
echo "---- repo: vitest setupFiles / setupFilesAfterEnv mentions ----"
rg -n --hidden --glob '!node_modules' "setupFiles|setupFilesAfterEnv" || trueLength of output: 8312
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- root package.json ----"
if [ -f package.json ]; then sed -n '1,240p' package.json || true; else echo "MISSING: package.json"; fi
echo
echo "---- packages/vitest-config/base.config.js ----"
if [ -f packages/vitest-config/base.config.js ]; then sed -n '1,240p' packages/vitest-config/base.config.js || true; else echo "MISSING: packages/vitest-config/base.config.js"; fi
echo
echo "---- Search for toSatisfy occurrences (raw) ----"
rg -n -uu "toSatisfy" || true
echo
echo "---- Search for toSatisfy matcher definition patterns ----"
rg -n -uu "toSatisfy\\s*\\(|toSatisfy\\s*:" || true
echo
echo "---- Search for expect.extend registrations ----"
rg -n -uu "expect\\.extend" || true
echo
echo "---- Search for vitest-extended / jest-extended mentions ----"
rg -n -uu "vitest-extended|jest-extended" || true
echo
echo "---- Top of a representative test that uses toSatisfy ----"
if [ -f packages/runtime/test/client-api/default-values.test.ts ]; then sed -n '1,120p' packages/runtime/test/client-api/default-values.test.ts; else echo "MISSING: packages/runtime/test/client-api/default-values.test.ts"; fi
echo
echo "---- packages/runtime/test/vitest-ext.ts (full) ----"
if [ -f packages/runtime/test/vitest-ext.ts ]; then sed -n '1,240p' packages/runtime/test/vitest-ext.ts || true; else echo "MISSING: packages/runtime/test/vitest-ext.ts"; fi
echoLength of output: 13185
toSatisfy is not registered — register the matcher or replace usages with portable assertions
Repo search: no vitest-extended/jest-extended in package.json and the only local expect.extend is packages/runtime/test/vitest-ext.ts (it does not add toSatisfy). Tests that call toSatisfy will fail at runtime.
Locations: packages/language/test/utils.ts (lines 14–17, 30–38); also used in packages/runtime/test/client-api/{default-values.test.ts, update.test.ts, delegate.test.ts}.
Options:
- Option A (keep): Add and register an extended matcher that provides toSatisfy (e.g., add vitest-extended or jest-extended to devDependencies and import it from a Vitest setup file, or implement a central expect.extend that defines toSatisfy and register that setup in your Vitest config).
- Option B (portable): Replace toSatisfy uses with portable assertions — example diff:
- expect(r).toSatisfy(
- (r) => r.success,
- `Failed to load schema: ${(r as any).errors?.map((e) => e.toString()).join(', ')}`,
- );
+ if (!r.success) {
+ expect.fail(`Failed to load schema: ${r.errors.map((e) => e.toString()).join(', ')}`);
+ }
...
- expect(r).toSatisfy(
- (r) => r.errors.some((e) => e.toString().toLowerCase().includes(error.toLowerCase())),
- `Expected error message to include "${error}" but got: ${r.errors.map((e) => e.toString()).join(', ')}`,
- );
+ expect(
+ r.errors.some((e) => e.toString().toLowerCase().includes(error.toLowerCase())),
+ `Expected error message to include "${error}" but got: ${r.errors.map((e) => e.toString()).join(', ')}`
+ ).toBe(true);
...
- expect(r).toSatisfy(
- (r) => r.errors.some((e) => error.test(e)),
- `Expected error message to match "${error}" but got: ${r.errors.map((e) => e.toString()).join(', ')}`,
- );
+ expect(
+ r.errors.some((e) => error.test(e)),
+ `Expected error message to match "${error}" but got: ${r.errors.map((e) => e.toString()).join(', ')}`
+ ).toBe(true);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/language/test/utils.ts around lines 14–17 (and other usages in lines
30–38 and runtime tests), tests call expect(...).toSatisfy but that matcher is
not registered — tests will fail at runtime; fix by either registering an
extended matcher or replacing usages with portable assertions: Option A — add a
devDependency that provides toSatisfy (e.g., vitest-extended or jest-extended),
import/require it in the Vitest setup file or add an expect.extend
implementation in the shared test setup and register that file in vitest.config;
Option B — replace each toSatisfy call with a portable assertion such as
asserting the boolean success property directly (e.g.,
expect(r.success).toBe(true) or expect(r.success).toBeTruthy()) and include the
existing error message interpolation so failures still show detailed errors.
| function getPluginModels() { | ||
| return glob.sync(path.resolve(__dirname, '../../runtime/src/plugins/**/plugin.zmodel')); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make __dirname ESM-safe for globbing plugin models.
Tests may run under ESM where __dirname is undefined. Mirror the approach used in loadDocument.
+import { fileURLToPath } from 'node:url';
...
function getPluginModels() {
- return glob.sync(path.resolve(__dirname, '../../runtime/src/plugins/**/plugin.zmodel'));
+ const _dirname =
+ typeof __dirname !== 'undefined' ? __dirname : path.dirname(fileURLToPath(import.meta.url));
+ return glob.sync(path.resolve(_dirname, '../../runtime/src/plugins/**/plugin.zmodel'));
}Also applies to: 1-3
🤖 Prompt for AI Agents
In packages/language/test/utils.ts around lines 41-43, the glob call uses
__dirname which breaks under ESM; mirror the loadDocument approach by deriving a
safe dirname (use fileURLToPath(new URL('.', import.meta.url)) when __dirname is
undefined or fallback to existing __dirname), import fileURLToPath/URL from
'url' if needed, then use that derived directory in the path.resolve call for
the glob so tests work in both CJS and ESM environments.
| { | ||
| "name": "@zenstackhq/runtime", | ||
| "version": "3.0.0-beta.3", | ||
| "version": "3.0.0-beta.4", |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Version bump to 3.0.0-beta.4 — LGTM.
Looks consistent with the monorepo release.
Run to verify all package versions are aligned and no stale beta.3 refs remain:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
TARGET="3.0.0-beta.4"
echo "Root version:" && jq -r '.version' package.json
echo -e "\nPackages with mismatched versions:"
fd -HI '^package\.json$' packages tests samples . \
| while read -r f; do
v=$(jq -r '.version // empty' "$f")
if [[ -n "$v" && "$v" != "$TARGET" ]]; then
echo "$f -> $v"
fi
done
echo -e "\nOccurrences of previous beta.3:"
rg -n "3\.0\.0-beta\.3" || trueLength of output: 123
🏁 Script executed:
#!/bin/bash
set -euo pipefail
TARGET="3.0.0-beta.4"
echo "Root version:"
jq -r '.version // empty' package.json || true
echo
echo "Packages with mismatched versions:"
fd -HI --type f -g 'package.json' . | while IFS= read -r f; do
v=$(jq -r '.version // empty' "$f" 2>/dev/null || true)
if [ -n "$v" ] && [ "$v" != "$TARGET" ]; then
printf '%s -> %s\n' "$f" "$v"
fi
done
echo
echo "Occurrences of previous beta.3:"
rg -n --hidden --no-ignore-vcs "3\.0\.0-beta\.3" || trueLength of output: 11721
Action required — leftover version mismatches found
- tests/regression/package.json -> "version": "3.0.0-beta.3" — update to 3.0.0-beta.4.
- packages/ide/vscode/package.json -> "version": "3.0.6" — confirm if this is intentional or should be aligned to 3.0.0-beta.4.
🤖 Prompt for AI Agents
In packages/runtime/package.json around line 3, the crate version was bumped to
"3.0.0-beta.4" but related package.json files still reference older/unaligned
versions; update tests/regression/package.json version from "3.0.0-beta.3" to
"3.0.0-beta.4" and review packages/ide/vscode/package.json which currently shows
"3.0.6" — either change it to "3.0.0-beta.4" to align with the release (if that
is intended) or document/confirm why it should remain at 3.0.6, then commit the
version changes.
| await db.user.create({ data: { id: 1 } }); | ||
| await expect(db.profile.create({ data: { id: 1 } })).toBeRejectedByPolicy(); | ||
| await expect(db.$setAuth({ id: 0 }).profile.create({ data: { id: 1, userId: 1 } })).toBeRejectedByPolicy(); | ||
| await expect(db.$setAuth({ id: 1 }).profile.create({ data: { id: 1, userId: 1 } })).resolves.toMatchObject({ | ||
| id: 1, | ||
| }); | ||
|
|
||
| await expect(db.profile.create({ data: { id: 2, user: { create: { id: 2 } } } })).toBeRejectedByPolicy(); | ||
| await expect(db.user.findUnique({ where: { id: 2 } })).toResolveNull(); | ||
| await expect( | ||
| db | ||
| .$setAuth({ id: 2 }) | ||
| .profile.create({ data: { id: 2, user: { create: { id: 2 } } }, include: { user: true } }), | ||
| ).resolves.toMatchObject({ | ||
| id: 2, | ||
| user: { | ||
| id: 2, | ||
| }, | ||
| }); | ||
|
|
||
| await db.user.create({ data: { id: 3 } }); | ||
| await expect( | ||
| db.$setAuth({ id: 2 }).profile.create({ data: { id: 3, user: { connect: { id: 3 } } } }), | ||
| ).toBeRejectedByPolicy(); | ||
| await expect( | ||
| db.$setAuth({ id: 3 }).profile.create({ data: { id: 3, user: { connect: { id: 3 } } } }), | ||
| ).toResolveTruthy(); | ||
|
|
||
| await expect(db.$setAuth({ id: 4 }).profile.create({ data: { id: 2, userId: 4 } })).toBeRejectedByPolicy(); | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Likely wrong assertion: failure is due to PK/FK, not policy
You already created Profile { id: 2 } earlier, and User { id: 4 } doesn’t exist. The final expectation will fail for DB constraints (duplicate PK or missing FK), not policy. Adjust the scenario to truly exercise the policy mismatch.
Consider this fix to test a real policy rejection while avoiding constraint noise:
@@
- await db.user.create({ data: { id: 3 } });
+ await db.user.create({ data: { id: 3 } });
+ await db.user.create({ data: { id: 4 } });
@@
- await expect(db.$setAuth({ id: 4 }).profile.create({ data: { id: 2, userId: 4 } })).toBeRejectedByPolicy();
+ // auth=4 trying to create a profile for userId=3 (not self) → reject by policy
+ await expect(
+ db.$setAuth({ id: 4 }).profile.create({ data: { id: 4, userId: 3 } })
+ ).toBeRejectedByPolicy();If you intended to test DB constraints instead, assert a generic rejection (e.g., rejects.toThrow) and clarify in the test name.
Fix test: final create will fail on DB PK/FK, not policy
File: packages/runtime/test/policy/crud/create.test.ts Lines: 179-208
Profile id=2 is already created above and User id=4 does not exist — the final create will error on DB constraints (duplicate PK or missing FK), so toBeRejectedByPolicy() is the wrong assertion. Either assert a DB constraint error or change the inputs to exercise a policy rejection. Suggested fix to test policy rejection:
@@
- await db.user.create({ data: { id: 3 } });
+ await db.user.create({ data: { id: 3 } });
+ await db.user.create({ data: { id: 4 } });
@@
- await expect(db.$setAuth({ id: 4 }).profile.create({ data: { id: 2, userId: 4 } })).toBeRejectedByPolicy();
+ // auth=4 trying to create a profile for userId=3 (not self) → reject by policy
+ await expect(
+ db.$setAuth({ id: 4 }).profile.create({ data: { id: 4, userId: 3 } })
+ ).toBeRejectedByPolicy();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await db.user.create({ data: { id: 1 } }); | |
| await expect(db.profile.create({ data: { id: 1 } })).toBeRejectedByPolicy(); | |
| await expect(db.$setAuth({ id: 0 }).profile.create({ data: { id: 1, userId: 1 } })).toBeRejectedByPolicy(); | |
| await expect(db.$setAuth({ id: 1 }).profile.create({ data: { id: 1, userId: 1 } })).resolves.toMatchObject({ | |
| id: 1, | |
| }); | |
| await expect(db.profile.create({ data: { id: 2, user: { create: { id: 2 } } } })).toBeRejectedByPolicy(); | |
| await expect(db.user.findUnique({ where: { id: 2 } })).toResolveNull(); | |
| await expect( | |
| db | |
| .$setAuth({ id: 2 }) | |
| .profile.create({ data: { id: 2, user: { create: { id: 2 } } }, include: { user: true } }), | |
| ).resolves.toMatchObject({ | |
| id: 2, | |
| user: { | |
| id: 2, | |
| }, | |
| }); | |
| await db.user.create({ data: { id: 3 } }); | |
| await expect( | |
| db.$setAuth({ id: 2 }).profile.create({ data: { id: 3, user: { connect: { id: 3 } } } }), | |
| ).toBeRejectedByPolicy(); | |
| await expect( | |
| db.$setAuth({ id: 3 }).profile.create({ data: { id: 3, user: { connect: { id: 3 } } } }), | |
| ).toResolveTruthy(); | |
| await expect(db.$setAuth({ id: 4 }).profile.create({ data: { id: 2, userId: 4 } })).toBeRejectedByPolicy(); | |
| }); | |
| await db.user.create({ data: { id: 1 } }); | |
| await expect(db.profile.create({ data: { id: 1 } })).toBeRejectedByPolicy(); | |
| await expect(db.$setAuth({ id: 0 }).profile.create({ data: { id: 1, userId: 1 } })).toBeRejectedByPolicy(); | |
| await expect(db.$setAuth({ id: 1 }).profile.create({ data: { id: 1, userId: 1 } })).resolves.toMatchObject({ | |
| id: 1, | |
| }); | |
| await expect(db.profile.create({ data: { id: 2, user: { create: { id: 2 } } } })).toBeRejectedByPolicy(); | |
| await expect(db.user.findUnique({ where: { id: 2 } })).toResolveNull(); | |
| await expect( | |
| db | |
| .$setAuth({ id: 2 }) | |
| .profile.create({ data: { id: 2, user: { create: { id: 2 } } }, include: { user: true } }), | |
| ).resolves.toMatchObject({ | |
| id: 2, | |
| user: { | |
| id: 2, | |
| }, | |
| }); | |
| await db.user.create({ data: { id: 3 } }); | |
| await db.user.create({ data: { id: 4 } }); | |
| await expect( | |
| db.$setAuth({ id: 2 }).profile.create({ data: { id: 3, user: { connect: { id: 3 } } } }), | |
| ).toBeRejectedByPolicy(); | |
| await expect( | |
| db.$setAuth({ id: 3 }).profile.create({ data: { id: 3, user: { connect: { id: 3 } } } }), | |
| ).toResolveTruthy(); | |
| // auth=4 trying to create a profile for userId=3 (not self) → reject by policy | |
| await expect( | |
| db.$setAuth({ id: 4 }).profile.create({ data: { id: 4, userId: 3 } }), | |
| ).toBeRejectedByPolicy(); | |
| }); |
| it('works with to-one relation check owner side', async () => { | ||
| const db = await createPolicyTestClient( | ||
| ` | ||
| model User { | ||
| id Int @id | ||
| name String | ||
| profile Profile @relation(fields: [profileId], references: [id]) | ||
| profileId Int @unique | ||
| @@allow('all', true) | ||
| } | ||
| model Profile { | ||
| id Int @id | ||
| bio String | ||
| user User? | ||
| @@allow('create,read', true) | ||
| @@allow('update', user.name == 'User2') | ||
| } | ||
| `, | ||
| ); | ||
|
|
||
| await db.user.create({ data: { id: 1, name: 'User1', profile: { create: { id: 1, bio: 'Bio1' } } } }); | ||
| await expect(db.profile.update({ where: { id: 1 }, data: { bio: 'UpdatedBio1' } })).toBeRejectedNotFound(); | ||
|
|
||
| await db.user.create({ data: { id: 2, name: 'User2', profile: { create: { id: 2, bio: 'Bio2' } } } }); | ||
| await expect(db.profile.update({ where: { id: 2 }, data: { bio: 'UpdatedBio2' } })).resolves.toMatchObject({ | ||
| bio: 'UpdatedBio2', | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Duplicate test name detected.
Lines 159 and 129 both have the same test description "works with to-one relation check owner side", but they test different scenarios (owner vs non-owner foreign key placement).
Apply this diff to fix the test name:
- it('works with to-one relation check owner side', async () => {
+ it('works with to-one relation check non-owner side', async () => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('works with to-one relation check owner side', async () => { | |
| const db = await createPolicyTestClient( | |
| ` | |
| model User { | |
| id Int @id | |
| name String | |
| profile Profile @relation(fields: [profileId], references: [id]) | |
| profileId Int @unique | |
| @@allow('all', true) | |
| } | |
| model Profile { | |
| id Int @id | |
| bio String | |
| user User? | |
| @@allow('create,read', true) | |
| @@allow('update', user.name == 'User2') | |
| } | |
| `, | |
| ); | |
| await db.user.create({ data: { id: 1, name: 'User1', profile: { create: { id: 1, bio: 'Bio1' } } } }); | |
| await expect(db.profile.update({ where: { id: 1 }, data: { bio: 'UpdatedBio1' } })).toBeRejectedNotFound(); | |
| await db.user.create({ data: { id: 2, name: 'User2', profile: { create: { id: 2, bio: 'Bio2' } } } }); | |
| await expect(db.profile.update({ where: { id: 2 }, data: { bio: 'UpdatedBio2' } })).resolves.toMatchObject({ | |
| bio: 'UpdatedBio2', | |
| }); | |
| }); | |
| it('works with to-one relation check non-owner side', async () => { | |
| const db = await createPolicyTestClient( | |
| ` | |
| model User { | |
| id Int @id | |
| name String | |
| profile Profile @relation(fields: [profileId], references: [id]) | |
| profileId Int @unique | |
| @@allow('all', true) | |
| } | |
| model Profile { | |
| id Int @id | |
| bio String | |
| user User? | |
| @@allow('create,read', true) | |
| @@allow('update', user.name == 'User2') | |
| } | |
| `, | |
| ); | |
| await db.user.create({ data: { id: 1, name: 'User1', profile: { create: { id: 1, bio: 'Bio1' } } } }); | |
| await expect(db.profile.update({ where: { id: 1 }, data: { bio: 'UpdatedBio1' } })).toBeRejectedNotFound(); | |
| await db.user.create({ data: { id: 2, name: 'User2', profile: { create: { id: 2, bio: 'Bio2' } } } }); | |
| await expect(db.profile.update({ where: { id: 2 }, data: { bio: 'UpdatedBio2' } })).resolves.toMatchObject({ | |
| bio: 'UpdatedBio2', | |
| }); | |
| }); |
🤖 Prompt for AI Agents
packages/runtime/test/policy/crud/update.test.ts lines 159-187: the test name
duplicates another at line 129 but the two cover different FK placements; rename
this test's description to clearly indicate it's the owner-side FK scenario,
e.g. change the it(...) string to something like "works with to-one relation
check owner side (foreign key on User)" so the names are unique and reflect the
different scenarios.
| export function testLogger(e: LogEvent) { | ||
| console.log(e.query.sql, e.query.parameters); | ||
| } |
There was a problem hiding this comment.
Guard against non-query log events to prevent runtime errors.
LogEvent can represent non-query events; e.query may be absent, leading to a crash. Narrow by level.
Apply:
-export function testLogger(e: LogEvent) {
- console.log(e.query.sql, e.query.parameters);
-}
+export function testLogger(e: LogEvent) {
+ if (e.level === 'query') {
+ console.log(e.query.sql, e.query.parameters);
+ }
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function testLogger(e: LogEvent) { | |
| console.log(e.query.sql, e.query.parameters); | |
| } | |
| export function testLogger(e: LogEvent) { | |
| if (e.level === 'query') { | |
| console.log(e.query.sql, e.query.parameters); | |
| } | |
| } |
🤖 Prompt for AI Agents
In packages/runtime/test/utils.ts around lines 196 to 198, the testLogger
assumes e.query exists and will crash for non-query LogEvent types; update the
function to first check that the event is a query (e.g., verify e.level
indicates a query or that e.query is defined) and only then access e.query.sql
and e.query.parameters; for non-query events simply return or handle them safely
(e.g., log a different summary) so the logger never dereferences an undefined
e.query.
| const pluginModelFiles = glob.sync(path.resolve(dir, '../../packages/runtime/dist/**/plugin.zmodel')); | ||
| const result = await loadDocument(schemaPath, pluginModelFiles); | ||
| if (!result.success) { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Make plugin model discovery robust (dist may not exist yet)
When running locally, dist//plugin.zmodel might be absent. Fall back to src//plugin.zmodel.
- const pluginModelFiles = glob.sync(path.resolve(dir, '../../packages/runtime/dist/**/plugin.zmodel'));
- const result = await loadDocument(schemaPath, pluginModelFiles);
+ const distPlugins = glob.sync(path.resolve(dir, '../../packages/runtime/dist/**/plugin.zmodel'));
+ const srcPlugins =
+ distPlugins.length === 0
+ ? glob.sync(path.resolve(dir, '../../packages/runtime/src/**/plugin.zmodel'))
+ : [];
+ const pluginModelFiles = [...distPlugins, ...srcPlugins];
+ const result = await loadDocument(schemaPath, pluginModelFiles);Run to verify files exist:
🏁 Script executed:
#!/bin/bash
fd -a -t f plugin.zmodel packages/runtimeLength of output: 68
Make plugin model discovery robust — fall back to src when dist is missing
dist/**/plugin.zmodel can be absent locally; verification located packages/runtime/src/plugins/policy/plugin.zmodel. Update discovery to prefer dist then fall back to src.
File: tests/regression/generate.ts Lines: 22-24
- const pluginModelFiles = glob.sync(path.resolve(dir, '../../packages/runtime/dist/**/plugin.zmodel'));
- const result = await loadDocument(schemaPath, pluginModelFiles);
+ const distPlugins = glob.sync(path.resolve(dir, '../../packages/runtime/dist/**/plugin.zmodel'));
+ const srcPlugins =
+ distPlugins.length === 0
+ ? glob.sync(path.resolve(dir, '../../packages/runtime/src/**/plugin.zmodel'))
+ : [];
+ const pluginModelFiles = [...distPlugins, ...srcPlugins];
+ const result = await loadDocument(schemaPath, pluginModelFiles);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const pluginModelFiles = glob.sync(path.resolve(dir, '../../packages/runtime/dist/**/plugin.zmodel')); | |
| const result = await loadDocument(schemaPath, pluginModelFiles); | |
| if (!result.success) { | |
| const distPlugins = glob.sync(path.resolve(dir, '../../packages/runtime/dist/**/plugin.zmodel')); | |
| const srcPlugins = | |
| distPlugins.length === 0 | |
| ? glob.sync(path.resolve(dir, '../../packages/runtime/src/**/plugin.zmodel')) | |
| : []; | |
| const pluginModelFiles = [...distPlugins, ...srcPlugins]; | |
| const result = await loadDocument(schemaPath, pluginModelFiles); | |
| if (!result.success) { |
🤖 Prompt for AI Agents
In tests/regression/generate.ts around lines 22 to 24, plugin model discovery
currently only searches dist/**/plugin.zmodel which can be missing locally;
update the logic to first attempt to find plugin.zmodel under
packages/runtime/dist/** and if that yields no files, fall back to
packages/runtime/src/**; implement this by running the dist glob and if the
result is empty then run the src glob (or use a combined pattern that checks
both locations) and pass the found files to loadDocument so the test works both
for built and local dev trees.
| "scripts": { | ||
| "generate": "tsx generate.ts", | ||
| "test": "pnpm generate && tsc && vitest run" | ||
| }, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure local scripts have their toolchain available
Add explicit devDeps to avoid relying on hoisting.
"scripts": {
"generate": "tsx generate.ts",
"test": "pnpm generate && tsc && vitest run"
},
@@
"devDependencies": {
+ "tsx": "^4.16.0",
+ "typescript": "^5.5.4",
+ "vitest": "^1.6.0",
"@zenstackhq/cli": "workspace:*",
"@zenstackhq/sdk": "workspace:*",
"@zenstackhq/language": "workspace:*",
"@zenstackhq/runtime": "workspace:*",
"@zenstackhq/typescript-config": "workspace:*",
"@zenstackhq/vitest-config": "workspace:*"
}If these already exist at the root, confirm they resolve here:
🏁 Script executed:
#!/bin/bash
pnpm -w ls tsx typescript vitestLength of output: 134
Add missing local devDependencies (tsx, vitest) for tests/regression
Root workspace has [email protected] but tsx and vitest are not present — update tests/regression/package.json devDependencies to include tsx and vitest (or ensure workspace hoisting exposes them).
🤖 Prompt for AI Agents
In tests/regression/package.json around lines 6–9 the test script references tsx
and vitest but those packages are not declared locally; add tsx and vitest to
the package's devDependencies (or point them to the workspace versions via
workspace: specifier) and then reinstall so the scripts can run reliably —
alternatively ensure workspace hoisting exposes them by adding them to the root
devDependencies and verifying pnpm installs them for the tests/regression
package.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
Tests