merge dev to main (v3.0.0-beta.10) #310
Conversation
* refactor: move policy attributes to its own zmodel * update * update * fix tests
Co-authored-by: ymc9 <[email protected]>
|
Claude finished @ymc9's task —— View job PR Review CompleteAfter thoroughly reviewing this v3.0.0-beta.10 release merge, I've identified several key areas of feedback: Review Checklist
🎯 SummaryThis PR represents a significant architectural improvement by extracting policy attributes to a dedicated ✅ Positive Changes
|
|
Warning Rate limit exceeded@ymc9 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 35 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (8)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughMove policy DSL from stdlib into a new policy plugin, add plugin discovery and plugin-aware document loading across CLI and test tooling, update runtime/sdk exports and policy handlers to use runtime-provided utils, adjust validators to validate policy functions universally, and bump workspace packages to 3.0.0-beta.10. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
There was a problem hiding this comment.
Pull Request Overview
This PR merges the dev branch to main for version 3.0.0-beta.10. The key change is the extraction of policy plugin functionality from the core language package into a standalone plugin package, with supporting infrastructure updates to enable dynamic plugin model loading.
Key changes:
- Policy-related definitions moved from stdlib.zmodel to a new plugin.zmodel file in the policy plugin package
- Updated module loading infrastructure to support dynamic plugin model files
- Version bumps across all packages to 3.0.0-beta.10
Reviewed Changes
Copilot reviewed 44 out of 46 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/plugins/policy/plugin.zmodel | New file defining policy-specific attributes and functions previously in stdlib |
| packages/plugins/policy/package.json | Updated to export plugin.zmodel and remove @zenstackhq/sdk dependency |
| packages/language/res/stdlib.zmodel | Removed policy-related definitions now in plugin.zmodel |
| packages/cli/src/actions/action-utils.ts | Added getPluginDocuments function to dynamically discover and load plugin models |
| packages/testtools/src/utils.ts | New utility function to load documents with plugin models |
| packages/runtime/src/index.ts | Exported KyselyUtils and SchemaUtils for use by plugins |
| packages/sdk/src/index.ts | Removed exports no longer needed externally |
| samples/blog/zenstack/schema.zmodel | Added policy plugin declaration and access policies |
| Multiple package.json files | Version bumps to 3.0.0-beta.10 |
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: 4
🧹 Nitpick comments (7)
samples/blog/main.ts (1)
94-103: Consider adding error handling for policy violations.The policy-enabled read demonstration shows the correct usage pattern:
- Apply PolicyPlugin via
$use- Set authentication context with
$setAuth- Execute queries that respect policy rules
However, the code lacks error handling for potential policy violations. When queries violate access policies, they may throw
RejectedByPolicyErrorexceptions.Consider wrapping the policy-enabled queries in try-catch blocks to demonstrate error handling:
// policy-enabled read const authDb = db.$use(new PolicyPlugin()); const user1Db = authDb.$setAuth({ id: user1.id }); const user2Db = authDb.$setAuth({ id: user2.id }); - console.log('Posts readable to', user1.email); - console.table(await user1Db.post.findMany({ select: { title: true, published: true } })); - - console.log('Posts readable to', user2.email); - console.table(await user2Db.post.findMany({ select: { title: true, published: true } })); + try { + console.log('Posts readable to', user1.email); + console.table(await user1Db.post.findMany({ select: { title: true, published: true } })); + } catch (error) { + console.error('Policy violation for user1:', error); + } + + try { + console.log('Posts readable to', user2.email); + console.table(await user2Db.post.findMany({ select: { title: true, published: true } })); + } catch (error) { + console.error('Policy violation for user2:', error); + }tests/e2e/orm/scripts/generate.ts (1)
22-33: Resolve plugin.zmodel via ESM instead of hard-coded pathHard-coding ../../node_modules is brittle across workspaces. Use import.meta.resolve to locate the plugin’s plugin.zmodel.
Apply:
- // isomorphic __dirname - const _dirname = typeof __dirname !== 'undefined' ? __dirname : path.dirname(fileURLToPath(import.meta.url)); - - // plugin models - const pluginDocs = [path.resolve(_dirname, '../../node_modules/@zenstackhq/plugin-policy/plugin.zmodel')]; - - const result = await loadDocument(schemaPath, pluginDocs); + // plugin models (resolve via ESM) + let pluginDocs: string[] = []; + try { + const url = import.meta.resolve('@zenstackhq/plugin-policy/plugin.zmodel'); + pluginDocs = [fileURLToPath(url)]; + } catch { + // fallback: proceed without plugin docs if not available + } + const result = await loadDocument(schemaPath, pluginDocs);packages/cli/src/actions/generate.ts (1)
67-89: Don’t fully silence plugin import failures; warn when skippingSwallowing all errors makes diagnosing plugin issues hard. Emit a warning (honoring options.silent) when a provider can’t be loaded as a generator.
Apply:
- try { - cliPlugin = (await import(moduleSpec)).default as CliPlugin; - } catch { - // plugin may not export a generator so we simply ignore the error here - } + try { + cliPlugin = (await import(moduleSpec)).default as CliPlugin; + } catch (e) { + if (!options.silent) { + console.warn( + colors.yellow( + `Plugin "${provider}" couldn't be loaded as a CLI generator, skipping${ + e instanceof Error && e.message ? `: ${e.message}` : '' + }`, + ), + ); + } + }packages/language/src/validators/function-invocation-validator.ts (1)
54-87: Universal context validation + casing checks look solidTraversal to enclosing attribute and expression-context enforcement aligns with plugin functions. Nice.
If desired, consider extending getExpressionContext to treat '@id', '@@id', '@unique', '@@unique' as Index as well.
packages/cli/src/actions/action-utils.ts (1)
55-111: Plugin doc discovery is good; add small hardening
- Dedupe discovered plugin files to avoid double-builds.
- Optionally add a require.resolve fallback when import.meta.resolve is unavailable.
Minimal dedupe:
- return result; + return [...new Set(result)];Optional fallback (if you choose to support non-ESM resolution environments), replace the import.meta.resolve try-block with:
- if (!pluginModelFile) { - // try loading it as a ESM module - try { - const resolvedUrl = import.meta.resolve(`${provider}/${PLUGIN_MODULE_NAME}`); - pluginModelFile = fileURLToPath(resolvedUrl); - } catch { - // noop - } - } + if (!pluginModelFile) { + // try loading it as an ESM module, fall back to require.resolve + try { + const resolvedUrl = import.meta.resolve(`${provider}/${PLUGIN_MODULE_NAME}`); + pluginModelFile = fileURLToPath(resolvedUrl); + } catch { + try { + const { createRequire } = await import('node:module'); + const req = createRequire(import.meta.url); + pluginModelFile = req.resolve(`${provider}/${PLUGIN_MODULE_NAME}`); + } catch { + // noop + } + } + }Also confirm the minimum Node version for the CLI supports import.meta.resolve.
packages/plugins/policy/plugin.zmodel (2)
7-7: Improve formatting consistency in completionHint array.The completionHint array has inconsistent spacing after
'post-update'.Apply this diff for consistent spacing:
-attribute @@allow(_ operation: String @@@completionHint(["'create'", "'read'", "'update'", "'post-update'","'delete'", "'all'"]), _ condition: Boolean) +attribute @@allow(_ operation: String @@@completionHint(["'create'", "'read'", "'update'", "'post-update'", "'delete'", "'all'"]), _ condition: Boolean)
25-25: Improve formatting consistency in completionHint array.The completionHint array has inconsistent spacing after
'post-update'.Apply this diff for consistent spacing:
-attribute @@deny(_ operation: String @@@completionHint(["'create'", "'read'", "'update'", "'post-update'","'delete'", "'all'"]), _ condition: Boolean) +attribute @@deny(_ operation: String @@@completionHint(["'create'", "'read'", "'update'", "'post-update'", "'delete'", "'all'"]), _ condition: Boolean)
📜 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 (44)
.vscode/launch.json(1 hunks)package.json(1 hunks)packages/cli/package.json(1 hunks)packages/cli/src/actions/action-utils.ts(3 hunks)packages/cli/src/actions/generate.ts(2 hunks)packages/cli/src/constants.ts(1 hunks)packages/common-helpers/package.json(1 hunks)packages/config/eslint-config/package.json(1 hunks)packages/config/typescript-config/package.json(1 hunks)packages/config/vitest-config/package.json(1 hunks)packages/create-zenstack/package.json(1 hunks)packages/dialects/sql.js/package.json(1 hunks)packages/language/package.json(1 hunks)packages/language/res/stdlib.zmodel(0 hunks)packages/language/src/index.ts(2 hunks)packages/language/src/utils.ts(1 hunks)packages/language/src/validators/function-invocation-validator.ts(1 hunks)packages/language/test/utils.ts(2 hunks)packages/plugins/policy/package.json(3 hunks)packages/plugins/policy/plugin.zmodel(1 hunks)packages/plugins/policy/src/column-collector.ts(1 hunks)packages/plugins/policy/src/policy-handler.ts(2 hunks)packages/runtime/package.json(1 hunks)packages/runtime/src/index.ts(1 hunks)packages/runtime/src/utils/schema-utils.ts(1 hunks)packages/sdk/package.json(1 hunks)packages/sdk/src/index.ts(0 hunks)packages/sdk/src/model-utils.ts(1 hunks)packages/tanstack-query/package.json(1 hunks)packages/testtools/package.json(1 hunks)packages/testtools/src/client.ts(2 hunks)packages/testtools/src/schema.ts(5 hunks)packages/testtools/src/utils.ts(1 hunks)packages/zod/package.json(1 hunks)samples/blog/main.ts(2 hunks)samples/blog/package.json(2 hunks)samples/blog/zenstack/models.ts(0 hunks)samples/blog/zenstack/schema.ts(3 hunks)samples/blog/zenstack/schema.zmodel(3 hunks)tests/e2e/orm/policy/migrated/current-model.test.ts(1 hunks)tests/e2e/orm/policy/migrated/current-operation.test.ts(1 hunks)tests/e2e/orm/scripts/generate.ts(2 hunks)tests/e2e/package.json(1 hunks)tests/regression/package.json(1 hunks)
💤 Files with no reviewable changes (3)
- samples/blog/zenstack/models.ts
- packages/language/res/stdlib.zmodel
- packages/sdk/src/index.ts
🧰 Additional context used
📓 Path-based instructions (5)
{packages,samples,tests}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place packages only under
packages/,samples/, ortests/
Files:
packages/config/eslint-config/package.jsonpackages/cli/package.jsonpackages/sdk/package.jsontests/regression/package.jsonpackages/runtime/src/index.tspackages/cli/src/constants.tspackages/plugins/policy/package.jsonpackages/language/src/index.tspackages/testtools/src/schema.tspackages/testtools/src/client.tspackages/testtools/src/utils.tspackages/language/test/utils.tspackages/cli/src/actions/generate.tstests/e2e/orm/policy/migrated/current-model.test.tspackages/testtools/package.jsonpackages/plugins/policy/src/policy-handler.tspackages/common-helpers/package.jsonpackages/tanstack-query/package.jsonpackages/runtime/package.jsonpackages/zod/package.jsonpackages/config/typescript-config/package.jsonpackages/plugins/policy/src/column-collector.tspackages/language/src/validators/function-invocation-validator.tssamples/blog/zenstack/schema.tspackages/sdk/src/model-utils.tssamples/blog/package.jsontests/e2e/orm/policy/migrated/current-operation.test.tstests/e2e/package.jsontests/e2e/orm/scripts/generate.tspackages/config/vitest-config/package.jsonpackages/runtime/src/utils/schema-utils.tspackages/create-zenstack/package.jsonpackages/cli/src/actions/action-utils.tspackages/plugins/policy/plugin.zmodelpackages/language/package.jsonpackages/language/src/utils.tssamples/blog/main.tspackages/dialects/sql.js/package.jsonsamples/blog/zenstack/schema.zmodel
**/schema.ts
📄 CodeRabbit inference engine (CLAUDE.md)
The generated TypeScript schema should be named
schema.ts
Files:
packages/testtools/src/schema.tssamples/blog/zenstack/schema.ts
tests/e2e/**
📄 CodeRabbit inference engine (CLAUDE.md)
End-to-end tests must live under
tests/e2e/
Files:
tests/e2e/orm/policy/migrated/current-model.test.tstests/e2e/orm/policy/migrated/current-operation.test.tstests/e2e/package.jsontests/e2e/orm/scripts/generate.ts
package.json
📄 CodeRabbit inference engine (CLAUDE.md)
Pin the repository package manager to
[email protected]via thepackageManagerfield
Files:
package.json
**/schema.zmodel
📄 CodeRabbit inference engine (CLAUDE.md)
Name ZModel schema files
schema.zmodel
Files:
samples/blog/zenstack/schema.zmodel
🧠 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:
tests/e2e/orm/scripts/generate.ts
🧬 Code graph analysis (11)
packages/testtools/src/schema.ts (1)
packages/testtools/src/utils.ts (1)
loadDocumentWithPlugins(4-7)
packages/testtools/src/client.ts (1)
packages/testtools/src/utils.ts (1)
loadDocumentWithPlugins(4-7)
packages/testtools/src/utils.ts (1)
packages/language/src/index.ts (1)
loadDocument(21-136)
packages/language/test/utils.ts (2)
packages/testtools/src/schema.ts (1)
loadSchema(94-124)packages/language/src/index.ts (1)
loadDocument(21-136)
packages/cli/src/actions/generate.ts (1)
packages/sdk/src/cli-plugin.ts (1)
CliPlugin(32-47)
packages/language/src/validators/function-invocation-validator.ts (2)
packages/language/src/generated/ast.ts (6)
DataModelAttribute(395-400)DataModelAttribute(402-402)DataFieldAttribute(348-353)DataFieldAttribute(355-355)isDataModelAttribute(404-406)isDataFieldAttribute(357-359)packages/language/src/utils.ts (2)
getFunctionExpressionContext(333-347)getLiteral(268-282)
samples/blog/zenstack/schema.ts (1)
packages/runtime/src/schema/expression.ts (1)
ExpressionUtils(16-120)
tests/e2e/orm/scripts/generate.ts (1)
packages/language/src/index.ts (1)
loadDocument(21-136)
packages/cli/src/actions/action-utils.ts (5)
packages/language/src/index.ts (2)
createZModelServices(11-13)loadDocument(21-136)packages/language/src/module.ts (1)
ZModelServices(31-31)packages/language/src/generated/ast.ts (1)
isPlugin(628-630)packages/language/src/utils.ts (1)
getLiteral(268-282)packages/cli/src/constants.ts (1)
PLUGIN_MODULE_NAME(5-5)
packages/language/src/utils.ts (2)
packages/language/src/ast.ts (2)
AstNode(4-4)AstNode(66-71)packages/language/src/generated/ast.ts (1)
isInvocationExpr(542-544)
samples/blog/main.ts (1)
packages/plugins/policy/src/plugin.ts (1)
PolicyPlugin(6-29)
⏰ 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, sqlite)
- GitHub Check: build-test (20.x, postgresql)
🔇 Additional comments (48)
packages/language/package.json (1)
4-4: Version bump consistent with release cycle. No concerns.packages/cli/package.json (1)
6-6: Version bump consistent with release cycle. No concerns.packages/create-zenstack/package.json (1)
3-3: Version bump consistent with release cycle. No concerns.packages/config/typescript-config/package.json (1)
3-3: Version bump consistent with release cycle. No concerns.samples/blog/package.json (1)
3-3: Version bump consistent; new policy plugin dependency appropriately added to sample dependencies.The addition of
@zenstackhq/plugin-policyenables the blog sample to demonstrate policy-driven access patterns, aligning with the new plugin-based architecture introduced in this PR.Also applies to: 18-18
package.json (1)
3-3: Root package version and packageManager correctly configured.The packageManager field is correctly pinned to
[email protected]per coding guidelines. Version bump is consistent with all subordinate packages.Also applies to: 5-5
tests/regression/package.json (1)
3-3: Version bump consistent with release cycle. No concerns.tests/e2e/package.json (1)
3-3: Version bump and policy plugin dependency verified as correct.The version
3.0.0-beta.10is consistent with the workspace packages, and@zenstackhq/plugin-policyis a valid workspace package atpackages/plugins/policy/with correctworkspace:*reference. The dependency addition enables policy-driven tests aligned with the plugin-based architecture refactor.packages/testtools/package.json (1)
3-3: LGTM! Version bump aligned with release.The version bump to 3.0.0-beta.10 is consistent with the coordinated release across all packages in this PR.
packages/language/src/utils.ts (1)
450-453: Confirm the semantic change aligns with plugin refactoring.The TODO comment and function implementation suggest that
beforeinvocations are no longer required to originate from the stdlib, which aligns with moving policy declarations to a plugin-based architecture. This is a semantic change that broadens the scope of what constitutes a "before invocation."Ensure that:
- Downstream code that relies on
isBeforeInvocationcan handle invocations from plugin sources.- This change is consistent with how
isCheckInvocation(line 360-362) is already implemented.Based on learnings about ZenStack's V3 plugin architecture refactoring.
packages/zod/package.json (1)
3-3: LGTM! Version bump aligned with release.The version bump to 3.0.0-beta.10 is part of the coordinated release across all packages.
packages/config/eslint-config/package.json (1)
3-3: LGTM! Version bump aligned with release.The version bump to 3.0.0-beta.10 is consistent with the coordinated release.
packages/common-helpers/package.json (1)
3-3: LGTM! Version bump aligned with release.The version bump to 3.0.0-beta.10 is part of the coordinated release across all packages.
packages/runtime/package.json (1)
3-3: LGTM! Version bump aligned with release.The version bump to 3.0.0-beta.10 is part of the coordinated release across all packages.
packages/sdk/src/model-utils.ts (1)
16-16: LGTM! Type-only import is a best practice.Changing
Modelto a type-only import is appropriate since it's only used in type annotations (line 90, 104). This improves bundle size and makes the import intent explicit..vscode/launch.json (1)
14-14: Based on the verification of the CLI implementation, the working directory change is correct and necessary.The ZenStack CLI defaults to searching for
zenstack/schema.zmodelrelative to the current working directory. By changing the working directory from${workspaceFolder}/samples/blog/zenstackto${workspaceFolder}/samples/blog, thegeneratecommand will now correctly locate the schema file at the expectedzenstack/schema.zmodelpath.The schema file is confirmed at
samples/blog/zenstack/schema.zmodel, and this change aligns with the CLI's default schema discovery behavior.packages/tanstack-query/package.json (1)
3-3: LGTM! Version bump is consistent with the release.The version increment from beta.9 to beta.10 aligns with the PR's objective to release v3.0.0-beta.10.
packages/dialects/sql.js/package.json (1)
3-3: LGTM! Version bump is consistent with the release.The version increment from beta.9 to beta.10 aligns with the PR's objective to release v3.0.0-beta.10.
packages/config/vitest-config/package.json (1)
4-4: LGTM! Version bump is consistent with the release.The version increment from beta.9 to beta.10 aligns with the PR's objective to release v3.0.0-beta.10.
packages/cli/src/constants.ts (1)
4-5: LGTM! Well-documented constant for plugin discovery.The constant is clearly named and documented, providing a standardized filename for plugin-contributed model files that will be used during plugin discovery.
packages/language/src/index.ts (1)
54-72: LGTM! Improved variable naming.The renaming from
pluginDocstoadditionalDocsbetter reflects the general-purpose nature of theadditionalModelFilesparameter. The usage is consistent throughout the function.packages/language/test/utils.ts (3)
8-8: LGTM! Hardcoded path is appropriate for test utilities.The hardcoded path to the policy plugin using
__dirnamefor relative resolution is acceptable in test utilities and will correctly resolve to the plugin source in the monorepo structure.
14-14: LGTM! Plugin-aware document loading.The addition of
pluginDocsas the second argument enables plugin-aware schema loading in the test utility, aligning with the PR's objective to support plugin-based policy architecture.
28-28: LGTM! Consistent plugin loading in error case.Plugin documents are correctly passed to
loadDocumentin the error test scenario as well, ensuring consistent behavior across test helpers.packages/testtools/src/utils.ts (1)
4-7: LGTM! Plugin-aware document loader for test tooling.The helper correctly resolves the plugin path from
node_modulesusing__dirname, which is appropriate for test tooling that depends on installed packages. The workspace dependency structure should ensure the plugin package is available.packages/testtools/src/client.ts (2)
17-17: LGTM! Updated to use plugin-aware document loading.The import change from
loadDocumenttoloadDocumentWithPluginscorrectly enables plugin support in the test client infrastructure, aligning with the PR's plugin-based architecture refactoring.
119-119: LGTM! Consistent plugin loading in Prisma push flow.The call to
loadDocumentWithPluginsensures that plugin model files are loaded when using Prisma DB push, maintaining consistency with the new plugin-aware loading strategy.packages/plugins/policy/package.json (2)
3-3: LGTM: Version bump aligns with PR release.The version bump to 3.0.0-beta.10 is consistent with the PR's objective to release this beta version.
15-18: Verification confirmed: plugin.zmodel file exists and contains valid policy DSL declarations.The file is properly structured with all expected policy-related attributes and helper functions (
@@allow,@@deny,check(),before(),currentModel(),currentOperation()). The declarations are complete and correctly documented for the plugin-based policy architecture.samples/blog/zenstack/schema.ts (4)
1-4: LGTM: Generated file with appropriate warning.The file header correctly indicates this is auto-generated by ZenStack CLI and should not be manually modified.
72-75: Review: User model policy rules appear correct.The User model includes two access control rules:
- Read and create operations allowed for all users (condition: true)
- All operations allowed when the authenticated user matches the resource (this == auth)
This is a common pattern for user self-management. However, verify that allowing "create" globally aligns with your authorization requirements—this permits any user (including unauthenticated) to create User records, which might be intentional for registration but should be confirmed.
139-141: Review: Profile model policy uses check function.The Profile model allows all operations when
check(user)returns true. Based on the retrieved learnings and the policy plugin structure, thecheckfunction evaluates the policy of the related entity.Verify that this delegation pattern is intentional—it means Profile accessibility is entirely determined by the associated User's policies.
198-201: LGTM: Post model policy rules are appropriate.The Post model has sensible access control:
- Read allowed for published posts
- All operations allowed when the author matches the authenticated user
This implements a standard content visibility pattern where authors can manage their own posts and published posts are publicly readable.
packages/sdk/package.json (1)
3-3: LGTM: Version bump aligns with PR release.The version bump to 3.0.0-beta.10 is consistent with the PR's objective.
packages/runtime/src/utils/schema-utils.ts (1)
13-13: LGTM: Import path updated to match directory structure.The import path change from
'./schema'to'../schema'adjusts to the module structure. This is a straightforward refactor with no runtime impact.packages/plugins/policy/src/column-collector.ts (1)
1-1: LGTM: Updated to use runtime's KyselyUtils abstraction.The change from directly importing Kysely internals to using
KyselyUtils.DefaultOperationNodeVisitorfrom the runtime package improves the abstraction layer and aligns with the removal of@zenstackhq/sdkdependency noted in the PR summary.Also applies to: 7-7
samples/blog/main.ts (1)
1-1: LGTM: PolicyPlugin import added.The import statement correctly brings in the PolicyPlugin from the new plugin package.
packages/plugins/policy/src/policy-handler.ts (2)
10-10: LGTM: Added SchemaUtils import from runtime.The import of
SchemaUtilsfrom@zenstackhq/runtimealigns with the architectural refactoring to remove the@zenstackhq/sdkdependency from the policy plugin.
273-281: LGTM: Updated to use runtime's SchemaUtils.ExpressionVisitor.The anonymous class now extends
SchemaUtils.ExpressionVisitorinstead of directly importing from SDK. This change:
- Removes the SDK dependency from the policy plugin
- Uses the runtime's abstraction layer
- Maintains the same visitor pattern functionality
This aligns with the broader refactoring to make the policy plugin independent of SDK internals.
tests/e2e/orm/policy/migrated/current-model.test.ts (1)
161-161: Negative test update looks correctAdding @id keeps the error context as DefaultValue; expectation remains valid. LGTM.
packages/runtime/src/index.ts (1)
2-3: Namespace re‑exports look goodExposes utils under stable names. Verify type declarations include these namespaces in the published package.
tests/e2e/orm/policy/migrated/current-operation.test.ts (1)
130-130: LGTM for the negative test tweakAdding @id keeps the invalid DefaultValue context; assertion remains correct.
packages/plugins/policy/plugin.zmodel (6)
9-17: Verify the purpose of the commented field-level@allowattribute.The field-level
@allowattribute declaration is commented out without explanation. Clarify whether this feature is:
- Not yet implemented and should remain commented
- Deprecated and should be removed
- Ready to be enabled
27-33: Verify the purpose of the commented field-level@denyattribute.Similar to
@allow, the field-level@denyattribute is commented out. Additionally, note that the commented@allowdocumentation mentions a thirdoverrideparameter, but this@denydeclaration doesn't include it. Clarify:
- Whether this feature should be enabled or removed
- Whether an
overrideparameter should be added for consistency with@allow
35-43: LGTM!The
checkfunction is well-documented and the signature correctly matches its intended purpose for verifying access permissions on fields.
45-49: LGTM!The
beforefunction is properly documented with a clear explanation of its usage context (post-update policies).
51-60: LGTM!The
currentModelfunction is well-documented with clear parameter options for casing transformation.
22-22: Fix typo in the documentation.Same typo as in line 4:
" "delete"should be"delete".Apply this diff to fix the typo:
- * @param operation: comma-separated list of "create", "read", "update", "post-update", "delete". Use "all" to denote all operations. + * @param operation: comma-separated list of "create", "read", "update", "post-update", "delete". Use "all" to denote all operations.Likely an incorrect or invalid review comment.
|
Claude finished @ymc9's task —— View job PR Review for v3.0.0-beta.10 Policy Plugin Refactoring
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/plugins/policy/plugin.zmodel (1)
62-72: Update documentation to include "post-update" in the "all" operation expansion.Line 64 states that "all" expands to "create", "read", "update", and "delete", but "post-update" is consistently referenced as a valid operation throughout this file (lines 4, 13, 22, 30, 39) and is included in the completion hints (lines 7, 25). The documentation should clarify whether "post-update" is included in the "all" expansion.
If "post-update" should be included in the expansion, apply this diff:
* The operation for which the policy rule is defined for. Note that a rule with - * "all" operation is expanded to "create", "read", "update", and "delete" rules, + * "all" operation is expanded to "create", "read", "update", "post-update", and "delete" rules, * and the function returns corresponding value for each expanded version.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/plugins/policy/plugin.zmodel(1 hunks)packages/testtools/src/utils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/testtools/src/utils.ts
🧰 Additional context used
📓 Path-based instructions (1)
{packages,samples,tests}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place packages only under
packages/,samples/, ortests/
Files:
packages/plugins/policy/plugin.zmodel
⏰ 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). (3)
- GitHub Check: build-test (20.x, postgresql)
- GitHub Check: build-test (20.x, sqlite)
- GitHub Check: claude-review
|
Claude finished @ymc9's task —— View job PR Review for v3.0.0-beta.10 Policy Plugin Refactoring
|
* fix: clean up plugin loading and fix VSCode extension * address pr comments
|
Claude finished @ymc9's task —— View job PR Review for v3.0.0-beta.10 Policy Plugin Refactoring
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/cli/src/actions/migrate.ts (1)
85-90: LGTM! Command construction improved.The refactoring to a multi-line array format improves readability and adding
--skip-generatebrings consistency withrunDev. The conditional--forcelogic is correct.Minor style note: Consider adding a trailing comma after line 89 for consistency with
runDev(line 74) and better diffs.const cmd = [ 'prisma migrate reset', ` --schema "${prismaSchemaFile}"`, ' --skip-generate', - options.force ? ' --force' : '' + options.force ? ' --force' : '', ].join('');
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/cli/src/actions/migrate.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
{packages,samples,tests}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place packages only under
packages/,samples/, ortests/
Files:
packages/cli/src/actions/migrate.ts
⏰ 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). (3)
- GitHub Check: claude-review
- GitHub Check: build-test (20.x, sqlite)
- GitHub Check: build-test (20.x, postgresql)
Summary by CodeRabbit
New Features
Bug Fixes
Chores