refactor: move policy zmodel back to stdlib #265
Conversation
WalkthroughMoved policy DSL declarations (allow/deny/check) from runtime plugin to language stdlib; restored future() in AccessPolicy. Updated loadDocument API to accept only schema path and removed plugin model discovery across tests, tooling, and scripts. Deleted runtime policy plugin file and removed related build copy hook. 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
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 refactors the policy zmodel definitions by moving them from the runtime package plugin system back to the language package's standard library. This simplifies the architecture by eliminating the need for plugin-specific model files and their associated loading mechanisms.
- Removes plugin-based policy model loading infrastructure
- Consolidates policy attributes and functions into the main stdlib.zmodel
- Simplifies document loading by removing plugin model file parameters
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/regression/generate.ts | Removes plugin model file loading from schema generation |
| packages/testtools/src/schema.ts | Eliminates plugin module resolution and removes glob dependency |
| packages/runtime/tsup.config.ts | Removes build step that copied plugin.zmodel files |
| packages/runtime/test/utils.ts | Updates test utilities to use simplified document loading |
| packages/runtime/test/scripts/generate.ts | Removes plugin model file loading from test script |
| packages/runtime/src/plugins/policy/plugin.zmodel | Deletes the standalone plugin model file |
| packages/language/test/utils.ts | Removes plugin model loading from language tests |
| packages/language/res/stdlib.zmodel | Adds all policy attributes and functions to the standard library |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Claude finished @ymc9's task —— View job PR Review: Move Policy ZModel Back to StdlibReviewing this architectural refactoring that moves policy zmodel definitions from the runtime plugin system to the language stdlib. Review Tasks:
✅ Overall AssessmentThis is a well-executed architectural simplification that consolidates policy-related ZModel definitions from a separate plugin system into the main standard library. The refactoring reduces complexity while maintaining functionality. 📋 Detailed ReviewArchitectural Changes - Excellent
Code Quality - Good
Potential Issues - Minor
Performance Implications - Positive
Security Considerations - Good
Test Coverage Impact - Needs Verification
The test utility changes look correct, but comprehensive test verification is recommended. Breaking Changes
🎯 Recommendations
✅ ConclusionThis is a solid architectural improvement that simplifies the codebase without sacrificing functionality. The refactoring is well-executed with proper attention to preserving existing APIs and behavior. The consolidation of policy definitions into the standard library makes the system more maintainable and performant. Recommendation: ✅ Approve - This change improves the architecture and should be merged after confirming test suite passes. |
|
Claude finished @ymc9's task —— View job PR Review: Move Policy ZModel Back to StdlibReviewing this architectural refactoring that moves policy zmodel definitions from the runtime plugin system to the language stdlib. Review Tasks:
Overall Assessment: ✅ APPROVED - Well-executed refactoring with positive architectural benefitsThis PR successfully consolidates policy-related ZModel definitions from a plugin-based architecture back to the language stdlib, resulting in cleaner code and reduced complexity. Code Quality & Architecture 🏗️Strengths:
Clean Code Changes:
Bug Prevention & Issues 🐛✅ No Issues Found:
Potential Future Consideration:
Performance ⚡Improvements:
Security 🔒✅ No Security Concerns:
Test Coverage 🧪Maintained Coverage:
Files Updated Correctly:
Migration Quality 📦Excellent Content Migration:
Recommendations
SummaryThis is a well-executed architectural simplification that improves the codebase by:
The changes are consistent, complete, and maintain all existing functionality while simplifying the overall architecture. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (13)
packages/language/test/utils.ts (2)
12-12: Single‑arg loadDocument: OK; clean up temp fileAvoid leaking temp files. Wrap in try/finally and unlink after parsing.
export async function loadSchema(schema: string) { // create a temp file const tempFile = path.join(os.tmpdir(), `zenstack-schema-${crypto.randomUUID()}.zmodel`); - fs.writeFileSync(tempFile, schema); - const r = await loadDocument(tempFile); + fs.writeFileSync(tempFile, schema); + let r; + try { + r = await loadDocument(tempFile); + } finally { + fs.existsSync(tempFile) && fs.unlinkSync(tempFile); + }
25-25: Mirror the same temp cleanup hereApply the same try/finally unlink pattern to loadSchemaWithError.
export async function loadSchemaWithError(schema: string, error: string | RegExp) { // create a temp file const tempFile = path.join(os.tmpdir(), `zenstack-schema-${crypto.randomUUID()}.zmodel`); - fs.writeFileSync(tempFile, schema); - const r = await loadDocument(tempFile); + fs.writeFileSync(tempFile, schema); + let r; + try { + r = await loadDocument(tempFile); + } finally { + fs.existsSync(tempFile) && fs.unlinkSync(tempFile); + }Ensure your test runtime is Node ≥18 so global crypto.randomUUID() is available.
packages/language/res/stdlib.zmodel (3)
708-712: Fix future() doc: “before” → “after” updateName implies post‑update value; current text says “before”.
-/** - * Gets entities value before an update. Only valid when used in a "update" policy rule. - */ +/** + * Gets the entity's value after the update (the new value). Only valid inside an "update" policy rule. + */ function future(): Any { } @@@expressionContext([AccessPolicy])
671-680: Align @Allow(field) docs vs allowed operationsDoc says “read or update”, but completion hints include create/delete/all. Either restrict hints or broaden the doc.
-attribute @allow(_ operation: String @@@completionHint(["'create'", "'read'", "'update'", "'delete'", "'all'"]), _ condition: Boolean, _ override: Boolean?) +attribute @allow(_ operation: String @@@completionHint(["'read'", "'update'"]), _ condition: Boolean, _ override: Boolean?)If create/delete are intentionally supported at field level, update the doc comment accordingly instead.
689-696: Align @deny(field) similarlySame inconsistency as @Allow(field).
-attribute @deny(_ operation: String @@@completionHint(["'create'", "'read'", "'update'", "'delete'", "'all'"]), _ condition: Boolean) +attribute @deny(_ operation: String @@@completionHint(["'read'", "'update'"]), _ condition: Boolean)packages/runtime/test/scripts/generate.ts (1)
22-22: LGTM on single‑arg loadDocument; surface warnings for visibilityOptional: log non‑fatal warnings to ease triage.
-const result = await loadDocument(schemaPath); +const result = await loadDocument(schemaPath); +if (result.success && result.warnings.length) { + console.warn(`[loadDocument warnings] ${schemaPath}:`, result.warnings); +}packages/runtime/test/utils.ts (1)
116-116: LGTM; consider logging load warningsWhen falling back to loadDocument, propagate warnings for easier debugging.
-const r = await loadDocument(path.join(workDir, 'schema.zmodel')); +const r = await loadDocument(path.join(workDir, 'schema.zmodel')); +if (r.success && r.warnings.length) { + console.warn('[loadDocument warnings]:', r.warnings); +}packages/testtools/src/schema.ts (6)
59-61: Unify return shape of helpers (includemodelin in-place variant or document divergence)Today
generateTsSchemareturns{ workDir, schema, model }whilegenerateTsSchemaInPlacereturns{ workDir, schema }. Prefer consistent APIs to avoid caller surprises.If you want parity, adjust as below:
- return compileAndLoad(workDir); + return { ...(await compileAndLoad(workDir)), model: result.model };Also applies to: 86-88
69-71: Make dynamic import path cross‑platform (Windows-safe)ESM import of absolute Windows paths can fail; use
pathToFileURL.Apply:
+import { pathToFileURL } from 'node:url'; @@ - const module = await import(path.join(workDir, 'schema.js')); + const module = await import(pathToFileURL(path.join(workDir, 'schema.js')).href);Also applies to: 1-1
51-56: Prevent path escape in extraSourceFilesGuard against
../escapingworkDireven if test-only.- const filePath = path.resolve(workDir, `${fileName}.ts`); + const filePath = path.resolve(workDir, `${fileName}.ts`); + if (!filePath.startsWith(workDir + path.sep)) { + throw new Error(`Invalid extraSourceFiles entry: ${fileName}`); + } fs.mkdirSync(path.dirname(filePath), { recursive: true }); fs.writeFileSync(filePath, content);
40-41: Make prelude detection more robustAvoid false positives from comments/strings.
- const noPrelude = schemaText.includes('datasource '); + const noPrelude = /\bdatasource\s+\w+\s*\{/.test(schemaText);
44-46: Surface load warnings to aid debuggingEmit
result.warningsso tests reveal potential issues early.if (!result.success) { throw new Error(`Failed to load schema from ${zmodelPath}: ${result.errors}`); } + if (result.warnings?.length) { + console.warn(`Schema warnings:\n- ${result.warnings.join('\n- ')}`); + }And similarly for the in-place path:
if (!result.success) { throw new Error(`Failed to load schema from ${schemaPath}: ${result.errors}`); } + if (result.warnings?.length) { + console.warn(`Schema warnings:\n- ${result.warnings.join('\n- ')}`); + }Also applies to: 82-84
20-27: Prefer postgresql:// in datasource URL (matches Prisma docs/examples)Prisma accepts both schemes, but docs and examples use and recommend postgresql:// — update default to match.
- url = '${dbUrl ?? 'postgres://postgres:postgres@localhost:5432/db'}' + url = '${dbUrl ?? 'postgresql://postgres:postgres@localhost:5432/db'}'
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/language/res/stdlib.zmodel(1 hunks)packages/language/test/utils.ts(2 hunks)packages/runtime/src/plugins/policy/plugin.zmodel(0 hunks)packages/runtime/test/scripts/generate.ts(1 hunks)packages/runtime/test/utils.ts(2 hunks)packages/runtime/tsup.config.ts(0 hunks)packages/testtools/src/schema.ts(2 hunks)tests/regression/generate.ts(1 hunks)
💤 Files with no reviewable changes (2)
- packages/runtime/tsup.config.ts
- packages/runtime/src/plugins/policy/plugin.zmodel
🧰 Additional context used
📓 Path-based instructions (2)
{packages,samples,tests}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place packages only under
packages/,samples/, ortests/
Files:
packages/runtime/test/scripts/generate.tstests/regression/generate.tspackages/runtime/test/utils.tspackages/language/test/utils.tspackages/testtools/src/schema.tspackages/language/res/stdlib.zmodel
**/schema.ts
📄 CodeRabbit inference engine (CLAUDE.md)
The generated TypeScript schema should be named
schema.ts
Files:
packages/testtools/src/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/runtime/test/scripts/generate.tstests/regression/generate.tspackages/testtools/src/schema.ts
🧬 Code graph analysis (5)
packages/runtime/test/scripts/generate.ts (1)
packages/language/src/index.ts (1)
loadDocument(21-132)
tests/regression/generate.ts (1)
packages/language/src/index.ts (1)
loadDocument(21-132)
packages/runtime/test/utils.ts (1)
packages/language/src/index.ts (1)
loadDocument(21-132)
packages/language/test/utils.ts (1)
packages/language/src/index.ts (1)
loadDocument(21-132)
packages/testtools/src/schema.ts (1)
packages/language/src/index.ts (1)
loadDocument(21-132)
⏰ 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, postgresql)
- GitHub Check: build-test (20.x, sqlite)
🔇 Additional comments (4)
packages/runtime/test/utils.ts (1)
5-5: Import change looks goodSwitching to generateTsSchema simplifies setup.
tests/regression/generate.ts (1)
21-21: Approve — no remaining two-arg loadDocument callers found.Scanned repo: all loadDocument usages pass a single argument and no getPluginModels/getPluginModules usages were found. Locations: packages/cli/src/actions/action-utils.ts; packages/language/src/index.ts; packages/language/test/import.test.ts; packages/language/test/utils.ts; packages/runtime/test/scripts/generate.ts; packages/runtime/test/utils.ts; packages/testtools/src/schema.ts; tests/regression/generate.ts.
packages/testtools/src/schema.ts (2)
81-81: LGTM: consistent update to new loadDocument() signatureMatches the change above.
43-43: LGTM: switched to stdlib-only loadDocument()Call site now matches the simplified API. Ripgrep returned "No files were searched" — re-run to confirm no lingering plugin-discovery calls using one of these:
git grep -n -e 'getPluginModules' -e 'pluginModelFiles' || true
rg -n -C2 -S --hidden --no-ignore --glob '!/node_modules/' 'getPluginModules|pluginModelFiles' || true
Summary by CodeRabbit