chore: reenable post-update test cases

[ymc9]

Summary by CodeRabbit

• New Features

  • Enabled post-update access checks across models; added safeguards preventing owner changes after updates.
  • Simplified update rules by removing future()-based conditions.
  • Pet model now requires authentication for updates.

• Tests

  • Re-enabled multiple previously skipped suites and cases (Pet Store, Todo, nested/to-one, multi-id, toplevel operations).
  • Updated assertions to reflect new post-update and update policies.

• Documentation

  • Added a new TODO subtask: “Custom functions.”

[coderabbitai]

Walkthrough

Removes a runtime assertion in ExpressionTransformer._member, activates previously skipped policy tests, updates test policies to replace future() with post-update rules, and introduces new allow/deny rules in sample schemas (Pet and Todo/List/Space). Adds a TODO item for custom functions.

Changes

Cohort / File(s)	Summary
Policy transformer packages/runtime/src/plugins/policy/expression-transformer.ts	Removed a runtime assertion checking currNode as SelectQueryNode within _member during relation-branch iteration.
Migrated policy tests packages/runtime/test/policy/migrated/*	Unskipped multiple tests; replaced future()-based update guards with simpler update plus new post-update rules; adjusted expectations and some test data types. Files: multi-id-fields.test.ts, nested-to-one.test.ts, petstore-sample.test.ts, todo-sample.test.ts, toplevel-operations.test.ts.
Petstore schema packages/runtime/test/schemas/petstore/schema.zmodel, packages/runtime/test/schemas/petstore/schema.ts	Added @@allow('update', auth() != null) for Pet in zmodel and corresponding TS schema entry.
Todo/List/Space schema packages/runtime/test/schemas/todo/todo.zmodel, packages/runtime/test/schemas/todo/schema.ts	Simplified List update rule (removed future()); added @@deny('post-update', before().ownerId != ownerId) for Todo, List, Space; aligned TS schema with zmodel.
Docs/TODO TODO.md	Added a new TODO subtask: Custom functions under Access Policy.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• feat(policy): post-update policies #275 — Implements the same post-update/before() policy feature, overlapping with transformer and policy handling.
• fix(policy): relation access via this, more test cases #252 — Modifies ExpressionTransformer member handling; closely related to this PR’s change in _member.
• chore: migrate more v2 tests #254 — Touches the same runtime policy tests and related schema updates.

Poem

A rabbit taps keys with careful delight,
Snips one assert from the moonlit night.
Tests hop awake, post-update in tune,
Pets need auth, Todos change soon.
Before() whispers, policies align—
Carrots compiled, everything’s fine. 🥕✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The title “chore: reenable post-update test cases” accurately reflects the core purpose of the changeset, which is to reactivate previously skipped post-update tests across multiple policy and schema suites, and does so in a concise, conventional-style format that a reviewer can quickly understand.
Docstring Coverage	✅ Passed	No functions found in the changes. Docstring coverage check skipped.

✨ Finishing touches

• 📝 Generate Docstrings

🧪 Generate unit tests

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch chore/reenable-post-testys

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

🧪 Early access (Sonnet 4.5): enabled

We are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience.

Note:

• Public repositories are always opted into early access features.
• You can enable or disable early access features from the CodeRabbit UI or by updating the CodeRabbit configuration file.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[claude]

Claude finished @ymc9's task —— View job

---

PR Review: chore: reenable post-update test cases

Summary

This PR re-enables post-update policy test cases that were previously disabled, following the implementation of post-update policies in PR #275. The changes are well-structured and consistent with the codebase patterns.

Code Quality and Best Practices ✅

Positive aspects:

• Clean, focused changes that serve a specific purpose
• Consistent test patterns across all modified test files
• Proper use of ZenStack's policy system with @@allow('post-update', ...) directives
• Test schemas follow established patterns in the codebase
• Good separation of concerns between different test scenarios

Schema changes:

• packages/runtime/test/schemas/petstore/schema.zmodel:41: Added post-update policy for Pet model ensuring immutability of core fields during updates
• packages/runtime/test/schemas/todo/todo.zmodel:122,150: Added post-update policies preventing ownership changes in List and Todo models

Test Coverage ✅

Comprehensive test scenarios:

• Multi-ID field operations (multi-id-fields.test.ts)
• Nested relation operations (nested-to-one.test.ts)
• Top-level CRUD operations (toplevel-operations.test.ts)
• Complex business logic scenarios in todo and petstore samples

Test quality:

• Tests properly validate both successful and rejected scenarios
• Good use of toBeRejectedByPolicy() and toResolveTruthy() matchers
• Tests cover edge cases like ID updates and nested operations

Potential Issues ⚠️

1. TODO.md entry: Line 1 adds "- [ ] CLI" which seems unrelated to this PR's scope

2. Missing test documentation: While tests are comprehensive, there's no documentation explaining the specific post-update policy behaviors being tested

3. Schema synchronization: Need to ensure the generated TypeScript schemas (schema.ts files) are properly regenerated after ZModel changes

Performance Considerations ✅

• Post-update policies may add query overhead, but this is expected behavior
• Test database operations use in-memory SQLite, appropriate for testing
• No performance anti-patterns detected in the test code

Security Concerns ✅

Positive security aspects:

• Post-update policies provide important data integrity checks
• Proper authentication checks using auth() function
• Prevention of unauthorized ownership changes
• Field immutability enforcement (e.g., Pet model fields in petstore schema)

No security vulnerabilities identified

Recommendations

1. TODO.md: Remove the unrelated "- [ ] CLI" entry added on line 1
2. Documentation: Consider adding comments in test files explaining the specific post-update policy scenarios being validated
3. Schema regeneration: Verify that all generated schema.ts files are properly synchronized after ZModel changes

Conclusion

This is a solid PR that properly re-enables important test coverage for post-update policies. The implementation is consistent with ZenStack patterns and provides good validation of the post-update policy feature. The changes are minimal, focused, and well-executed.

Overall Rating: ✅ Approved - Minor cleanup suggestions but no blocking issues.

---

[Copilot]

Pull Request Overview

This PR re-enables post-update test cases by migrating from deprecated future() function to the post-update operation with before() function. The change allows testing of policy rules that validate data state transitions during updates.

• Replaces commented-out future() policies with @@deny('post-update', before().ownerId != ownerId) rules
• Updates test expectations from toBeRejectedNotFound() to toBeRejectedByPolicy() for policy violations
• Re-enables previously skipped test suites and individual test cases

Reviewed Changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
packages/runtime/test/schemas/todo/todo.zmodel	Adds post-update deny rules to prevent owner changes in List and Todo models
packages/runtime/test/schemas/todo/schema.ts	Updates compiled schema with new post-update deny attributes
packages/runtime/test/schemas/petstore/schema.zmodel	Adds update allow rule for authenticated users to Pet model
packages/runtime/test/schemas/petstore/schema.ts	Updates compiled schema with new update allow attribute
packages/runtime/test/policy/todo-sample.test.ts	Re-enables post-update test case
packages/runtime/test/policy/migrated/toplevel-operations.test.ts	Migrates from future() to post-update and updates test expectations
packages/runtime/test/policy/migrated/todo-sample.test.ts	Re-enables post-update test case
packages/runtime/test/policy/migrated/petstore-sample.test.ts	Re-enables entire test suite
packages/runtime/test/policy/migrated/nested-to-one.test.ts	Migrates nested update tests from future() to post-update
packages/runtime/test/policy/migrated/multi-id-fields.test.ts	Migrates multi-id field tests and fixes type inconsistencies
packages/runtime/src/plugins/policy/expression-transformer.ts	Removes invariant check for select query node
TODO.md	Adds custom functions task to todo list

---

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f531ad6 and 81bf438.

📒 Files selected for processing (12)

• TODO.md (1 hunks)
• packages/runtime/src/plugins/policy/expression-transformer.ts (0 hunks)
• packages/runtime/test/policy/migrated/multi-id-fields.test.ts (6 hunks)
• packages/runtime/test/policy/migrated/nested-to-one.test.ts (2 hunks)
• packages/runtime/test/policy/migrated/petstore-sample.test.ts (1 hunks)
• packages/runtime/test/policy/migrated/todo-sample.test.ts (1 hunks)
• packages/runtime/test/policy/migrated/toplevel-operations.test.ts (3 hunks)
• packages/runtime/test/policy/todo-sample.test.ts (1 hunks)
• packages/runtime/test/schemas/petstore/schema.ts (1 hunks)
• packages/runtime/test/schemas/petstore/schema.zmodel (1 hunks)
• packages/runtime/test/schemas/todo/schema.ts (2 hunks)
• packages/runtime/test/schemas/todo/todo.zmodel (2 hunks)

💤 Files with no reviewable changes (1)

• packages/runtime/src/plugins/policy/expression-transformer.ts

🧰 Additional context used

📓 Path-based instructions (3)

{packages,samples,tests}/**

📄 CodeRabbit inference engine (CLAUDE.md)

Place packages only under packages/, samples/, or tests/

Files:

• packages/runtime/test/schemas/todo/todo.zmodel
• packages/runtime/test/policy/todo-sample.test.ts
• packages/runtime/test/schemas/petstore/schema.zmodel
• packages/runtime/test/schemas/petstore/schema.ts
• packages/runtime/test/policy/migrated/toplevel-operations.test.ts
• packages/runtime/test/policy/migrated/nested-to-one.test.ts
• packages/runtime/test/schemas/todo/schema.ts
• packages/runtime/test/policy/migrated/multi-id-fields.test.ts
• packages/runtime/test/policy/migrated/todo-sample.test.ts
• packages/runtime/test/policy/migrated/petstore-sample.test.ts

**/schema.zmodel

📄 CodeRabbit inference engine (CLAUDE.md)

Name ZModel schema files schema.zmodel

Files:

• packages/runtime/test/schemas/petstore/schema.zmodel

**/schema.ts

📄 CodeRabbit inference engine (CLAUDE.md)

The generated TypeScript schema should be named schema.ts

Files:

• packages/runtime/test/schemas/petstore/schema.ts
• packages/runtime/test/schemas/todo/schema.ts

🧬 Code graph analysis (2)

packages/runtime/test/schemas/petstore/schema.ts (1)

packages/runtime/src/schema/expression.ts (1)

• ExpressionUtils (16-120)

packages/runtime/test/schemas/todo/schema.ts (1)

packages/runtime/src/schema/expression.ts (1)

• ExpressionUtils (16-120)

🪛 markdownlint-cli2 (0.18.1)

TODO.md

104-104: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)

⏰ 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, postgresql)
• GitHub Check: build-test (20.x, sqlite)

🔇 Additional comments (16)

packages/runtime/test/policy/todo-sample.test.ts (1)

386-444: LGTM! Post-update checks test successfully re-enabled.

The test correctly verifies post-update policy enforcement by attempting to change ownership on both List and Todo entities (including nested updates), all of which should be rejected by policy.

TODO.md (1)

104-104: New TODO item added for custom functions.

The addition tracks future work on custom functions in the access policy system, aligning with the PR's post-update policy improvements.

Note: The markdown linter flags the indentation (4 spaces vs. 2), but this matches the existing style in this file.

packages/runtime/test/policy/migrated/todo-sample.test.ts (1)

373-430: LGTM! Post-update checks test successfully re-enabled.

This migrated test correctly verifies the same post-update policy enforcement scenarios as the non-migrated version, ensuring owner changes are properly rejected by policy.

packages/runtime/test/policy/migrated/petstore-sample.test.ts (1)

5-44: LGTM! Pet Store test suite successfully re-enabled.

The test suite activation aligns with the PR objective and depends on the updated Pet schema with new authentication-based update permissions.

packages/runtime/test/schemas/petstore/schema.zmodel (1)

38-39: LGTM! Authentication-based update permission added.

The new rule requires authentication for Pet updates, working in conjunction with the existing post-update rule to ensure:

1. Only authenticated users can update pets
2. Only the orderId field can be changed (and only when previously null)

This properly enforces the business logic for marking pets as sold.

packages/runtime/test/policy/migrated/nested-to-one.test.ts (2)

200-200: LGTM! Test successfully re-enabled.

The test is now active and exercises the new post-update policy semantics.

---

218-219: Policy update aligns with post-update test semantics.

The simplified update guard (value > 1) and new post-update rule (value > 2) correctly enforce:

• Pre-update: current value must be > 1 to allow the update operation
• Post-update: final value must be > 2 to commit the change

Test expectations at lines 238 and 249 correctly validate denial and success scenarios.

packages/runtime/test/policy/migrated/toplevel-operations.test.ts (3)

136-136: LGTM! Test successfully re-enabled.

The test now validates top-level update operations with the new post-update policy semantics.

---

145-146: Policy update correctly implements post-update validation.

The simplified update rule (value > 1) and new post-update constraint (value > 2) align with the test scenarios at lines 159-167 (rejection) and lines 170-178 (success).

---

167-167: Correct expectation: policy rejection rather than not-found.

The change from toBeRejectedNotFound() to toBeRejectedByPolicy() properly reflects that the update fails due to the post-update policy constraint (value > 2 fails for value: 1), not because the record is missing.

packages/runtime/test/schemas/todo/schema.ts (2)

314-314: Post-update ownership protection added for List.

This generated rule correctly prevents ownership transfers by denying updates where before().ownerId != ownerId. Ensure the source .zmodel file contains the corresponding rule.

---

384-385: Post-update ownership protection added for Todo.

The new denial rule at line 385 prevents ownership transfers, matching the pattern applied to the List model. The modification at line 384 appears to be part of the existing space membership access rule. Verify both align with the source .zmodel.

packages/runtime/test/schemas/todo/todo.zmodel (2)

120-122: Post-update denial correctly replaces future()-based ownership protection.

The simplified update rule at line 120 combined with the new post-update denial at line 122 cleanly enforces the ownership immutability constraint. This approach is more explicit and aligns with the PR's migration away from future().

---

149-150: Post-update denial added for Todo ownership protection.

Mirrors the pattern applied to the List model, preventing ownership transfers via a post-update constraint. The comment at line 149 clearly documents the intent.

packages/runtime/test/policy/migrated/multi-id-fields.test.ts (2)

60-126: LGTM! Test re-enabled with correct policy logic.

The test correctly validates the new policy structure where @@allow('update', value > 0) checks the pre-update state and @@allow('post-update', value > 1) validates the post-update state. All test assertions align with these rules.

---

322-394: LGTM! Nested update test correctly implements new policy structure.

The test validates nested updates with the same policy rules as the top-level update test. The data type correction on lines 372 and 387 (y: 4 as number) correctly matches the Int type in the schema.