fix(core): refuse to prune a sealed filter condition - #879
Conversation
A filters `validate` hook may answer with a sealed policy residual that scopes the leaf it saw. When that residual names a relation the relations `validate` hook rejects, `pruneFiltersByRelations` recursed into the sealed group and dropped the residual's leaf along with the client's own, returning a query wider than the policy intended, silently. Pruning now treats a seal as protecting the whole subtree it heads: a drop inside (or of) a sealed condition throws `SchemaError` with the new `SCHEMA_SEALED_CONDITION_PRUNED` code, naming both the rejected relation and the sealed field. The two validators contradict each other (dropping the condition widens the result set, keeping it joins a relation the gate refused), which is a server misconfiguration rather than bad client input, hence `SchemaError` and not a `ParseError`. An unsealed residual stays displaceable and is pruned as before. Closes #877
📝 WalkthroughWalkthroughAdds a typed schema error for rejected relations required by sealed filter conditions. Relation pruning now propagates sealed state through nested groups, ChangesSealed condition pruning
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant FilterParser
participant RelationPruner
participant RelationValidator
participant SchemaError
FilterParser->>RelationPruner: provide validated filter conditions
RelationPruner->>RelationValidator: resolve referenced relation
RelationValidator-->>RelationPruner: return rejected relation
RelationPruner->>SchemaError: raise sealedConditionPruned(relation, field)
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
There was a problem hiding this comment.
Pull request overview
This PR fixes a fail-open edge case in the core relation-pruning logic when a schema’s filters validate hook returns a sealed policy residual that traverses a relation the relations gate later rejects. Instead of silently pruning part of a sealed subtree (which can widen results), pruning now treats the seal as protecting the entire subtree and throws a SchemaError (ErrorCode.SCHEMA_SEALED_CONDITION_PRUNED) on contradictions.
Changes:
- Core: relation-based filter pruning now detects drops within sealed subtrees and throws
SchemaError.sealedConditionPruned(...)instead of pruning. - Core: adds
ErrorCode.SCHEMA_SEALED_CONDITION_PRUNEDand a correspondingSchemaErrorconstructor helper with a detailed message. - Tests + docs: adds unit and end-to-end coverage for sealed residual vs relations-gate contradictions and documents the new error/behavior.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/core/src/parser/relation-prune.ts | Tracks “sealed subtree” during pruning; throws when a rejected relation would prune any sealed condition. |
| packages/core/src/errors/schema.ts | Adds SchemaError.sealedConditionPruned(relation, field) with the new error code and explanatory message. |
| packages/core/src/errors/code.ts | Introduces ErrorCode.SCHEMA_SEALED_CONDITION_PRUNED. |
| packages/core/test/unit/parser/relation-prune.spec.ts | Adds unit tests covering sealed leaves, sealed groups, sealed elemMatch, and “prune around untouched seal”. |
| packages/parser-simple/test/unit/parser/relations-traversal.spec.ts | Adds end-to-end parser coverage for the reproduction scenario (sync, async, standalone filters parse; sealed vs unsealed residual boundaries). |
| packages/docs/guide/relations.md | Documents that pruning never drops sealed conditions and that contradictions throw SchemaError with the new code. |
| packages/docs/guide/recipes/authorization.md | Adds warning about sealed residuals that traverse relations needing to align with the relations gate. |
| packages/docs/guide/merging-queries.md | Documents seal’s subtree protection and the parsing-time contradiction behavior. |
| packages/docs/guide/filters.md | Documents sealed replacement behavior vs relation pruning and the thrown SchemaError. |
| packages/docs/guide/errors.md | Adds the new schema error code and its trigger to the error code table. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Self-audit follow-up. The schema `default` is deliberately exempt from the relations gate: it is re-applied after pruning, un-pruned. That left the sealed case input-dependent, because whether a default is materialized before the pruning pass (the client sent no filters) or after it (the client sent filters that all pruned away) is decided by client input: the same sealed default naming a rejected relation threw in the first case and survived silently in the second. The fallback now runs the same check, so a sealed default behaves like a sealed residual in every shape. Unsealed defaults keep surviving unchanged, which is what makes them the trusted baseline. Also: rename the `drop` helper to `dropUnlessSealed`, correct the claim that a pruned seal always widens (it narrows under an `or`, and the refusal is a per-node rule rather than a per-operator judgement), and pin the `or` and `not` shapes with tests.
Self-audit follow-up (eef5040)Probed the edges rather than trusting the happy path. Three findings, all addressed. 1. Sealed schema
|
The filters `validate` recipe recommended `seal(and(filter, <scope>))`. A seal protects the whole subtree it heads, so that shape also protects the client's own leaf: an actor filtering on a relation it may not traverse then raised SCHEMA_SEALED_CONDITION_PRUNED instead of simply having that filter dropped, even though the residual sat on a local column and nothing was misconfigured. `and(filter, seal(<scope>))` protects the same residual (a group is never displaced as a unit either, and the marker survives a `flatten()` that hoists the residual into the root), keeps the client leaf prunable, and reserves the error for a residual that itself names a rejected relation, which is the actual contradiction the check is for. Recipe updated in the filters guide, the authorization recipe, the seal section of the composition guide and .agents/architecture.md, with tests pinning all three properties of the recommended shape.
Recipe change: seal the residual, not the group (7b59c77)Follow-up to the audit. The blast radius of the new error was wider than #877 assumed, and the cause was the recommended shape rather than the rule. The issue notes that "a residual on a local column ( The first case is not a misconfiguration: the residual is fine, the gate is fine, and they collide only because the seal happens to cover the client's leaf. The second is what you want, and it gives up nothing: A group is never displaced as a unit either, so both shapes resist a merge; the seal only matters once So the recipe now reads Net effect: the error now fires only on the contradiction #877 actually describes. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/test/unit/parser/relation-prune.spec.ts`:
- Around line 280-291: Update the test around seal and the OR filter so only the
user.name arm is sealed while the surrounding OR node remains unsealed, then
retain the expectation that pruning for user throws
SCHEMA_SEALED_CONDITION_PRUNED. Adjust the test name and comments to describe a
sealed OR arm rather than a sealed OR group.
- Around line 323-332: The sealed-default coverage currently only exercises
pruning after a client filter is present. Extend the relation-pruning tests
around pruneFiltersByRelations to also call it with an empty client filters tree
and a sealed default referencing the rejected relation, asserting
ErrorCode.SCHEMA_SEALED_CONDITION_PRUNED; preserve the existing non-empty filter
case.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 628a23be-0025-4541-af89-22465df300d1
📒 Files selected for processing (8)
.agents/architecture.mdpackages/core/src/parser/relation-prune.tspackages/core/test/unit/parser/relation-prune.spec.tspackages/docs/guide/filters.mdpackages/docs/guide/merging-queries.mdpackages/docs/guide/recipes/authorization.mdpackages/docs/guide/relations.mdpackages/parser-simple/test/unit/parser/relations-traversal.spec.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/docs/guide/recipes/authorization.md
- packages/docs/guide/relations.md
- packages/core/src/parser/relation-prune.ts
- packages/parser-simple/test/unit/parser/relations-traversal.spec.ts
| // The two shapes below cannot fail open: dropping an OR arm narrows, | ||
| // and dropping the interior of a NOT removes a restriction the seal | ||
| // put there. Pruning still refuses, because the seal is a per-node | ||
| // marker and not a per-operator judgement call. | ||
| it('throws for a sealed OR arm, where a drop would narrow rather than widen', () => { | ||
| const filters = new Filters(FilterCompoundOperator.AND, [ | ||
| seal(new Filters(FilterCompoundOperator.OR, [eq('id'), eq('user.name')])), | ||
| ]); | ||
|
|
||
| expect(() => pruneFiltersByRelations(filters, ['user'])) | ||
| .toThrowError(expect.objectContaining({ code: ErrorCode.SCHEMA_SEALED_CONDITION_PRUNED })); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Exercise a sealed OR arm, not only a sealed OR group.
Line 286 calls seal() on the entire OR node. The test does not verify that a sealed user.name arm is rejected while the surrounding OR remains unsealed. Move seal() to the rejected arm, or rename the test and comments to describe a sealed group.
Proposed test shape
const filters = new Filters(FilterCompoundOperator.AND, [
- seal(new Filters(FilterCompoundOperator.OR, [eq('id'), eq('user.name')])),
+ new Filters(FilterCompoundOperator.OR, [eq('id'), seal(eq('user.name'))]),
]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // The two shapes below cannot fail open: dropping an OR arm narrows, | |
| // and dropping the interior of a NOT removes a restriction the seal | |
| // put there. Pruning still refuses, because the seal is a per-node | |
| // marker and not a per-operator judgement call. | |
| it('throws for a sealed OR arm, where a drop would narrow rather than widen', () => { | |
| const filters = new Filters(FilterCompoundOperator.AND, [ | |
| seal(new Filters(FilterCompoundOperator.OR, [eq('id'), eq('user.name')])), | |
| ]); | |
| expect(() => pruneFiltersByRelations(filters, ['user'])) | |
| .toThrowError(expect.objectContaining({ code: ErrorCode.SCHEMA_SEALED_CONDITION_PRUNED })); | |
| }); | |
| // The two shapes below cannot fail open: dropping an OR arm narrows, | |
| // and dropping the interior of a NOT removes a restriction the seal | |
| // put there. Pruning still refuses, because the seal is a per-node | |
| // marker and not a per-operator judgement call. | |
| it('throws for a sealed OR arm, where a drop would narrow rather than widen', () => { | |
| const filters = new Filters(FilterCompoundOperator.AND, [ | |
| new Filters(FilterCompoundOperator.OR, [eq('id'), seal(eq('user.name'))]), | |
| ]); | |
| expect(() => pruneFiltersByRelations(filters, ['user'])) | |
| .toThrowError(expect.objectContaining({ code: ErrorCode.SCHEMA_SEALED_CONDITION_PRUNED })); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/test/unit/parser/relation-prune.spec.ts` around lines 280 -
291, Update the test around seal and the OR filter so only the user.name arm is
sealed while the surrounding OR node remains unsealed, then retain the
expectation that pruning for user throws SCHEMA_SEALED_CONDITION_PRUNED. Adjust
the test name and comments to describe a sealed OR arm rather than a sealed OR
group.
| it('throws for a SEALED default naming a rejected relation', () => { | ||
| // otherwise the same default would throw when it is materialized | ||
| // before this pass (client sent no filters) and survive when it is | ||
| // materialized after it (client sent filters that all pruned away). | ||
| const filters = new Filters(FilterCompoundOperator.AND, [eq('user.a')]); | ||
| const schema = defineFiltersSchema({ default: seal(eq('user.b')) }); | ||
|
|
||
| expect(() => pruneFiltersByRelations(filters, ['user'], schema)) | ||
| .toThrowError(expect.objectContaining({ code: ErrorCode.SCHEMA_SEALED_CONDITION_PRUNED })); | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 12 \
'SCHEMA_SEALED_CONDITION_PRUNED|default: seal|defineFiltersSchema' \
packages/core/test/unit/parser/relation-prune.spec.tsRepository: tada5hi/rapiq
Length of output: 8732
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate source definitions =="
rg -n "function pruneFiltersByRelations|export .*pruneFiltersByRelations|SCHEMA_SEALED_CONDITION_PRUNED|defineFiltersSchema" packages/core/src packages/core/test -g '*.ts' | sed -n '1,220p'
echo
echo "== relevant source snippets =="
for file in $(rg -l "pruneFiltersByRelations|SCHEMA_SEALED_CONDITION_PRUNED|defineFiltersSchema" packages/core/src packages/core/test -g '*.ts' | head -20); do
echo "--- $file ---"
rg -n -C 20 "pruneFiltersByRelations|SCHEMA_SEALED_CONDITION_PRUNED|defineFiltersSchema" "$file" | sed -n '1,220p'
doneRepository: tada5hi/rapiq
Length of output: 44603
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
path = Path('packages/core/src/parser/relation-prune.ts')
text = path.read_text()
m = re.search(r'export function pruneFiltersByRelations[\s\S]+?^}', text, re.M)
print(m.group(0) if m else 'function not found')
print()
print('uses buildFiltersDefaults:')
print('buildFiltersDefaults' in (m.group(0) if m else ''))
print('check all calls to pruneFiltersByRelations in tracked TS:')
for p in Path('packages/core').glob('**/*.ts'):
if any(part.endswith(('.test', '.spec')) for part in p.parts):
lines = p.read_text(errors='ignore').splitlines()
for i, line in enumerate(lines, 1):
if 'pruneFiltersByRelations(' in line or 'buildFiltersDefaults(' in line:
print(f'{p}:{i}:{line.strip()}')
PYRepository: tada5hi/rapiq
Length of output: 1448
Cover the no-client-filter sealed-default path.
The existing case only prunes a client leaf and then checks the default fallback. Add the opposite call shape with an empty client filters tree, or cover that exact sealed default in another sealed-default test.
[loweffort_and_high_reward]
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/test/unit/parser/relation-prune.spec.ts` around lines 323 -
332, The sealed-default coverage currently only exercises pruning after a client
filter is present. Extend the relation-pruning tests around
pruneFiltersByRelations to also call it with an empty client filters tree and a
sealed default referencing the rejected relation, asserting
ErrorCode.SCHEMA_SEALED_CONDITION_PRUNED; preserve the existing non-empty filter
case.
Closes #877 with option C: the contradiction between the two validators throws instead of failing open.
The gap
A filters
validatehook may answer with a sealed policy residual scoping the leaf it saw (the authorization recipe pattern). When that residual names a relation the relationsvalidatehook rejects,pruneConditionrecursed into the sealed group and dropped the residual along with the client's own leaf:The fix
Pruning now treats a seal as protecting the whole subtree it heads. A drop inside (or of) a sealed condition throws, naming both sides of the contradiction:
An unsealed residual is an ordinary displaceable condition and is still pruned, which keeps
sealthe single marker that says "this must survive". Everything the gate did before is unchanged when no seal is in the way.SchemaError, notParseErrorThe issue's option C said "typed
ParseError", but also called this a server misconfiguration, and those point at different HTTP buckets inerrors.md(ParseErrormaps to 400). Two contradicting hooks are a configuration bug on the receiving side, and a 400 would be swallowed as ordinary client noise. Precedent:SCHEMA_VALIDATOR_ASYNC_REQUIRES_ASYNC_PARSERis likewise detected during parse and classed as a schema error. New code:ErrorCode.SCHEMA_SEALED_CONDITION_PRUNED.Tests
packages/core/test/unit/parser/relation-prune.spec.ts: sealed leaf, residual inside a sealed group, sealed condition below an unsealed group, sealedelemMatch(target and interior), and pruning that leaves an untouched seal alone.packages/parser-simple/test/unit/parser/relations-traversal.spec.ts: the issue's reproduction end to end (sync, async, standalone filters parse), plus the permitted-relation and unsealed-residual boundaries.Docs updated:
errors.md(code table),relations.md(rejection prunes deep),filters.md(validate replacements),merging-queries.md(seal section),recipes/authorization.md(warning box on scoping through a relation path vs. a local column).Summary by CodeRabbit
New Features
Documentation
Tests