fix(policy): validator fixes, more tests migrated #258
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. WalkthroughRefactors policy error handling to use a typed RejectedByPolicyReason, makes FK assignment async with on-demand parent PK reads, tightens validator mutual-exclusivity and nested-update shapes, adds view metadata and excludes views from DB push, and introduces multiple policy-focused tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
|
Claude finished @ymc9's task —— View job Code Review Complete ✅Reviewing PR: fix(policy): validator fixes, more tests migrated Todo List:
Code Review SummaryThis PR implements validator fixes and migrates additional tests from the v2 codebase to v3. The changes span CRUD operations, input validation, policy handling, and test coverage improvements. Key Changes Analyzed1. Validator Improvements (
2. CRUD Operations Enhancement (
3. Policy Engine Updates (
4. Error Handling (
Code Quality Assessment ⭐⭐⭐⭐⭐Strengths:
Areas for attention:
Potential Issues 🔍Low Risk:
Recommendations:
Test Coverage Assessment ✅The PR adds 6 new comprehensive test files:
These tests cover critical functionality and edge cases, significantly improving the test suite. Performance Considerations ⚡
Security Review 🔒
Overall AssessmentThis is a well-executed PR that successfully migrates tests while improving core functionality. The validator fixes address real issues, and the migrated tests provide valuable coverage. The code quality is high with good error handling and type safety. Recommendation: ✅ APPROVE The changes are safe, well-tested, and improve the codebase. No blocking issues identified. |
There was a problem hiding this comment.
Pull Request Overview
This PR includes validator fixes and continues the migration of policy tests to a new testing framework. The changes enhance error handling in the policy system with more specific rejection reasons and fix several validation issues.
Key changes:
- Enhanced policy error handling with categorized rejection reasons
- Fixed validator issues related to schema validation and foreign key handling
- Added support for view models in schema generation and database operations
- Migrated several policy test suites to the new testing framework
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/sdk/src/ts-schema-generator.ts | Adds isView property assignment for view models in schema generation |
| packages/sdk/src/schema/schema.ts | Adds optional isView property to ModelDef type |
| packages/runtime/src/plugins/policy/errors.ts | Introduces RejectedByPolicyReason enum with categorized error types |
| packages/runtime/src/plugins/policy/policy-handler.ts | Updates error handling to use new reason codes |
| packages/runtime/src/client/helpers/schema-db-pusher.ts | Filters out view models from table creation |
| packages/runtime/src/client/crud/validator.ts | Fixes schema validation issues and adds missing refinements |
| packages/runtime/src/client/crud/operations/*.ts | Updates operations to use new error reason codes |
| packages/runtime/test/policy/migrated/*.test.ts | Migrated test suites with comprehensive policy validation scenarios |
Comments suppressed due to low confidence (1)
packages/runtime/src/client/crud/validator.ts:1
- The condition should check for 'where' in item instead of 'data' in item to maintain the original logic. This change breaks the detection of update operations that have explicit where clauses versus those that only have data.
import { invariant } from '@zenstackhq/common-helpers';
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 Code Review Complete ✅Reviewing PR: fix(policy): validator fixes, more tests migrated Todo List:
Code Review SummaryThis PR implements validator fixes and migrates additional tests from the v2 codebase to v3. The changes span CRUD operations, input validation, policy handling, and test coverage improvements. Key Changes Analyzed1. Validator Improvements (
2. CRUD Operations Enhancement (
3. Policy Engine Updates (
4. Error Handling (
Code Quality Assessment ⭐⭐⭐⭐⭐Strengths:
Areas for attention:
Potential Issues 🔍Low Risk:
Recommendations:
Test Coverage Assessment ✅The PR adds 6 new comprehensive test files:
These tests cover critical functionality and edge cases, significantly improving the test suite. Performance Considerations ⚡
Security Review 🔒
Overall AssessmentThis is a well-executed PR that successfully migrates tests while improving core functionality. The validator fixes address real issues, and the migrated tests provide valuable coverage. The code quality is high with good error handling and type safety. Recommendation: ✅ APPROVE The changes are safe, well-tested, and improve the codebase. No blocking issues identified. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/runtime/src/client/helpers/schema-db-pusher.ts (1)
41-74: EnsuresortModelsnever reintroduces views
Inschema-db-pusher.ts:41–74, restrict the topo‐sorted list to the original non‐view inputs and skip edges toisViewtargets. For example:
- At the top of
sortModels, captureconst allowed = new Set(models)- When building edges,
if (baseDef.isView) continueand similarly fortargetModel.isView- After
.reverse(), use.filter((m): m is ModelDef => m != null && allowed.has(m))Add a test: a non‐view model with a relation to a view must not attempt to create the view.
packages/runtime/src/client/crud/operations/base.ts (1)
437-470: Make buildFkAssignments pure; stop mutating caller and update createMany to reuse it
- Change buildFkAssignments to use a local copy (e.g.,
const resolvedParent = { ...(entity ?? {}) }) and update the error to:Field "<pk>" not found on parent or not readable by policy.- Do not Object.assign into the incoming
entity; returnparentFkFieldsand apply them to each created row.- Replace per-row FK reads from
fromRelation.ids[...]with a single call once per operation:
e.g.const parentFkFields = fromRelation ? await this.buildFkAssignments(kysely, fromRelation.model, fromRelation.field, fromRelation.ids) : undefined;thenObject.assign(newItem, parentFkFields)for each row.- Call sites that still read un-enriched
fromRelation.idsand must be updated: packages/runtime/src/client/crud/operations/base.ts lines 716–720 and 942–950 (also review 290–294 and 1563–1564 for related parent-update logic).
🧹 Nitpick comments (14)
packages/runtime/test/policy/migrated/update-many-and-return.test.ts (6)
45-51: Also assert updated fields to catch stale read-backsAdd an assertion that the returned row reflects the updated title to ensure
updateManyAndReturnreturns post-update values.const r = await db.post.updateManyAndReturn({ data: { title: 'foo' }, }); expect(r).toHaveLength(1); - expect(r[0].id).toBe(1); + expect(r[0].id).toBe(1); + expect(r[0].title).toBe('foo');
52-59: Verify non-updated rows stayed intactAfter expecting an empty result for
where: { id: 2 }, also assert the row remained unchanged to guard against unintended updates.await expect( db.post.updateManyAndReturn({ where: { id: 2 }, data: { title: 'foo' }, }), ).resolves.toHaveLength(0); + + // verify unchanged + await expect(rawDb.post.findUnique({ where: { id: 2 } })) + .resolves.toMatchObject({ title: 'world1' });
74-83: Avoid brittle string matching for policy reasonPrefer asserting a typed reason (e.g.,
RejectedByPolicyReason.ReadBackDenied) instead of the literal message to reduce coupling to error text.If the matcher supports reason codes, update to something like:
await expect(...).toBeRejectedByPolicy([RejectedByPolicyReason.ReadBackDenied]);
81-83: ReuserawDbfor consistencyYou already have
rawDb. Reuse it instead of callingdb.$unuseAll()again.- await expect(db.$unuseAll().post.findUnique({ where: { id: 1 } })).resolves.toMatchObject({ published: false }); + await expect(rawDb.post.findUnique({ where: { id: 1 } })).resolves.toMatchObject({ published: false });
85-119: Order-agnostic assertions for skipped testDon’t rely on array index ordering from
updateManyAndReturn. Assert byid, and prefertoHaveLength.- const r = await db.post.updateManyAndReturn({ - data: { title: 'foo' }, - }); - - expect(r.length).toBe(2); - expect(r[0].title).toBeTruthy(); - expect(r[1].title).toBeUndefined(); + const r = await db.post.updateManyAndReturn({ + data: { title: 'foo' }, + }); + + expect(r).toHaveLength(2); + const byId = Object.fromEntries(r.map((p: any) => [p.id, p])); + expect(byId[1].title).toBeTruthy(); + expect(byId[2].title).toBeUndefined();
85-86: Track unskip planAdd an issue reference to unskip once field-level policy read-back is finalized.
I can open a tracking issue and link it here if helpful.
packages/runtime/test/policy/migrated/toplevel-operations.test.ts (2)
76-85: Optionally assert rejection reason for read-back casesGiven the new RejectedByPolicyReason, consider asserting the reason (e.g., CANNOT_READ_BACK) if your matcher supports it to tighten the test.
223-224: Gate noisy SQL logging behind an env flagConsole logging in CI can be noisy. Consider enabling testLogger only when, e.g., process.env.DEBUG_SQL is set.
packages/runtime/test/policy/migrated/omit.test.ts (1)
51-51: Remove console.log from test code.Debug logging should not be committed to the test suite.
- console.log(found);packages/runtime/src/client/crud/validator.ts (1)
1051-1059: Consider extracting the refinement pattern into a helper method.Since the pattern of creating a schema, then applying both refinements is repeated across multiple methods, consider extracting it to reduce duplication.
You could add a helper method like:
private applySelectIncludeOmitRefinements(schema: ZodSchema): ZodSchema { schema = this.refineForSelectIncludeMutuallyExclusive(schema); schema = this.refineForSelectOmitMutuallyExclusive(schema); return schema; }Then use it consistently:
- let schema: ZodSchema = base.extend({ - select: this.makeSelectSchema(model).optional(), - omit: this.makeOmitSchema(model).optional(), - }); - schema = this.refineForSelectIncludeMutuallyExclusive(schema); - schema = this.refineForSelectOmitMutuallyExclusive(schema); - return schema; + let schema: ZodSchema = base.extend({ + select: this.makeSelectSchema(model).optional(), + omit: this.makeOmitSchema(model).optional(), + }); + return this.applySelectIncludeOmitRefinements(schema);packages/runtime/test/policy/migrated/todo-sample.test.ts (1)
374-432: Document why the post-update checks test is skipped.The skipped test references
future()support. Consider adding a more detailed comment or linking to an issue tracking this feature.- // TODO: `future()` support + // TODO: Enable when `future()` support is implemented + // This test validates post-update policy checks that require + // the ability to evaluate future state after mutations + // See: [link to issue if available]packages/runtime/src/client/crud/operations/base.ts (1)
1431-1437: Guard against null “data” payload in nested update.
typeof null === 'object', sodata = item.datacan becomenulland later triggerInternalError('data must be an object'). Add an explicit null check.- if ('data' in item && typeof item.data === 'object') { + if ('data' in item && item.data !== null && typeof item.data === 'object') { where = item.where; data = item.data; } else {packages/runtime/src/client/crud/operations/delete.ts (1)
6-6: Unify import path with other files.Other modules import from
../../../plugins/policy/errors. Consider aligning to reduce churn and accidental barrel export drift.-import { RejectedByPolicyError, RejectedByPolicyReason } from '../../../plugins/policy'; +import { RejectedByPolicyError, RejectedByPolicyReason } from '../../../plugins/policy/errors';packages/runtime/src/plugins/policy/policy-handler.ts (1)
346-356: Use a consistent reason for m2m pre-create failures.Pre-create checks gate “update” eligibility, not read-back. Using
CANNOT_READ_BACKfor A andNO_ACCESSfor B is inconsistent. RecommendNO_ACCESSfor both.- throw new RejectedByPolicyError( - m2m.firstModel as GetModels<Schema>, - RejectedByPolicyReason.CANNOT_READ_BACK, - `many-to-many relation participant model "${m2m.firstModel}" not updatable`, - ); + throw new RejectedByPolicyError( + m2m.firstModel as GetModels<Schema>, + RejectedByPolicyReason.NO_ACCESS, + `many-to-many relation participant model "${m2m.firstModel}" not updatable`, + );
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
packages/runtime/src/client/crud/operations/base.ts(4 hunks)packages/runtime/src/client/crud/operations/create.ts(2 hunks)packages/runtime/src/client/crud/operations/delete.ts(2 hunks)packages/runtime/src/client/crud/operations/update.ts(4 hunks)packages/runtime/src/client/crud/validator.ts(6 hunks)packages/runtime/src/client/helpers/schema-db-pusher.ts(1 hunks)packages/runtime/src/plugins/policy/errors.ts(1 hunks)packages/runtime/src/plugins/policy/policy-handler.ts(4 hunks)packages/runtime/test/policy/migrated/omit.test.ts(1 hunks)packages/runtime/test/policy/migrated/todo-sample.test.ts(1 hunks)packages/runtime/test/policy/migrated/toplevel-operations.test.ts(1 hunks)packages/runtime/test/policy/migrated/unique-as-id.test.ts(1 hunks)packages/runtime/test/policy/migrated/update-many-and-return.test.ts(1 hunks)packages/runtime/test/policy/migrated/view.test.ts(1 hunks)packages/sdk/src/schema/schema.ts(1 hunks)packages/sdk/src/ts-schema-generator.ts(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/sdk/src/ts-schema-generator.tspackages/runtime/src/plugins/policy/errors.tspackages/runtime/test/policy/migrated/view.test.tspackages/runtime/src/client/crud/operations/base.tspackages/runtime/test/policy/migrated/update-many-and-return.test.tspackages/runtime/src/client/helpers/schema-db-pusher.tspackages/sdk/src/schema/schema.tspackages/runtime/test/policy/migrated/omit.test.tspackages/runtime/test/policy/migrated/todo-sample.test.tspackages/runtime/test/policy/migrated/toplevel-operations.test.tspackages/runtime/test/policy/migrated/unique-as-id.test.tspackages/runtime/src/client/crud/operations/create.tspackages/runtime/src/client/crud/operations/delete.tspackages/runtime/src/client/crud/validator.tspackages/runtime/src/plugins/policy/policy-handler.tspackages/runtime/src/client/crud/operations/update.ts
**/schema.ts
📄 CodeRabbit inference engine (CLAUDE.md)
The generated TypeScript schema should be named
schema.ts
Files:
packages/sdk/src/schema/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.zmodel : Name ZModel schema files `schema.zmodel`
Applied to files:
packages/runtime/src/client/crud/validator.ts
🧬 Code graph analysis (12)
packages/runtime/test/policy/migrated/view.test.ts (1)
packages/runtime/test/policy/utils.ts (1)
createPolicyTestClient(14-25)
packages/runtime/src/client/crud/operations/base.ts (5)
packages/runtime/src/plugins/policy/policy-handler.ts (1)
kysely(59-61)packages/runtime/src/client/executor/zenstack-query-executor.ts (1)
kysely(60-62)packages/runtime/src/client/query-builder.ts (1)
ToKysely(19-19)packages/sdk/src/schema/schema.ts (1)
GetModels(109-109)packages/runtime/src/client/errors.ts (1)
QueryError(13-17)
packages/runtime/test/policy/migrated/update-many-and-return.test.ts (1)
packages/runtime/test/policy/utils.ts (1)
createPolicyTestClient(14-25)
packages/runtime/src/client/helpers/schema-db-pusher.ts (1)
packages/sdk/src/schema/schema.ts (1)
ModelDef(20-37)
packages/runtime/test/policy/migrated/omit.test.ts (1)
packages/runtime/test/policy/utils.ts (1)
createPolicyTestClient(14-25)
packages/runtime/test/policy/migrated/todo-sample.test.ts (2)
packages/runtime/test/policy/utils.ts (1)
createPolicyTestClient(14-25)packages/runtime/src/client/crud/operations/base.ts (1)
schema(91-93)
packages/runtime/test/policy/migrated/toplevel-operations.test.ts (2)
packages/runtime/test/policy/utils.ts (1)
createPolicyTestClient(14-25)packages/runtime/test/utils.ts (1)
testLogger(203-205)
packages/runtime/test/policy/migrated/unique-as-id.test.ts (1)
packages/runtime/test/policy/utils.ts (1)
createPolicyTestClient(14-25)
packages/runtime/src/client/crud/operations/create.ts (1)
packages/runtime/src/plugins/policy/errors.ts (1)
RejectedByPolicyError(24-32)
packages/runtime/src/client/crud/operations/delete.ts (1)
packages/runtime/src/plugins/policy/errors.ts (1)
RejectedByPolicyError(24-32)
packages/runtime/src/plugins/policy/policy-handler.ts (2)
packages/runtime/src/plugins/policy/errors.ts (1)
RejectedByPolicyError(24-32)packages/sdk/src/schema/schema.ts (1)
GetModels(109-109)
packages/runtime/src/client/crud/operations/update.ts (2)
packages/runtime/src/plugins/policy/errors.ts (1)
RejectedByPolicyError(24-32)packages/runtime/src/client/query-utils.ts (1)
getIdValues(166-172)
⏰ 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 (25)
packages/runtime/test/policy/migrated/update-many-and-return.test.ts (4)
1-2: Confirm custom matcher registrationThe test relies on
toBeRejectedByPolicy. Ensure the matcher is registered globally for Vitest in this workspace; otherwise, add the setup import.
16-25: Clarify update policy evaluation timingThe test assumes
@@allow('update', contains(title, 'hello'))is checked against the pre-update state. Please confirm this is the intended semantics; otherwise, the first update to{ title: 'foo' }might inadvertently lock itself out.
31-36: Explicit IDs with autoincrement: provider compatibilityYou set
idexplicitly for models with@default(autoincrement()). Verify this is supported across all providers/environments your test matrix uses (e.g., SQLite, PostgreSQL), to avoid portability flakes.
4-83: Solid coverage of model-level policy + read-back semanticsThe test cleanly validates selective updates, policy-filtered
where, and read-back denial while confirming side effects via raw access. Nice.packages/runtime/src/client/helpers/schema-db-pusher.ts (1)
32-34: Filtering out views before topo sort — good first stepThis avoids attempting to create tables for views in the happy path.
packages/sdk/src/ts-schema-generator.ts (1)
314-315: Emit isView: true for views — confirmeddm.isView is present on the generated AST/schema types (packages/language/src/generated/ast.ts; packages/sdk/src/schema/schema.ts), so the conditional is safe. Consider gating generation of Create/Update/Delete inputs for views to keep views read-only.
packages/sdk/src/schema/schema.ts (1)
36-37: isView flag addition looks good; ensure type alignment across packagesThe new optional flag is sensible. Please verify that the re-exported SchemaDef/ModelDef in @zenstackhq/runtime/schema also includes isView; otherwise generated code using “satisfies SchemaDef” will error.
Run:
Optional: if you’ll frequently branch on views in types, consider adding a utility like:
export type GetViewModels<Schema extends SchemaDef> = keyof { [K in GetModels<Schema> as Schema['models'][K]['isView'] extends true ? K : never]: true };packages/runtime/test/policy/migrated/omit.test.ts (1)
4-56: LGTM! Test coverage for omit semantics under policy constraints is comprehensive.The test effectively validates that:
- Field-level
omitworks correctly with policy rules- Nested relation field omission respects policy constraints
- Both
includewith nestedomitandselectwith nestedomitproduce consistent resultspackages/runtime/src/plugins/policy/errors.ts (2)
4-19: Well-structured enum for policy rejection reasons.The typed
RejectedByPolicyReasonenum provides clear semantic distinction between different rejection scenarios, improving error handling throughout the codebase.
24-32: Good improvement to error constructor signature.The change from optional
stringto requiredRejectedByPolicyReason(with a sensible default) ensures consistent error categorization across the codebase. The parameter order fix (reason -> message) is also correct.packages/runtime/test/policy/migrated/unique-as-id.test.ts (2)
5-52: LGTM! Comprehensive test coverage for unique field scenarios.The test effectively validates policy enforcement with unique fields as identifiers, including edge cases where nested creates may succeed but not be readable due to policy constraints.
159-214: Excellent coverage of nested update operations with unique constraints.The tests thoroughly validate both to-many and to-one nested updates, ensuring that operations respect unique field constraints and policy rules while maintaining data integrity.
packages/runtime/src/client/crud/validator.ts (3)
767-776: Good refactoring to apply both mutual exclusivity refinements.The consistent pattern of applying both
refineForSelectIncludeMutuallyExclusiveandrefineForSelectOmitMutuallyExclusiveensures proper validation of query arguments.
939-939: Making nested update "where" clause optional is correct.This change aligns with Prisma's behavior where the "where" clause in nested updates can be optional when the relation is already uniquely identified by the parent context.
Also applies to: 947-947
1031-1041: Consistent application of mutual exclusivity refinements across CRUD operations.The systematic refactoring to use mutable
ZodSchemavariables and apply both refinements improves code consistency and maintainability.Also applies to: 1062-1073, 1176-1184
packages/runtime/test/policy/migrated/todo-sample.test.ts (2)
1-503: Excellent comprehensive test suite for policy enforcement in a realistic domain model.The test suite effectively validates:
- User isolation and space-based access control
- Create operations with read-back policy enforcement
- Private vs. public resource access patterns
- Nested relation queries respecting policies
- Cross-user permissions within shared spaces
The helper function
createSpaceAndUsersprovides good test data isolation.
31-33: Good use of detailed policy rejection assertions.Testing for specific rejection messages like "result is not allowed to be read back" helps ensure the new
RejectedByPolicyReason.CANNOT_READ_BACKis working correctly.packages/runtime/src/client/crud/operations/base.ts (1)
282-287: Good: FK assignment now resolves missing parent referenced keys in-place.Awaiting
buildFkAssignments(kysely, ...)here is the right move for relations referencing non-id parent fields.packages/runtime/src/client/crud/operations/create.ts (1)
42-47: Reasoned policy error on unreadable read-back looks good.Throwing
RejectedByPolicyError(model, CANNOT_READ_BACK, ...)aligns with update/delete behavior.packages/runtime/src/client/crud/operations/delete.ts (1)
37-43: Reasoned policy error on unreadable read-back after delete.Consistent with create/update; good.
packages/runtime/src/client/crud/operations/update.ts (3)
51-55: Reasoned policy error when update succeeds but read-back is blocked.This matches the new policy semantics.
78-101: Transactional updateManyAndReturn with read-back parity check is solid.Single transaction, then throw
CANNOT_READ_BACKif any updated rows are unreadable. Good.
132-137: Upsert read-back policy handling is consistent.Good use of reasoned policy error for unreadable results.
packages/runtime/src/plugins/policy/policy-handler.ts (2)
69-73: Non-CRUD rejection now reasoned (OTHER).Looks correct.
113-117: Mutation read-back enforcement withCANNOT_READ_BACK.Matches CRUD handlers; good.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/runtime/test/policy/migrated/toplevel-operations.test.ts (4)
27-29: DRY: cache raw clientYou call
$unuseAll()multiple times. Cache it once (e.g.,const raw = db.$unuseAll()) for clarity.Also applies to: 236-236, 243-243, 247-247, 255-255
243-243: Fix: awaiting expect on a Promise with a non-async matcher
await expect(promise).toBeTruthy()won’t wait for resolution. Use the async-aware matcher you already use elsewhere.Apply this diff:
- await expect(db.$unuseAll().model.findUnique({ where: { id: '2' } })).toBeTruthy(); + await expect(db.$unuseAll().model.findUnique({ where: { id: '2' } })).toResolveTruthy();
224-225: Noisy logs in CIPassing
{ log: testLogger }will print SQL in normal runs. Consider gating behind an env flag to keep CI output clean.
137-210: Skip block is fine; optionally link a tracking issueLeaving it skipped with a brief TODO is okay. Consider referencing an issue ID to track enablement.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/runtime/test/policy/migrated/toplevel-operations.test.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/runtime/test/policy/migrated/toplevel-operations.test.ts
🧬 Code graph analysis (1)
packages/runtime/test/policy/migrated/toplevel-operations.test.ts (2)
packages/runtime/test/policy/utils.ts (1)
createPolicyTestClient(14-25)packages/runtime/test/utils.ts (1)
testLogger(203-205)
⏰ 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). (1)
- GitHub Check: build-test (20.x)
🔇 Additional comments (5)
packages/runtime/test/policy/migrated/toplevel-operations.test.ts (5)
19-31: Read-back semantics validated — good coverageCreate is rejected by policy while the raw client can still see the record; this precisely exercises read-back denial behavior. Nice.
67-95: Write/update/upsert flows look correctDenials, not-found, updateMany(count=0), and upsert(create path) assertions align with the declared policies.
Also applies to: 96-106, 107-115, 116-125, 126-135
19-26: Optional: assert rejection reason for read-backIf your matcher supports it, assert the specific RejectedByPolicyReason for these cases to lock in semantics (e.g., “read-back denied”).
Would you like me to update these assertions to check the reason code as well?
Also applies to: 245-246
1-51: Overall: solid, self-contained read testsGood mix of negative/positive paths, including find* and *OrThrow variants.
1-260: Sweep for similar async-expect patternsThere may be other occurrences of
await expect(promise).toBe...in the repo. Run this to catch them and convert to.resolvesor customtoResolve*matchers.
Summary by CodeRabbit
New Features
Bug Fixes
Tests