refactor: move policy attributes to its own zmodel #307
Conversation
|
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. WalkthroughThis PR moves policy DSL declarations from the core stdlib into a dedicated policy plugin, adds plugin discovery and loading into schema parsing, updates runtime/SDK exports to support plugin-driven utilities, and wires the policy plugin into samples, tests, and tooling. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 PR Review: Move Policy Attributes to Own ZModelI've completed a comprehensive review of this PR that moves policy attributes from the core Review Tasks:
✅ Positive Aspects1. Excellent Separation of Concerns
2. Proper Plugin Discovery Mechanism
3. Comprehensive Integration
🔍 Code Quality Analysis1. Implementation Quality
2. Documentation and Comments
|
|
Claude finished @ymc9's task —— View job PR Review: Move Policy Attributes to Own ZModelI've completed a comprehensive review of this PR that moves policy attributes from the core Review Tasks:
✅ Positive Aspects1. Excellent Separation of Concerns
2. Proper Plugin Discovery Mechanism
3. Comprehensive Integration
🔍 Code Quality Analysis1. Implementation Quality
2. Documentation and Comments
|
There was a problem hiding this comment.
Pull Request Overview
Refactors policy-related DSL constructs out of the stdlib into a dedicated plugin module and wires up discovery/loading of plugin-contributed model files across CLI, tests, and samples.
- Introduce plugin.zmodel for the policy plugin and update sample schema to consume it.
- Update loaders (CLI/test tools/scripts) to include plugin models during parsing and generation.
- Move/adjust utilities: use runtime’s SchemaUtils/KyselyUtils in policy plugin; tweak SDK exports.
Reviewed Changes
Copilot reviewed 27 out of 29 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/e2e/orm/scripts/generate.ts | Script now loads plugin.zmodel when generating TS schema for tests. |
| tests/e2e/orm/policy/migrated/current-operation.test.ts | Test schema updated to mark id as @id. |
| tests/e2e/orm/policy/migrated/current-model.test.ts | Test schema updated to mark id as @id. |
| samples/blog/zenstack/schema.zmodel | Adds policy plugin provider and model-level allow rules. |
| samples/blog/zenstack/schema.ts | Generated schema now contains policy attributes (ExpressionUtils usage). |
| samples/blog/zenstack/models.ts | Removes extra comment lines (doc comment change only). |
| samples/blog/package.json | Adds dependency on @zenstackhq/plugin-policy. |
| samples/blog/main.ts | Demonstrates policy-enabled reads via PolicyPlugin. |
| packages/testtools/src/utils.ts | Adds helper to load language with plugin model files. |
| packages/testtools/src/schema.ts | Uses loadDocumentWithPlugins to generate/load schemas in tests. |
| packages/testtools/src/client.ts | Uses loadDocumentWithPlugins when using prisma push path. |
| packages/sdk/src/model-utils.ts | Minor type-only import adjustment for Model. |
| packages/sdk/src/index.ts | Stops re-exporting expression-utils and default-operation-node-visitor. |
| packages/runtime/src/utils/schema-utils.ts | Corrects schema types import path; centralizes ExpressionVisitor. |
| packages/runtime/src/index.ts | Re-exports KyselyUtils and SchemaUtils from runtime. |
| packages/plugins/policy/src/policy-handler.ts | Uses SchemaUtils.ExpressionVisitor from runtime. |
| packages/plugins/policy/src/column-collector.ts | Uses KyselyUtils.DefaultOperationNodeVisitor from runtime. |
| packages/plugins/policy/plugin.zmodel | New policy plugin DSL surface (attributes/functions). |
| packages/plugins/policy/package.json | Publishes plugin.zmodel via files/exports. |
| packages/language/src/validators/function-invocation-validator.ts | Removes stdlib-only guard; validator logic no longer limited to stdlib functions. |
| packages/language/src/utils.ts | Loosens isBeforeInvocation to not rely on stdlib origin. |
| packages/language/src/index.ts | Renames pluginDocs to additionalDocs and builds with additional docs. |
| packages/language/res/stdlib.zmodel | Removes policy constructs from stdlib. |
| packages/cli/src/constants.ts | Adds PLUGIN_MODULE_NAME constant. |
| packages/cli/src/actions/generate.ts | Tolerates plugins without generators. |
| packages/cli/src/actions/action-utils.ts | Loads plugin model files discovered from plugin provider values. |
| .vscode/launch.json | Updates sample launch cwd. |
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: 3
🧹 Nitpick comments (3)
packages/plugins/policy/plugin.zmodel (1)
17-17: Note: commented-out field-level attributes.The commented-out
@allow(line 17) and@deny(line 33) field-level attributes suggest these features are planned but not yet implemented. This is fine for a refactor PR focused on moving existing functionality.If field-level policies are not planned for the near term, consider adding a TODO comment explaining the deferral reason, or remove these placeholders to reduce confusion.
Also applies to: 33-33
tests/e2e/orm/scripts/generate.ts (1)
32-32: Consider removing redundant type cast.The cast
as Modelappears unnecessary sinceresult.modelis already typed asModelaccording to theloadDocumentreturn type signature.Apply this diff to remove the redundant cast:
- await generator.generate(result.model as Model, outputDir); + await generator.generate(result.model, outputDir);packages/cli/src/actions/action-utils.ts (1)
55-63: Silent failure on parse errors may hide issues.When there are lexer or parser errors (lines 61-62), the function returns an empty array without logging or warning. This could silently skip plugin loading when the schema has syntax errors, making it harder to diagnose issues.
Consider logging a warning or returning the parse errors so callers can decide how to handle them.
Apply this approach to improve diagnostics:
- // balk if there are syntax errors if (parseResult.lexerErrors.length > 0 || parseResult.parserErrors.length > 0) { + // Log parse errors but continue without plugins to allow validation to surface the errors + console.warn(colors.yellow('Warning: Schema has syntax errors, skipping plugin discovery')); return []; }
📜 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 (27)
.vscode/launch.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/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/plugins/policy/package.json(2 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/src/index.ts(1 hunks)packages/runtime/src/utils/schema-utils.ts(1 hunks)packages/sdk/src/index.ts(0 hunks)packages/sdk/src/model-utils.ts(1 hunks)packages/testtools/src/client.ts(2 hunks)packages/testtools/src/schema.ts(5 hunks)packages/testtools/src/utils.ts(1 hunks)samples/blog/main.ts(2 hunks)samples/blog/package.json(1 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)
💤 Files with no reviewable changes (3)
- packages/language/res/stdlib.zmodel
- samples/blog/zenstack/models.ts
- packages/sdk/src/index.ts
🧰 Additional context used
📓 Path-based instructions (4)
{packages,samples,tests}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place packages only under
packages/,samples/, ortests/
Files:
packages/runtime/src/index.tspackages/cli/src/constants.tstests/e2e/orm/policy/migrated/current-model.test.tspackages/runtime/src/utils/schema-utils.tspackages/plugins/policy/package.jsonpackages/testtools/src/schema.tspackages/language/src/validators/function-invocation-validator.tspackages/testtools/src/client.tstests/e2e/orm/policy/migrated/current-operation.test.tspackages/plugins/policy/src/column-collector.tspackages/language/src/index.tssamples/blog/package.jsonpackages/plugins/policy/plugin.zmodelpackages/testtools/src/utils.tssamples/blog/zenstack/schema.tspackages/cli/src/actions/action-utils.tspackages/plugins/policy/src/policy-handler.tspackages/language/src/utils.tspackages/cli/src/actions/generate.tssamples/blog/zenstack/schema.zmodelsamples/blog/main.tstests/e2e/orm/scripts/generate.tspackages/sdk/src/model-utils.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/orm/scripts/generate.ts
**/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
**/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 (10)
packages/testtools/src/schema.ts (1)
packages/testtools/src/utils.ts (1)
loadDocumentWithPlugins(4-7)
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)
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)
samples/blog/zenstack/schema.ts (1)
packages/runtime/src/schema/expression.ts (1)
ExpressionUtils(16-120)
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)
packages/cli/src/actions/generate.ts (1)
packages/sdk/src/cli-plugin.ts (1)
CliPlugin(32-47)
samples/blog/main.ts (1)
packages/plugins/policy/src/plugin.ts (1)
PolicyPlugin(6-29)
tests/e2e/orm/scripts/generate.ts (1)
packages/language/src/index.ts (1)
loadDocument(21-136)
🔇 Additional comments (30)
packages/sdk/src/model-utils.ts (1)
16-16: LGTM! Proper use of type-only import.The change to a type-only import is correct since
Modelis exclusively used in type annotation positions (getContainingModelreturn type on Line 90 andgetAuthDeclparameter type on Line 104). This follows TypeScript best practices and can improve build performance.samples/blog/package.json (1)
18-18: LGTM!The addition of the
@zenstackhq/plugin-policyworkspace dependency correctly enables the plugin-based policy architecture introduced in this PR..vscode/launch.json (1)
14-14: LGTM!The working directory adjustment aligns with the refactored project structure, removing the nested
zenstacksubdirectory requirement for CLI debugging.packages/plugins/policy/src/policy-handler.ts (2)
273-273: LGTM!The switch to
SchemaUtils.ExpressionVisitorcorrectly uses the runtime-provided base class, maintaining consistency with the plugin architecture refactor.
10-10: No issues found. SchemaUtils and ExpressionVisitor are properly exported.Verification confirms:
SchemaUtilsis exported from@zenstackhq/runtimeas a namespace (packages/runtime/src/index.ts:3)ExpressionVisitoris exported from the schema-utils module (packages/runtime/src/utils/schema-utils.ts:15)- The import chain is complete and the reference to
SchemaUtils.ExpressionVisitorwill resolve correctlypackages/plugins/policy/src/column-collector.ts (2)
7-7: LGTM!The base class change to
KyselyUtils.DefaultOperationNodeVisitorcorrectly aligns with the runtime utilities migration.
1-1: KyselyUtils export is properly configured and correctly imported.Verification confirms that
KyselyUtilsis correctly exported from@zenstackhq/runtimeand the import incolumn-collector.tsis valid. TheDefaultOperationNodeVisitorclass is properly accessible via the namespace, and the entire export chain is intact.packages/plugins/policy/package.json (2)
16-17: LGTM!The
plugin.zmodelfile inclusion correctly packages the policy DSL definitions for distribution.
30-33: LGTM!The export entry for
./plugin.zmodelis correctly configured, making the policy DSL file accessible to consumers of the plugin package.samples/blog/zenstack/schema.ts (3)
72-75: LGTM!The auto-generated policy attributes for the User model correctly express:
- Unconditional read/create access
- Full access when authenticated as this user
The expression structure using
ExpressionUtilsis correct.
139-141: LGTM!The auto-generated policy attribute for the Profile model correctly uses the
checkfunction to delegate access control to the relateduserfield.
198-201: LGTM!The auto-generated policy attributes for the Post model correctly express:
- Public read access when published
- Full access for the post author
packages/plugins/policy/plugin.zmodel (2)
1-26: LGTM!The
@@allowand@@denyattribute definitions are well-documented with:
- Clear parameter descriptions
- Comprehensive completion hints covering all operations (create, read, update, post-update, delete, all)
- Proper type annotations
42-72: LGTM!The helper functions (
check,before,currentModel,currentOperation) are well-defined with:
- Clear documentation and parameter descriptions
- Proper
@@@expressionContextannotations limiting usage to AccessPolicy contexts- Support for optional parameters (operation casing, etc.)
packages/runtime/src/utils/schema-utils.ts (1)
13-13: LGTM!The import path adjustment from
./schemato../schemacorrectly reflects the module structure reorganization in the runtime package.tests/e2e/orm/policy/migrated/current-model.test.ts (1)
161-161: LGTM: Proper addition of @id directive.The addition of the
@iddirective aligns the test model with proper schema conventions. This makes the test case more realistic and complete.packages/language/src/index.ts (1)
54-58: LGTM: Clear variable rename improves readability.Renaming
pluginDocstoadditionalDocsis more accurate since this parameter can accept any additional model files, not just plugin-specific documents. The change is consistent across both declaration and usage.Also applies to: 72-72
packages/runtime/src/index.ts (1)
2-3: Verify stability of newly exposed utilities.You're expanding the public API surface by exporting
KyselyUtilsandSchemaUtilsas namespaces. Ensure these utilities:
- Have stable interfaces suitable for public consumption
- Are properly documented
- Follow semantic versioning for any future changes
Once these utilities are public, breaking changes will affect external consumers.
packages/cli/src/constants.ts (1)
4-5: LGTM: Constant standardizes plugin model naming.Introducing
PLUGIN_MODULE_NAMEas a constant is a good practice that enables consistent plugin model file discovery across the codebase.samples/blog/main.ts (1)
1-1: LGTM: Clear demonstration of plugin-based policy feature.The addition demonstrates the new plugin-based policy architecture effectively:
- Proper
PolicyPluginintegration via$use()- Authorization context establishment with
$setAuth()- Clear demonstration of policy-aware reads for different users
This sample code provides a good reference for users adopting the new policy plugin.
Also applies to: 94-103
packages/cli/src/actions/generate.ts (1)
67-67: LGTM: Proper type widening and guard addition.The type change to
CliPlugin | undefinedand the corresponding guard at lines 86-88 correctly handle plugins that don't export generators. This allows the system to gracefully skip non-generator plugins.Also applies to: 86-88
packages/testtools/src/client.ts (1)
17-17: LGTM: Aligns with plugin-aware document loading.The change from
loadDocumenttoloadDocumentWithPluginscorrectly integrates plugin model resolution during the Prisma push path. Based on the code snippet frompackages/testtools/src/utils.ts, this function wrapsloadDocumentwith plugin model paths.Note: The implementation in
utils.tsappears to use a hardcoded path similar to the concern raised intests/e2e/orm/scripts/generate.ts. Ensure consistency in how plugin models are resolved across the codebase.Also applies to: 119-119
tests/e2e/orm/scripts/generate.ts (1)
22-26: Review comment is incorrect. The hardcoded path is appropriate for this test script context.The original suggestion to use
getPluginDocumentsmisidentifies the use case.getPluginDocumentsis an async function that discovers plugins by parsing schema declarations and requiresZModelServices(a full language server setup). The hardcoded path here provides a known plugin toloadDocument, which is the correct pattern for a standalone test utility script without language services.The same hardcoded path pattern is used consistently throughout
packages/testtools/src/utils.tsfor identical scenarios, and this approach is deterministic in npm workspaces where node_modules location is fixed. No changes needed.Likely an incorrect or invalid review comment.
packages/language/src/utils.ts (1)
450-453: LGTM! Aligns with plugin-based refactor.The removal of the
isFromStdlibcheck aligns with the broader refactoring to move policy-related functionality to a plugin architecture. The TODO comment appropriately flags this for future migration.tests/e2e/orm/policy/migrated/current-operation.test.ts (1)
130-130: LGTM! Corrects test schema.Adding the
@idattribute ensures the test schema is valid and properly tests the constraint thatcurrentOperation()cannot be used outside policy contexts.packages/testtools/src/schema.ts (1)
12-12: LGTM! Consistent migration to plugin-aware loading.The import change and subsequent usage throughout the file consistently migrates from
loadDocumenttoloadDocumentWithPlugins, enabling plugin policy integration in test tooling.packages/language/src/validators/function-invocation-validator.ts (1)
54-76: LGTM! Generalizes validation for plugin architecture.The refactoring removes the stdlib-only restriction, making validation context computation unconditional. This aligns with the broader plugin-based architecture where policy functions can come from plugins rather than only stdlib.
The logic correctly:
- Finds the containing attribute for any invocation
- Computes expression context regardless of origin
- Validates function context constraints consistently
samples/blog/zenstack/schema.zmodel (1)
31-32: LGTM! Demonstrates policy plugin usage.The access rules correctly demonstrate the new plugin-based policy feature with
@@allowdeclarations usingauth()for user authentication context.packages/cli/src/actions/action-utils.ts (1)
95-103: LGTM! ESM module resolution provides fallback.The ESM module resolution using
import.meta.resolveprovides a robust fallback for resolving plugin modules from node_modules, complementing the file-based resolution strategies.packages/testtools/src/utils.ts (1)
4-7: Verify plugin path resolution robustness and test coverage for workspace dependency.The hardcoded path
../node_modules/@zenstackhq/plugin-policy/plugin.zmodelis incompatible with workspace dependency resolution. Since@zenstackhq/plugin-policyis declared as aworkspace:*dependency inpackages/testtools/package.json, its location depends on the package manager and monorepo configuration—not always innode_modules. Additionally, there is no file existence validation before passing the path toloadDocument.The production approach exists in
packages/cli/src/actions/action-utils.tswheregetPluginDocumentsdynamically discovers plugins by:
- Parsing the model file to extract plugin declarations
- Validating paths with
fs.existsSync()- Returning empty array on parse errors (graceful degradation)
Manually verify:
- Whether tests exercise scenarios where the plugin is resolved differently (pnpm, yarn, different build output directories)
- Whether the current hardcoded path reliably resolves in CI/CD and local development environments
- Whether adopting dynamic discovery or adding existence checks is appropriate here
|
Claude finished @ymc9's task —— View job PR Review: Move Policy Attributes to Own ZModelI've completed a comprehensive review of this PR that moves policy attributes from the core Review Tasks:
✅ Positive Aspects1. Excellent Separation of Concerns
2. Proper Plugin Discovery Mechanism
3. Comprehensive Integration
🔍 Code Quality Analysis1. Implementation Quality
2. Documentation and Comments
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/plugins/policy/plugin.zmodel (1)
17-17: Consider removing or documenting commented-out field-level attributes.Lines 17 and 33 contain commented-out declarations for field-level
@allowand@deny. If these are planned for future implementation, add a TODO comment explaining the intent. Otherwise, remove them to reduce clutter.Also applies to: 33-33
📜 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 (2)
packages/language/test/utils.ts(2 hunks)packages/plugins/policy/plugin.zmodel(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/language/test/utils.tspackages/plugins/policy/plugin.zmodel
🧬 Code graph analysis (1)
packages/language/test/utils.ts (2)
packages/testtools/src/schema.ts (1)
loadSchema(94-124)packages/language/src/index.ts (1)
loadDocument(21-136)
⏰ 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)
🔇 Additional comments (3)
packages/language/test/utils.ts (2)
8-8: LGTM! Path construction is correct.The path correctly resolves to
packages/plugins/policy/plugin.zmodelfrom the test directory.
14-14: LGTM! Plugin documents correctly integrated.The plugin documents are properly passed to
loadDocumentin both test utilities, aligning with the updated function signature.Also applies to: 28-28
packages/plugins/policy/plugin.zmodel (1)
35-72: LGTM! Function declarations are well-documented.The
check,before,currentModel, andcurrentOperationfunctions are clearly documented with appropriate expression context annotations and parameter descriptions.
Summary by CodeRabbit
New Features
Tooling
Tests