fix: format zod errors, issue with "this" member access #250
Conversation
WalkthroughIntroduces a Zod error formatting utility and integrates it into input validation. Adjusts auth() expression transformation for per-id member comparison. Updates tests for nested policy behaviors and error message expectations. Adds catalog dependency for zod-validation-error and silences Prisma push output in tests. 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
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: Fix zod errors and "this" member access ✅I've completed my comprehensive review of this pull request. Here's my detailed analysis: Review Checklist
🎯 SummaryThis PR addresses two key issues:
✅ Positive Aspects1. Clean Architecture
2. Error Handling Improvement
3. "This" Member Access Fix
4. Comprehensive Test Coverage
🔍 Technical AnalysisZod Error Formatting ( export function formatError(error: ZodError): string {
if ('_zod' in error) {
return fromError4(error).toString();
} else {
return fromError3(error).toString();
}
}
Expression Transformer Fix ( private makeOrAppendMember(other: Expression, fieldName: string): Expression {
if (ExpressionUtils.isMember(other)) {
return ExpressionUtils.member(other.receiver, [...other.members, fieldName]);
} else {
return ExpressionUtils.member(other, [fieldName]);
}
}
🛡️ Security Assessment
⚡ Performance Considerations
📋 Code Quality
🧪 Test CoverageThe extensive test additions are well-structured:
💡 Minor Suggestions
🏆 Overall AssessmentThis is a high-quality pull request that addresses important functionality issues:
The changes are focused, well-tested, and don't introduce any breaking changes. The fix for nested member access is particularly important for policy expressions involving complex object relationships. Recommendation: ✅ APPROVE |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes Zod error formatting and resolves an issue with "this" member access in policy expressions. The main changes include introducing a new dependency for better error formatting and fixing policy expression transformation to handle member access correctly.
- Added zod-validation-error dependency for improved error formatting
- Fixed policy expression transformer to properly handle "this" member access in auth comparisons
- Added comprehensive test coverage for nested CRUD operations
Reviewed Changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-workspace.yaml | Added zod-validation-error dependency to catalog |
| packages/runtime/package.json | Added zod-validation-error as runtime dependency |
| packages/runtime/src/utils/zod-utils.ts | New utility for formatting Zod errors with v3/v4 compatibility |
| packages/runtime/src/client/crud/validator.ts | Updated to use new error formatting utility |
| packages/runtime/src/plugins/policy/expression-transformer.ts | Fixed "this" member access issue in auth comparisons |
| packages/runtime/test/policy/crud/update.test.ts | Added extensive test coverage for nested operations |
| packages/runtime/test/client-api/group-by.test.ts | Updated test expectations for new error format |
| packages/runtime/test/utils.ts | Changed stdio setting for cleaner test output |
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: 0
🧹 Nitpick comments (5)
packages/runtime/package.json (1)
76-78: Dependency wiring LGTM; add CI matrix for Zod v4 to prevent regressions.You depend on zod via peer (v3|v4) but dev builds run with Zod v3. Add a CI job that runs runtime tests with Zod v4 to validate
zod-utilsformatting at runtime.Would you like me to draft a CI matrix step that installs zod@^4 in the runtime package and runs the test suite?
packages/runtime/test/utils.ts (1)
153-156: Silence Prisma push by default, but keep a debug escape hatch.Gating
stdiobehind an env flag preserves quiet tests while enabling ad‑hoc debugging.execSync('npx prisma db push --schema ./schema.prisma --skip-generate --force-reset', { cwd: workDir, - stdio: 'ignore', + stdio: process.env.DEBUG_PRISMA_PUSH ? 'inherit' : 'ignore', });packages/runtime/test/policy/crud/update.test.ts (3)
159-187: Duplicate test title; clarify owner vs non‑owner.Both tests are titled “works with to-one relation check owner side”. This one models the other ownership variant. Rename to avoid confusion.
- it('works with to-one relation check owner side', async () => { + it('works with to-one relation check non-owner side', async () => {
467-470: Make array assertions order-agnostic.
toMatchObjecton arrays is order-sensitive. UsearrayContainingto avoid flakiness.- ).resolves.toMatchObject({ - posts: [{ id: 1 }, { id: 2 }], - }); + ).resolves.toMatchObject({ + posts: expect.arrayContaining([{ id: 1 }, { id: 2 }]), + });
601-607: Same: use order-agnostic match for posts.- ).resolves.toMatchObject({ - posts: [{ title: 'Post 1' }, { title: 'Updated Title' }], - }); + ).resolves.toMatchObject({ + posts: expect.arrayContaining([{ title: 'Post 1' }, { title: 'Updated Title' }]), + });
📜 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 (8)
packages/runtime/package.json(1 hunks)packages/runtime/src/client/crud/validator.ts(2 hunks)packages/runtime/src/plugins/policy/expression-transformer.ts(2 hunks)packages/runtime/src/utils/zod-utils.ts(1 hunks)packages/runtime/test/client-api/group-by.test.ts(2 hunks)packages/runtime/test/policy/crud/update.test.ts(4 hunks)packages/runtime/test/utils.ts(1 hunks)pnpm-workspace.yaml(1 hunks)
🧰 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/src/utils/zod-utils.tspackages/runtime/src/client/crud/validator.tspackages/runtime/package.jsonpackages/runtime/test/utils.tspackages/runtime/test/policy/crud/update.test.tspackages/runtime/test/client-api/group-by.test.tspackages/runtime/src/plugins/policy/expression-transformer.ts
pnpm-workspace.yaml
📄 CodeRabbit inference engine (CLAUDE.md)
Use pnpm workspaces for the monorepo
Files:
pnpm-workspace.yaml
🧠 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: Database migrations use Prisma CLI under the hood
Applied to files:
packages/runtime/test/utils.ts
🧬 Code graph analysis (3)
packages/runtime/src/client/crud/validator.ts (2)
packages/runtime/src/client/errors.ts (1)
InputValidationError(4-8)packages/runtime/src/utils/zod-utils.ts (1)
formatError(8-14)
packages/runtime/test/policy/crud/update.test.ts (1)
packages/runtime/test/policy/utils.ts (1)
createPolicyTestClient(14-25)
packages/runtime/src/plugins/policy/expression-transformer.ts (2)
packages/runtime/src/schema/expression.ts (1)
ExpressionUtils(16-120)packages/sdk/src/schema/expression.ts (1)
Expression(1-10)
⏰ 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: claude-review
- GitHub Check: build-test (20.x)
🔇 Additional comments (5)
pnpm-workspace.yaml (1)
16-16: Catalog entry for zod-validation-error looks good.Aligns with the new runtime dependency and allows central version control via catalog.
packages/runtime/test/client-api/group-by.test.ts (1)
272-272: Updated regex expectations match new formatted messages.Good alignment with validator’s error formatting and message text.
Also applies to: 283-284
packages/runtime/src/client/crud/validator.ts (1)
9-9: Error formatting integration looks correct.Import + message composition read well and keep the original error as cause.
Also applies to: 189-190
packages/runtime/src/plugins/policy/expression-transformer.ts (1)
284-291: Fix for auth() equality against member paths is correct.Appending id-field members on the RHS via
makeOrAppendMemberproperly handles cases likeauth() == this.user. The composite-id handling via AND plus final NOT for!=is also right.Please confirm there’s test coverage for:
auth() == thiswith composite auth ids,auth() == this.relation.deepchains.Also applies to: 300-306
packages/runtime/src/utils/zod-utils.ts (1)
8-14: Keep current discriminator — '_zod' is the documented Zod v4 marker; no change required.
zod-validation-error v4 exposes v3/ and v4/ entry points and Zod's guidance for library authors is to import versioned subpaths and detect Zod‑4 via the "_zod" property, so the existing if ('_zod' in error) branch is appropriate.Likely an incorrect or invalid review comment.
Summary by CodeRabbit
Bug Fixes
Tests
Chores