feat(policy): add optional error code to @@allow/@@deny attributes#2636
Open
Azzerty23 wants to merge 23 commits intozenstackhq:devfrom
Open
feat(policy): add optional error code to @@allow/@@deny attributes#2636Azzerty23 wants to merge 23 commits intozenstackhq:devfrom
Azzerty23 wants to merge 23 commits intozenstackhq:devfrom
Conversation
Adds an optional third `errorCode` string argument to @@Allow, @@deny, @Allow, and @deny policy attributes. When a create or post-update operation is rejected, the matching rule's code is surfaced on ORMError.policyCode so callers can handle specific policy violations without parsing error messages. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds optional per-policy error codes: extends policy DSL to accept an ChangesPolicy error codes + diagnostics
Runtime plumbing & options
Error surface & test support
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
…t-match only Changes `ORMError.policyCode` to `policyCodes: string[]` so every rule that contributed to a rejection (deny rules that fired, allow rules that failed) is reported. The diagnostic query path is also collapsed from N sequential per-rule round-trips into a single batched query with one EXISTS column per coded policy, which is both faster and correct for the multi-code case. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds a `fetchPolicyCodes` flag to `PolicyPluginOptions` (plugin-level default) and as a per-query arg on `$create`/`$update`, using AsyncLocalStorage to bridge the flag into the Kysely executor. Set to `false` to skip the extra diagnostic query when error codes are not needed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
Author
|
Ok, after thinking more, I think I found a respectable balance : like you said @ymc9 in the original issue, when a policy check fails, a second diagnostic query collects the error codes of all violated policies. Could be done in one round-trip with CTE-based approach but add complexity for all queries (even succeed ones). EDIT: for better compatibility I replaced |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/language/src/validators/attribute-application-validator.ts`:
- Around line 284-286: The validator currently accepts an errorCode argument for
field-level `@allow/`@deny silently; update _checkFieldLevelPolicy to detect when
attr.args[2] (passed into validateCustomErrorCode) is present for field-level
policies and emit a warning instead of silently accepting it: inside
_checkFieldLevelPolicy (the function validating operations
['read','update','all']) check if attr.args[2] exists and, since field-level
policies cannot surface error codes for non-create/post-update operations, call
the existing warning/logging path (or add one) to notify the user that errorCode
on field-level policies is ignored; keep calling validateCustomErrorCode to
preserve format checks but ensure a warning is emitted at this call site when
attr.args[2] is provided for field-level `@allow/`@deny.
- Around line 288-308: The policy-handler currently only extracts string error
codes via ExpressionUtils.isLiteral(... && typeof ... === 'string') which
ignores enum references; update the extraction in policy-handler.ts to also
accept ReferenceExpr enum references by checking if the expression is a
ReferenceExpr and isEnumFieldReference(referenceExpr) is true, and then derive
the code string from that reference (use the ReferenceExpr's name/target
components to build the same code identifier the validator expects) so the
downstream .filter((p) => p.code) will see a non-undefined code for enum values;
reference the existing symbols ExpressionUtils.isLiteral, ReferenceExpr, and
isEnumFieldReference when locating where to add this additional branch.
In `@packages/plugins/policy/src/policy-handler.ts`:
- Around line 178-187: The fast-fail branch currently only gathers codes from
unconditional deny rules (using getModelPolicies and isTrueExpr) and omits coded
allow-rule violations; fix by also collecting allow-rule codes and passing the
combined set to createRejectedByPolicyError: inspect
getModelPolicies(mutationModel, 'create') for policies with kind === 'allow' and
p.code (or use tryGetConstantPolicy to determine per-policy constant evaluation
if available), collect those allow codes that are not satisfied by the current
constant check, merge them with constantDenyCodes, and pass the merged
policyCodes array into createRejectedByPolicyError so callers receive the full
policyCodes set.
- Around line 1113-1149: The allow-policy diagnostic currently uses EXISTS(rows
WHERE allow_condition) and then negates it, which misreports batched post-update
failures; instead build the EXISTS expression to detect violating rows for allow
rules. Update the selections construction so that for policy.kind === 'allow'
you pass the negated condition into buildInnerExists (i.e., EXISTS(rows WHERE
NOT(allow_condition))) while keeping deny policies as-is; reference
compilePolicyCondition, buildInnerExists, codedPolicies and selections so you
can locate and change the conditional that creates SelectionNode entries,
leaving evaluatePolicyDiagnostics filtering logic unchanged.
In `@tests/e2e/orm/policy/crud/error-codes.test.ts`:
- Line 71: The test currently only checks rejection reason via
toBeRejectedByPolicy (e.g., await expect(db.foo.create({ data: { x: 0 }
})).toBeRejectedByPolicy(undefined, undefined)) but doesn’t assert that no
policy codes were returned; update the assertion to also explicitly assert the
absence of policyCodes on the rejection (for example, capture the thrown error
from db.foo.create and assert error.policyCodes is undefined or empty) and apply
the same change to the other occurrences noted (roughly the block around the
386-447 range); keep using the same matcher toBeRejectedByPolicy for the reason
check and add a separate assertion for policyCodes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2a92594a-ab5c-4567-9ecf-13efe7a9dbb9
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
packages/language/src/validators/attribute-application-validator.tspackages/orm/src/client/errors.tspackages/plugins/policy/package.jsonpackages/plugins/policy/plugin.zmodelpackages/plugins/policy/src/options.tspackages/plugins/policy/src/plugin.tspackages/plugins/policy/src/policy-handler.tspackages/plugins/policy/src/types.tspackages/plugins/policy/src/utils.tspackages/plugins/policy/tsconfig.jsonpackages/testtools/src/types.d.tspackages/testtools/src/vitest-ext.tstests/e2e/orm/policy/crud/error-codes.test.ts
… errorCode support
- Fix bug: allow rules in diagnostic queries now use EXISTS(NOT condition)
so a batch violation is detected even when some rows pass (previously
NOT EXISTS(condition) was used, which only fired when ALL rows failed)
- Simplify evaluatePolicyDiagnostics: negate at build time so the filter
is a uniform row[`$c${i}`] check instead of kind-branching
- Remove errorCode param from field-level @allow/@deny — codes are not
surfaced at runtime for field-level policies, so the grammar and
validator no longer accept them
- Remove the 200-char length limit check from validateCustomErrorCode
- Add 3 new tests: batch updateMany allow violation, enum code on
post-update, mixed enum + string codes
- Tighten fetchPolicyCodes opt-out assertions to toBeRejectedByPolicy(undefined, [])
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/plugins/policy/src/policy-handler.ts (1)
178-187:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the constant-deny shortcut aligned with the normal create diagnostics.
This branch still synthesizes
policyCodeslocally, sofetchPolicyCodes: falseis ignored here, and any codedallowrules that also fail are omitted because only unconditionaldeny(true)codes are collected. Please only use this shortcut when codes are disabled; otherwise fall back to the regular pre-create check so create rejections produce the samepolicyCodesset everywhere.Proposed fix
} else if (constCondition === false) { - const policies = this.getModelPolicies(mutationModel, 'create'); - const constantDenyCodes = policies - .filter((p) => p.kind === 'deny' && this.isTrueExpr(p.condition) && p.code) - .map((p) => p.code!); - throw createRejectedByPolicyError( - mutationModel, - RejectedByPolicyReason.NO_ACCESS, - undefined, - constantDenyCodes, - ); + if (this.options.fetchPolicyCodes === false) { + throw createRejectedByPolicyError(mutationModel, RejectedByPolicyReason.NO_ACCESS); + } + + await this.enforcePreCreatePolicy(node, mutationModel, isManyToManyJoinTable, proceed); + return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/plugins/policy/src/policy-handler.ts` around lines 178 - 187, The code currently short-circuits to collect only unconditional deny codes via getModelPolicies + isTrueExpr and throws createRejectedByPolicyError with undefined policyCodes; change this so the unconditional-deny shortcut is only used when fetchPolicyCodes is explicitly disabled (e.g. this.fetchPolicyCodes === false or equivalent flag on the handler); otherwise invoke the regular pre-create evaluation path used elsewhere in this class (the same routine that computes the full policyCodes set for creates) and throw createRejectedByPolicyError with that computed policyCodes and the appropriate deny codes, keeping mutationModel and RejectedByPolicyReason.NO_ACCESS as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/plugins/policy/src/policy-handler.ts`:
- Around line 178-187: The code currently short-circuits to collect only
unconditional deny codes via getModelPolicies + isTrueExpr and throws
createRejectedByPolicyError with undefined policyCodes; change this so the
unconditional-deny shortcut is only used when fetchPolicyCodes is explicitly
disabled (e.g. this.fetchPolicyCodes === false or equivalent flag on the
handler); otherwise invoke the regular pre-create evaluation path used elsewhere
in this class (the same routine that computes the full policyCodes set for
creates) and throw createRejectedByPolicyError with that computed policyCodes
and the appropriate deny codes, keeping mutationModel and
RejectedByPolicyReason.NO_ACCESS as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dd83e4ef-ed0a-4af9-bdca-21ec8b9ba3a2
📒 Files selected for processing (5)
packages/language/src/validators/attribute-application-validator.tspackages/plugins/policy/plugin.zmodelpackages/plugins/policy/src/policy-handler.tspackages/testtools/src/vitest-ext.tstests/e2e/orm/policy/crud/error-codes.test.ts
✅ Files skipped from review due to trivial changes (2)
- tests/e2e/orm/policy/crud/error-codes.test.ts
- packages/plugins/policy/plugin.zmodel
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/testtools/src/vitest-ext.ts
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/e2e/orm/policy/crud/error-codes.test.ts (1)
377-397: ⚡ Quick winRemove the duplicated enum post-update test case.
This block is a verbatim duplicate of Line 355-375 (same name + same body), which adds noise and makes failures ambiguous in reports.
Proposed cleanup
- it('surfaces code from enum value on post-update violation', async () => { - const db = await createPolicyTestClient( - ` - enum PolicyCode { - NEGATIVE_AFTER_UPDATE - } - - model Foo { - id Int `@id` - x Int - @@allow('create,read,update', true) - @@deny('post-update', x <= 0, NEGATIVE_AFTER_UPDATE) - } - `, - ); - await db.foo.create({ data: { id: 1, x: 1 } }); - await expect(db.foo.update({ where: { id: 1 }, data: { x: -1 } })).toBeRejectedByPolicy(undefined, [ - 'NEGATIVE_AFTER_UPDATE', - ]); - await expect(db.foo.update({ where: { id: 1 }, data: { x: 2 } })).resolves.toMatchObject({ x: 2 }); - });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/orm/policy/crud/error-codes.test.ts` around lines 377 - 397, The test "surfaces code from enum value on post-update violation" is duplicated; remove the redundant test block (the second occurrence that uses createPolicyTestClient with enum PolicyCode and the db.foo.create/db.foo.update assertions) so only the original test remains—locate the duplicate by the test title and references to createPolicyTestClient, PolicyCode, and db.foo.update and delete that repeated it(...) block to avoid ambiguous/noisy test reports.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/e2e/orm/policy/crud/error-codes.test.ts`:
- Around line 377-397: The test "surfaces code from enum value on post-update
violation" is duplicated; remove the redundant test block (the second occurrence
that uses createPolicyTestClient with enum PolicyCode and the
db.foo.create/db.foo.update assertions) so only the original test remains—locate
the duplicate by the test title and references to createPolicyTestClient,
PolicyCode, and db.foo.update and delete that repeated it(...) block to avoid
ambiguous/noisy test reports.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5eef659e-6392-41f9-95a2-e3ad6b5cf19e
📒 Files selected for processing (1)
tests/e2e/orm/policy/crud/error-codes.test.ts
Adds post-check logic that runs when 0 rows are affected by an update or delete, distinguishing "row not found" from "denied by policy" and collecting matching policy codes in a single combined diagnostic query. Extends fetchPolicyCodes query-level override to $delete, and adds corresponding E2E tests. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/plugins/policy/src/policy-handler.ts (1)
261-317:postModelLevelCheckadds a SELECT round-trip for every 0-rows update/delete — including genuine "not found" cases.The
isTrueNodeearly-return (line 271) avoids the extra query when no policies exist. But when any policy is present, every 0-rows UPDATE/DELETE — whether due to policy denial or a plain "record not found" — pays the cost of aSELECT EXISTS(…)before the handler can determine which case applies. For workloads where "record not found" is the common path (e.g., concurrent delete of already-deleted rows), this doubles the DB round-trips for those calls.If
fetchPolicyCodesdiagnostics are not needed, you can already setfetchPolicyCodes: falseglobally to skip code collection, but the existence check still runs. Consider exposing a separate opt-out (e.g.,detectPolicyDenial: false) for cases where callers are happy with a silent 0-row result.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/plugins/policy/src/policy-handler.ts` around lines 261 - 317, postModelLevelCheck currently issues an extra SELECT EXISTS whenever any model-level policy exists, even when callers don't need denial detection; add a new boolean option (e.g., detectPolicyDenial) defaulting to true and short-circuit the function when it's false. Specifically, update the method postModelLevelCheck to return immediately if this.options.detectPolicyDenial === false (place this check after the existing isTrueNode(modelLevelFilter) check), and ensure callers can set options.detectPolicyDenial to false to avoid the EXISTS/query and coded policy evaluation; keep existing behavior when the flag is true and document the new option default.tests/e2e/orm/policy/crud/error-codes.test.ts (1)
109-136: ⚡ Quick winMissing regression test: update/delete of a genuinely non-existent row should not throw after
postModelLevelCheckis added.
postModelLevelChecknow runs for every 0-rows UPDATE/DELETE when policies exist. The method's$existscheck is the safety net that prevents a "row not found" from being incorrectly raised asREJECTED_BY_POLICY. This path is not currently exercised by any test in this file.Consider adding a case like:
// verify "row not found" does not throw REJECTED_BY_POLICY await expect(db.foo.update({ where: { id: 9999 }, data: { x: 1 } })).resolves.toMatchObject({}); await expect(db.foo.delete({ where: { id: 9999 } })).resolves.toMatchObject({});into one of the existing schemas (e.g., the deny/allow update test) to confirm
$exists=falsecorrectly suppresses the error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/orm/policy/crud/error-codes.test.ts` around lines 109 - 136, Add assertions to ensure that updating/deleting a truly non-existent row does not raise REJECTED_BY_POLICY by exercising the postModelLevelCheck $exists path: in the existing test (the one that sets up model Foo and creates neg/noY/ok), after the current expectations add two checks using db.foo.update and db.foo.delete with a non-existent id (e.g., 9999) and assert they resolve (e.g., await expect(db.foo.update({ where: { id: 9999 }, data: { x: 1 } })).resolves.toMatchObject({}) and similarly for db.foo.delete) so that postModelLevelCheck’s $exists=false branch is covered and does not throw.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/plugins/policy/src/policy-handler.ts`:
- Around line 141-148: This changes post-check behavior to throw via
postModelLevelCheck when result.numAffectedRows is 0, which breaks callers that
relied on a silent 0 return and also leaves UPDATE…FROM unguarded; update the
code to (1) document the behavioral change in the changelog/migration notes that
model-level policy denials for update/delete now throw REJECTED_BY_POLICY
instead of returning numAffectedRows = 0 so callers can migrate, and (2) add the
same guard used for deletes to the update branch so UPDATE queries that use
FROM/join are skipped (e.g., change the condition to detect
UpdateQueryNode.is(node) && !(node.from ?? node.using) before calling
postModelLevelCheck), keeping the existing use of node.where?.where,
trueNode(this.dialect), mutationModel, and proceed.
---
Nitpick comments:
In `@packages/plugins/policy/src/policy-handler.ts`:
- Around line 261-317: postModelLevelCheck currently issues an extra SELECT
EXISTS whenever any model-level policy exists, even when callers don't need
denial detection; add a new boolean option (e.g., detectPolicyDenial) defaulting
to true and short-circuit the function when it's false. Specifically, update the
method postModelLevelCheck to return immediately if
this.options.detectPolicyDenial === false (place this check after the existing
isTrueNode(modelLevelFilter) check), and ensure callers can set
options.detectPolicyDenial to false to avoid the EXISTS/query and coded policy
evaluation; keep existing behavior when the flag is true and document the new
option default.
In `@tests/e2e/orm/policy/crud/error-codes.test.ts`:
- Around line 109-136: Add assertions to ensure that updating/deleting a truly
non-existent row does not raise REJECTED_BY_POLICY by exercising the
postModelLevelCheck $exists path: in the existing test (the one that sets up
model Foo and creates neg/noY/ok), after the current expectations add two checks
using db.foo.update and db.foo.delete with a non-existent id (e.g., 9999) and
assert they resolve (e.g., await expect(db.foo.update({ where: { id: 9999 },
data: { x: 1 } })).resolves.toMatchObject({}) and similarly for db.foo.delete)
so that postModelLevelCheck’s $exists=false branch is covered and does not
throw.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b0f06b43-07f3-4a7c-aa1c-5f7302c8f4c1
📒 Files selected for processing (4)
packages/orm/src/client/errors.tspackages/plugins/policy/src/plugin.tspackages/plugins/policy/src/policy-handler.tstests/e2e/orm/policy/crud/error-codes.test.ts
…wsCheck Rename and tighten the zero-rows diagnostic helper for update/delete: - Skip the diagnostic query entirely when no policies carry an error code, removing an unnecessary round-trip for the common case. - Fix BigInt comparison: use `> 0` negation instead of `=== 0` so numAffectedRows is handled correctly across drivers. - Reorder delete/update branches so the more specific guard (delete has no `using` restriction) runs first. - Inline the fetchPolicyCodes guard into a single early-exit path. Tests: add "does not throw for nonexistent row" cases for update and delete, and a focused test asserting the opt-in behaviour — rules without an errorCode keep the existing NotFound error, rules with an errorCode surface RejectedByPolicy instead. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
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 (1)
packages/plugins/policy/src/policy-handler.ts (1)
213-258:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftField-level update policy codes are still dropped here.
This path collapses all touched
@allow/@denyfield rules into a single boolean and then throws a genericNO_ACCESSerror. SincegetFieldPolicies()now parsescode, any rejection caused by a coded field-level update policy loses itspolicyCodes, so the new feature only works for model-level update rules.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/plugins/policy/src/policy-handler.ts` around lines 213 - 258, The field-level update path currently collapses all field `@allow/`@deny rules into a single boolean (fieldLevelFilter) and throws a generic NO_ACCESS, losing policyCodes parsed by getFieldPolicies(); update the flow to surface coded field-policy rejections: modify buildFieldPolicyFilter (or add a companion method) so it can produce both a boolean filter and, when a row would be rejected by a field rule, the associated policy code(s); change the violatingRowsQuery to SELECT the offending policy code(s) (instead of just lit(1)) for rows matching conjunction([updateFilter, NOT fieldLevelFilter]); then read those codes from preUpdateResult and pass them into createRejectedByPolicyError (adding a policyCodes argument) so the thrown error includes the field-level policy codes (referencing fieldsToUpdate, buildFieldPolicyFilter, fieldLevelFilter, updateFilter, violatingRowsQuery, preUpdateResult, and createRejectedByPolicyError).
♻️ Duplicate comments (1)
packages/plugins/policy/src/policy-handler.ts (1)
143-148:⚠️ Potential issue | 🟠 MajorGuard the zero-row diagnostic for joined
UPDATE/DELETEmutations.
postMutationZeroRowsCheck()rebuilds aSELECT ... FROM <model>and reusesnode.whereverbatim. ForUPDATE ... FROM/DELETE ... USING, predicates can reference joined tables, so this path can generate an invalid diagnostic query oncenumAffectedRowsis0.💡 Suggested guard
- if (!((result.numAffectedRows ?? 0) > 0)) { - if (DeleteQueryNode.is(node)) { + if (!((result.numAffectedRows ?? 0) > 0)) { + if (DeleteQueryNode.is(node) && !node.using) { await this.postMutationZeroRowsCheck(mutationModel, 'delete', node.where?.where, proceed); - } else if (UpdateQueryNode.is(node)) { + } else if (UpdateQueryNode.is(node) && !node.from) { await this.postMutationZeroRowsCheck(mutationModel, 'update', node.where?.where, proceed); } }#!/bin/bash set -e echo "=== zero-row call site ===" sed -n '141,148p' packages/plugins/policy/src/policy-handler.ts echo echo "=== diagnostic query shape ===" sed -n '279,309p' packages/plugins/policy/src/policy-handler.ts echo echo "=== joined mutation support in the transformer ===" sed -n '604,659p' packages/plugins/policy/src/policy-handler.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/plugins/policy/src/policy-handler.ts` around lines 143 - 148, The zero-row diagnostic call should be skipped for joined UPDATE/DELETE mutations because postMutationZeroRowsCheck rebuilds a SELECT using node.where verbatim and joined predicates can reference other tables; update the conditional around DeleteQueryNode.is(node) and UpdateQueryNode.is(node) to first detect joined mutations (e.g., presence of node.from, node.using, or any join entries—implement a small helper like hasJoins(node) or inline checks such as node.from?.length>0 || node.using?.length>0 || node.from != null) and only call this.postMutationZeroRowsCheck(mutationModel, 'delete'|'update', node.where?.where, proceed) when no joins are present; if joins exist, skip the diagnostic to avoid generating invalid SELECT queries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/plugins/policy/src/policy-handler.ts`:
- Around line 286-318: The diagnostic existence query and policy-code checks are
still executed even when fetchPolicyCodes is false; add an early guard so when
this.options.fetchPolicyCodes === false you skip building/running the diagnostic
(i.e., avoid constructing codeSelections and avoid calling proceed/await on the
SelectQueryNode) and do not call createRejectedByPolicyError or throw
RejectedByPolicyReason.NO_ACCESS; implement this by returning immediately (or
otherwise short-circuiting) before the code that references selectedPolicies,
codeSelections, proceed(rowExistsInner...), and createRejectedByPolicyError so
no diagnostic query runs and no throw occurs when fetchPolicyCodes is disabled.
---
Outside diff comments:
In `@packages/plugins/policy/src/policy-handler.ts`:
- Around line 213-258: The field-level update path currently collapses all field
`@allow/`@deny rules into a single boolean (fieldLevelFilter) and throws a generic
NO_ACCESS, losing policyCodes parsed by getFieldPolicies(); update the flow to
surface coded field-policy rejections: modify buildFieldPolicyFilter (or add a
companion method) so it can produce both a boolean filter and, when a row would
be rejected by a field rule, the associated policy code(s); change the
violatingRowsQuery to SELECT the offending policy code(s) (instead of just
lit(1)) for rows matching conjunction([updateFilter, NOT fieldLevelFilter]);
then read those codes from preUpdateResult and pass them into
createRejectedByPolicyError (adding a policyCodes argument) so the thrown error
includes the field-level policy codes (referencing fieldsToUpdate,
buildFieldPolicyFilter, fieldLevelFilter, updateFilter, violatingRowsQuery,
preUpdateResult, and createRejectedByPolicyError).
---
Duplicate comments:
In `@packages/plugins/policy/src/policy-handler.ts`:
- Around line 143-148: The zero-row diagnostic call should be skipped for joined
UPDATE/DELETE mutations because postMutationZeroRowsCheck rebuilds a SELECT
using node.where verbatim and joined predicates can reference other tables;
update the conditional around DeleteQueryNode.is(node) and
UpdateQueryNode.is(node) to first detect joined mutations (e.g., presence of
node.from, node.using, or any join entries—implement a small helper like
hasJoins(node) or inline checks such as node.from?.length>0 ||
node.using?.length>0 || node.from != null) and only call
this.postMutationZeroRowsCheck(mutationModel, 'delete'|'update',
node.where?.where, proceed) when no joins are present; if joins exist, skip the
diagnostic to avoid generating invalid SELECT queries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3c62a3f6-33de-473f-bfc1-56597ff21101
📒 Files selected for processing (4)
packages/plugins/policy/src/policy-handler.tstests/e2e/orm/policy/crud/delete.test.tstests/e2e/orm/policy/crud/error-codes.test.tstests/e2e/orm/policy/crud/update.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/orm/policy/crud/error-codes.test.ts
Contributor
Author
|
For update and delete policy violations, the ORM was silently returning "not found" — no way to tell if the row was missing or just denied, and no way to get the error code. In the last 2 commits, I added support to these operations (update/delete) by adding postMutationZeroRowsCheck: when numAffectedRows == 0, fire a single query to check if the row actually exists. If it does, the policy blocked it — throw REJECTED_BY_POLICY with the relevant codes. If not, stay silent and let the ORM handle "not found" as usual. Opt-in only — the check only runs when the model has at least one policy with an errorCode. No codes = no behavior change. Zero breaking changes for existing schemas... but can be confusing... Performance — no extra query on the happy path. One combined SELECT (existence + all code diagnostics) on the error path. |
…ctations Move the fetchPolicyCodes===false early-return before policy filtering so the diagnostic query is skipped entirely for update/delete, making those operations behave identically to a model with no error codes (NOT_FOUND). Update test expectations accordingly and add an explicit equivalence test. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Extend policy error code surfacing to `findFirst`/`findUnique` (and their
`OrThrow` variants): when the query returns 0 rows, a diagnostic query now
checks whether the row exists but was filtered by a deny/allow rule that
carries an `errorCode`. If so, the handler throws `REJECTED_BY_POLICY`
with the relevant codes instead of silently returning `null`.
`findMany` is unaffected — it continues to use filter-based enforcement
and returns an empty array for denied rows. This holds even for
`findMany({ take: 1 })`, which generates LIMIT 1 SQL but remains a
multi-row ORM operation by name.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…r dialects without RETURNING On dialects lacking RETURNING support (e.g. MySQL), the ORM pre-loads entity ID fields before an UPDATE to identify the row for re-reading. This pre-load ran through onKyselyQuery plugin hooks, causing a read-policy denial to surface as "Record not found" before the UPDATE executed — masking the correct error code. Introduce internalQueryContextStorage (AsyncLocalStorage) to mark these internal pre-load queries; ZenStackQueryExecutor now skips all onKyselyQuery hooks when the flag is set. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
Author
|
I tried to implement as much as I could. I hope I didn't head in the wrong direction — I ultimately stayed pretty close to the approach recommended in the issue, except that I don't override the error message but instead add all error codes (which can be enums) concerned by the policy violation. I'm happy to change this if it doesn't fit (keeping the same generic message regardless of whether you opt into retrieving error codes seems convenient to me). The last commit is a fix that came directly from Claude for two tests that were failing with MySQL — it deserves a review... |
Contributor
Author
|
By revisiting the PR description, I've just realized that findFirst / findUnique should never throw... (→ null). It's fixed now. |
Azzerty23
force-pushed
the
feat/policy-custom-error-codes
branch
7 times, most recently
from
6055f81 to
3b0e78f
Compare
Azzerty23
force-pushed
the
feat/policy-custom-error-codes
branch
7 times, most recently
from
b87f004 to
502f209
Compare
Add a browser export condition to @zenstackhq/orm so that browser bundlers (Turbopack, webpack) use an empty stub instead of dist/index.mjs, which imports node:async_hooks via internal-context.ts. Also include the Node.js version in the CI pnpm cache key to prevent native module (better-sqlite3) ABI mismatches after a Node.js upgrade. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Azzerty23
force-pushed
the
feat/policy-custom-error-codes
branch
from
502f209 to
58f0efb
Compare
On non-RETURNING dialects (MySQL), the ORM pre-loads entity IDs before an UPDATE. If the row is read-denied, the pre-load returns null and the UPDATE never runs, masking the real update-deny error code. Adds `requiresUpdatePreloadBypassReadPolicy` to the dialect interface (true for MySQL), `executeQueryDirect` to ZenStackQueryExecutor to run a query without onKyselyQuery interceptors, and `readUniqueDirect` in the base operation handler to use it for the pre-load step. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…rectly to executeQueryDirect Eliminates the withConnectionProvider/SingleConnectionProvider/scopedExecutor indirection in readUniqueDirect. executeQueryDirect now takes a DatabaseConnection parameter and delegates straight to internalExecuteQuery, removing the redundant provideConnection wrapper and double ensureProperQueryResult call. Also removes the dead ?? null on the bypass branch and trims the 8-line narration comment. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…eries for dialects without RETURNING" This reverts commit 885a04c.
…t map Eliminates the policyContextStorage AsyncLocalStorage by threading a per-operation Map<string,unknown> through the onQuery/onKyselyQuery hook chain instead. This removes the node:async_hooks dependency from the policy plugin (and the orm package browser stub that worked around it), letting the Next.js sample drop serverExternalPackages overrides. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…g transaction context
Azzerty23
force-pushed
the
feat/policy-custom-error-codes
branch
from
e8aa787 to
2ad4be0
Compare
… via direct flag Remove the requiresUpdatePreloadBypassReadPolicy dialect flag and the duplicate readUniqueDirect method. All non-RETURNING dialects now use the direct=true path in read/readUnique, which routes connection acquisition through the outer executor while bypassing onKyselyQuery interceptors. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Azzerty23
force-pushed
the
feat/policy-custom-error-codes
branch
from
2ad4be0 to
f3f126c
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Before this PR, policy rejections surfaced either as a generic error with no machine-readable detail, or — for
update,delete, and throwing single-row reads (findFirstOrThrow,findUniqueOrThrow) — as a plainNOT_FOUND, making it impossible to distinguish a missing row from a denied one. There was no structured information to branch on, even by parsing the error message.This PR adds an optional
errorCodeargument to@@allowand@@denypolicy attributes. When an operation is rejected, every matching rule's code is collected and surfaced onORMError.policyCodes, giving callers a reliable way to handle specific policy violations programmatically.Fixes #1723
Schema syntax
The third argument accepts a string literal or an enum value:
Error surfacing
ORMError.policyCodescontains the codes of all matching rules (not just the first), so a single failed operation can carry multiple codes. Ex:Scope: which operations surface codes
create/post-updateREJECTED_BY_POLICY(no codes)REJECTED_BY_POLICY+policyCodesupdate/delete/findFirstOrThrow/findUniqueOrThrowNOT_FOUNDREJECTED_BY_POLICY+policyCodesfindFirst/findUniquenullnull(never throws)findMany[][](never throws)createandpost-updatealready run a dedicated SELECT as part of normal enforcement (SELECT EXISTS(...)before the INSERT, post-write SELECT forpost-update). The rejection is alwaysREJECTED_BY_POLICY; code collection is piggybacked on that existing query — one extra EXISTS column per custom error code, zero additional round-trips.update,delete, andfindFirstOrThrow/findUniqueOrThrowuse filter-based enforcement (WHERE clause injection). When 0 rows come back, the ORM can't distinguish "row doesn't exist" from "row was denied by policy". To surface codes, a separate diagnostic query is required: first check whether a matching row exists without the policy filter, then collect the codes of the rules that fired. This extra round-trip only runs when at least one rule on the model carries a code — so the opt-in is implicit via the schema: adding a code to any rule automatically upgrades the default behavior toREJECTED_BY_POLICY(withpolicyCodesset) for that model, at the cost of one additional query for the rejected operation.findFirstandfindUniquealways returnnullon policy denial regardless — they are filter-based and never throw, so no diagnostic query is ever triggered for them.findManyis never affected — it uses filter-based enforcement and silently excludes denied rows.Performance opt-out
The codes collection step can be disabled via
fetchPolicyCodes: falseto fall back to the default behavior.Backward compatibility
Purely additive. No changes to the existing error message format or
ORMErrorstructure. Callers that don't use@@allow/@@denyerror codes see no behavioral change.policyCodesisundefined(not[]) when no codes are present.Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores