fix(core): scope=all with no org context must return everything the user owns - #181
Conversation
… user owns Second review pass over the #174 arc. One more helper divergence, this one pre-existing and unrelated to #174 — it had simply never been looked at. buildKnowledgeWhereV2 and buildRawProjectFilterV2 express ONE policy on two query surfaces. Under scope="all" with an empty accessible-project list they disagreed: raw -> ownerUserId = $user AND deletedAt IS NULL (everything owned) prisma -> (visibility='private' AND owner) OR (ownerProjectId IS NULL AND owner) So the Prisma path dropped the user's own `visibility: 'project'` rows — the DEFAULT visibility, i.e. most of them. That contradicts the documented ?scope=all contract in KNOWLEDGE §12.19 ("shows everything the authenticated user owns across all projects"), and is reachable whenever getAccessibleProjectIds() short-circuits to [] for a non-member (org.ts:1034). Fail-safe — it under-reported, it never leaked another user's rows — but wrong, and invisible because each helper looked correct in isolation. Fixed by returning the same catch-all the raw helper already used, and pinned by a test that asserts BOTH helpers in the same case. That assertion style is the actual lesson: a per-function test cannot see a gap that lives between two functions, and "both individually correct" is how such a gap survives review. Docs swept for the two parity fixes (this one and #180's): - GUIDELINES invariant 3: the opt-in governs branches that HAVE a boundary; with no activeProjectId it is unconditional. Adds the change-them-together rule for the helper pair. - KNOWLEDGE: scope table and §12.19 now state that the Prisma listing helper applies the identical rule, that "retrieval wider than listing" holds only where an active project supplies a boundary, and that ?scope=all with no org context returns everything owned. - KNOWN_ISSUES §0p: new row covering both divergences plus the zero-coverage finding on buildKnowledgeWhereV2. - APPROACH §5bf: "the fix is part of the system too — review it like one", and check coverage before adding to something. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rqj5y9fT3XPUqnqKyustp9
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
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 |
…ct a false claim CI failed on scope-filter-v2.test.ts, and it caught two things. 1. The test asserted the OLD object shape for scope="all" with an empty accessible list. Its intent — "no proj+org IN-list branch when there are no accessible projects" — is preserved and now asserted on the semantic instead: the helper returns the flat owner catch-all. Verified this is the right direction rather than assuming: V1 buildKnowledgeWhere has ALWAYS done `return base` here (scope-filter.ts:62), the raw V2 helper does the same, and KNOWLEDGE §12.19 documents ?scope=all as "everything the user owns". The old Prisma V2 was the lone outlier. Only that one test failed; the other scope="all" tests pass a non-empty list and are untouched. 2. It refuted a claim I published in #180 and in the docs: that buildKnowledgeWhereV2 had ZERO test coverage. It does not — a second file, scope-filter-v2.test.ts, covers it in 22 places. I had run `grep -c` against scope-filter.test.ts alone and generalised from one file to "no coverage anywhere". Retracted in KNOWN_ISSUES §0p and APPROACH §5bf rather than quietly softened. What was actually true is narrower: the #177 branch was unreachable because no caller passed the flag, and the cross-helper case had no test in either file. Both still worth fixing; neither is "zero coverage". The corrected lesson is smaller and sharper than the one first written: a coverage claim from a single-file grep is not a coverage claim, and reaching for the more dramatic version of a finding is its own failure mode — the same one APPROACH §5bf opens by describing. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rqj5y9fT3XPUqnqKyustp9
1 participant
Second review pass over the #174 arc. One more helper divergence — this one pre-existing and unrelated to #174; it had simply never been looked at.
The bug
buildKnowledgeWhereV2andbuildRawProjectFilterV2express one policy on two query surfaces. Underscope: "all"with an empty accessible-project list they disagreed:The Prisma path drops the user's own
visibility: 'project'rows — which is the default visibility, so most of them.That contradicts the documented contract in
KNOWLEDGE §12.19— "?scope=allshows everything the authenticated user owns across all projects" — and is reachable whenevergetAccessibleProjectIds()short-circuits to[]for a non-member (org.ts:1034).Fail-safe: it under-reported; it never leaked another user's rows. Wrong, but not a hole.
Why it survived
Each helper looks correct read on its own. The gap lives between them, and no per-function test can see that. So the fix is pinned by a test that asserts both helpers in the same case — which is the transferable part, and now the stated rule in GUIDELINES for this pair.
That's the third finding of this kind in two days: the first (no-
activeProjectId) was introduced by the #174 fix itself, the second wasbuildKnowledgeWhereV2having zero coverage, and this one predates all of it.Docs swept
activeProjectIdit's unconditional. Adds the change-them-together rule for the helper pair.?scope=allreturns with no org context.grep -cwould have said0).Test plan
scope-filter+rate-limitvia standalonenpx vitest@2.org.ts:1034's non-member short-circuit rather than assuming the path was dead.ownerUserId; no other user's data enters any branch.check-doc-refs.shagainst the live ceiling (fix(core): align the two V2 scope helpers; cover the untested one #180).cross-user-isolationsuite.Storeadapter still has no automated test (module-private inapps/web, can't run that suite locally). Exercised in production and clean under load.🤖 Generated with Claude Code
https://claude.ai/code/session_01Rqj5y9fT3XPUqnqKyustp9