fix(delegate,policy): several issues due to interaction between delegate and policy

[ymc9]

Summary by CodeRabbit

• New Features

  • Improved handling of delegated/base model fields in relations, enabling correct joins and policy evaluation across multi-model inheritance.

• Bug Fixes

  • More reliable policy enforcement for nested create/read/update/delete when relations reference delegated/base fields.
  • Prevents errors when evaluating policies on queries without a source table.

• Tests

  • Added comprehensive end-to-end and regression tests covering delegation, inheritance, relation checks, and nested operations.

• Chores

  • Updated test tooling build configuration to streamline type copying during builds.

[coderabbitai]

Walkthrough

Implements delegate base-field selection for relation access, replacing direct FK references with subselects when fields originate from delegated base models; refactors join construction to aggregate multi-path conditions; adds guards in policy handling; adjusts testtools build to copy types via tsup hook; introduces new e2e and regression tests for delegated policies.

Changes

Cohort / File(s)	Summary
Policy core: delegate base-field resolution and joins packages/plugins/policy/src/expression-transformer.ts, packages/plugins/policy/src/functions.ts, packages/plugins/policy/src/policy-handler.ts	Adds buildDelegateBaseFieldSelect and extends column ref generation to return subselects for delegate-origin fields; replaces single join with aggregated joinConditions covering ownership scenarios; computes op from args; adds no-from guard in transformSelectQuery; minor formatting tweak.
Build tooling: testtools packaging packages/testtools/package.json, packages/testtools/tsup.config.ts	Moves types copying from npm script to tsup onSuccess hook; removes copyfiles dependency; imports fs; drops clean option.
Tests: delegate policy behavior tests/e2e/orm/policy/delegate.test.ts, tests/regression/test/v2-migrated/issue-1930.test.ts	Adds e2e suite for delegated/inherited policies across models and relations; adds regression covering content visibility after related deletion via delegated checks.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~70 minutes

Possibly related PRs

• fix(policy): relation access via this, more test cases #252 — Similar adjustments to policy expression transformation for relation-backed selects and field resolution.
• getting more policy test cases to pass #3 — Extends expression transformation and handler logic with alias-aware select building and join/alias handling.
• fix(policy): relation field resolution issue #263 — Fixes relation-field resolution using related model metadata within policy transformation.

Poem

A rabbit peers through joins so tight,
Delegates whisper fields by night.
FK paths now weave and thread,
Subselects hop where columns led.
Tests burrow deep, the truths they find—
Policies align, all neatly signed. 🐇✨

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 clearly indicates that the pull request addresses fixes in the interaction between delegate models and policy enforcement, which aligns with the extensive changes to relation handling, query transformations, and tests all focused on delegate-policy integration.
Docstring Coverage	✅ Passed	No functions found in the changes. Docstring coverage check skipped.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/delegate-policy

---

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

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

[claude]

Claude finished @ymc9's task —— View job

---

PR Review Analysis

Completed comprehensive review of this PR that fixes delegate/policy interaction issues.

Todo List:

• Read and analyze all changed files
• Review code quality and best practices
• Check for potential bugs or issues
• Assess performance considerations
• Evaluate security concerns
• Analyze test coverage
• Provide comprehensive feedback

Summary

This PR fixes several important issues with the interaction between delegate models and policy enforcement in ZenStack. The changes primarily focus on ensuring that policies are correctly evaluated when delegate models are involved, particularly handling cases where relation fields originate from delegate base models.

Key Changes Analysis

1. Expression Transformer (expression-transformer.ts)

Improvements:

• ✅ Added proper handling for delegate base model fields in createColumnRef() (lines 650-667)
• ✅ Enhanced relation access logic to account for originModel in foreign key references (lines 592-598)
• ✅ Added buildDelegateBaseFieldSelect() method to properly select fields from delegate base models (lines 670-691)

Code Quality: ⭐⭐⭐⭐⭐

• Clean separation of concerns with dedicated helper methods
• Proper error handling with invariants
• Well-structured conditional logic for delegate vs non-delegate fields

2. Functions (functions.ts)

Improvements:

• ✅ Enhanced check() function to handle delegate base model relations (lines 44-86)
• ✅ Added buildBaseSelect() helper for delegate model field selection (lines 45-56)
• ✅ Proper handling of both owned and non-owned relations with delegate considerations

Code Quality: ⭐⭐⭐⭐⭐

• Excellent use of helper functions to reduce code duplication
• Clear logic flow for different relationship ownership scenarios
• Proper type safety with invariant checks

3. Policy Handler (policy-handler.ts)

Improvements:

• ✅ Minor but important addition of buildDelegateBaseFieldSelect method usage (line 685)
• ✅ Maintains consistency with other delegate handling patterns

Bug Analysis

Fixed Issues:

1. Delegate Field Resolution: Fixed cases where policies referencing fields from delegate base models weren't being resolved correctly
2. Relation Foreign Key Handling: Proper handling of foreign keys when relations are defined in delegate base models
3. Policy Evaluation Context: Ensures policy conditions can access fields from the correct model in delegate hierarchies

Potential Issues:

⚠️ Performance Consideration: The buildDelegateBaseFieldSelect() method creates subqueries for delegate base fields, which could impact performance with deep delegate hierarchies. However, this is likely necessary for correctness.

✅ No major bugs detected - The implementation looks solid with proper error handling and type safety.

Security Assessment

✅ No security concerns identified

• All database access goes through the existing Kysely query builder
• Proper use of parameterized queries via ValueNode.create()
• No SQL injection vulnerabilities introduced
• Policy enforcement remains intact and is actually strengthened

Performance Considerations

⚡ Generally good performance characteristics:

• Subqueries for delegate fields are necessary for correctness
• No unnecessary N+1 query patterns introduced
• Proper use of joins where applicable

⚠️ Minor concern: Deep delegate hierarchies could result in complex nested queries, but this is an acceptable trade-off for correctness.

Test Coverage

Excellent test coverage added:

1. delegate.test.ts - Comprehensive test suite covering:

   • ✅ Policy inheritance from delegate base models (lines 5-48)
   • ✅ Policies referencing base model fields (lines 50-105)
   • ✅ Policies referencing base model relations (lines 107-142)
   • ✅ Check function on delegate base relation fields (lines 144-175)

2. issue-1930.test.ts - Regression test covering:

   • ✅ Complex delegate + policy scenario with EntityContent delegate model
   • ✅ Proper policy evaluation with check() function on delegate relations

Test Quality: ⭐⭐⭐⭐⭐

• Tests cover edge cases and realistic scenarios
• Good use of both positive and negative test cases
• Proper cleanup and isolation between tests

Minor Issues

1. Package.json Changes

In packages/testtools/package.json:

• ✅ Dependency updates look reasonable
• ⚠️ Question: The removal of some dependencies - ensure they weren't actually needed

2. Build Configuration

In packages/testtools/tsup.config.ts:

• ✅ Addition of types.d.ts copying is fine
• Clean configuration changes

Recommendations

Strengths to Maintain:

1. ✅ Excellent error handling with invariants
2. ✅ Consistent patterns across the codebase
3. ✅ Comprehensive test coverage
4. ✅ Clear separation of delegate vs non-delegate logic

Suggestions:

1. Documentation: Consider adding inline comments explaining the delegate base field selection logic for future maintainers
2. Performance Monitoring: Monitor query performance with deep delegate hierarchies in production
3. Edge Cases: The current implementation handles the main scenarios well, but consider testing with very deep delegate chains (5+ levels)

Overall Assessment

Rating: ⭐⭐⭐⭐⭐ Excellent

This is a high-quality fix that properly addresses delegate/policy interaction issues. The code is well-structured, thoroughly tested, and maintains the existing architecture patterns. The implementation correctly handles the complex scenarios where policies need to evaluate fields and relations from delegate base models.

Recommendation: ✅ APPROVE

The PR successfully fixes the reported issues without introducing new bugs or security concerns. The comprehensive test suite provides confidence in the implementation's correctness.

[Copilot]

Pull Request Overview

This PR fixes several issues related to the interaction between delegate models and policy enforcement. The changes ensure that policies defined on base models are correctly inherited and enforced on delegate models, and that policy checks properly handle fields and relations defined in delegate base models.

Key Changes:

• Enhanced policy handling to correctly reference fields from delegate base models using nested select queries
• Added comprehensive test coverage for delegate-policy interactions including inheritance, field references, and relation checks
• Refactored build tooling to use tsup's onSuccess hook instead of copyfiles dependency

Reviewed Changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
tests/regression/test/v2-migrated/issue-1930.test.ts	Adds regression test verifying policy check on delegate relation fields
tests/e2e/orm/policy/delegate.test.ts	Adds comprehensive test suite for delegate-policy interactions
packages/testtools/tsup.config.ts	Replaces copyfiles with tsup onSuccess hook for file copying
packages/testtools/package.json	Removes copyfiles dependency from build script and devDependencies
packages/plugins/policy/src/policy-handler.ts	Adds null check for node.from before policy filter transformation
packages/plugins/policy/src/functions.ts	Refactors check function to handle delegate base model fields using nested selects
packages/plugins/policy/src/expression-transformer.ts	Updates column reference creation and relation joins to handle delegate base models

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]

Actionable comments posted: 1

🧹 Nitpick comments (1)

packages/testtools/package.json (1)

7-7: Build flow change is fine, but ensure types emission mapping is correct.

• tsup often emits only index.d.ts; your exports.require.types points to index.d.cts, which may not exist. Align it or configure generation accordingly.
• Also make sure the types copy step actually runs (see tsup.config.ts review).

Suggested change for types mapping if .d.cts is not emitted:

-                "types": "./dist/index.d.cts",
+                "types": "./dist/index.d.ts",

📜 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 8d9f296 and e99b37a.

⛔ Files ignored due to path filters (1)

• pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml

📒 Files selected for processing (7)

• packages/plugins/policy/src/expression-transformer.ts (2 hunks)
• packages/plugins/policy/src/functions.ts (1 hunks)
• packages/plugins/policy/src/policy-handler.ts (2 hunks)
• packages/testtools/package.json (1 hunks)
• packages/testtools/tsup.config.ts (2 hunks)
• tests/e2e/orm/policy/delegate.test.ts (1 hunks)
• tests/regression/test/v2-migrated/issue-1930.test.ts (1 hunks)

🧰 Additional context used

📓 Path-based instructions (2)

{packages,samples,tests}/**

📄 CodeRabbit inference engine (CLAUDE.md)

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

Files:

• packages/testtools/tsup.config.ts
• packages/plugins/policy/src/functions.ts
• tests/regression/test/v2-migrated/issue-1930.test.ts
• packages/plugins/policy/src/expression-transformer.ts
• packages/testtools/package.json
• tests/e2e/orm/policy/delegate.test.ts
• packages/plugins/policy/src/policy-handler.ts

tests/e2e/**

📄 CodeRabbit inference engine (CLAUDE.md)

End-to-end tests must live under tests/e2e/

Files:

• tests/e2e/orm/policy/delegate.test.ts

🧬 Code graph analysis (3)

tests/regression/test/v2-migrated/issue-1930.test.ts (1)

packages/testtools/src/client.ts (1)

• createPolicyTestClient (176-187)

packages/plugins/policy/src/expression-transformer.ts (1)

packages/plugins/policy/src/utils.ts (1)

• conjunction (49-67)

tests/e2e/orm/policy/delegate.test.ts (1)

packages/testtools/src/client.ts (1)

• createPolicyTestClient (176-187)

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

🔇 Additional comments (3)

packages/plugins/policy/src/policy-handler.ts (1)

300-303: Good guard for select without FROM.

Prevents applying policy filters to subquery-only selects. LGTM.

tests/regression/test/v2-migrated/issue-1930.test.ts (1)

1-76: No action needed; custom matchers are registered
vitest.config.ts’s setupFiles include '@zenstackhq/testtools', which via expect.extend in packages/testtools/src/vitest-ext.ts defines toResolveTruthy/toResolveNull.

packages/testtools/tsup.config.ts (1)

13-15: onSuccess callback functions are supported
Tsup’s onSuccess option accepts both shell-command strings and JS callbacks; your async onSuccess() will run as intended and copy types.d.ts. No change required.

Likely an incorrect or invalid review comment.