fix(core): align the two V2 scope helpers; cover the untested one - #180
Merged
Conversation
Review pass over the v2.3.0 -> v2.5.2 arc turned up three gaps, all
introduced by that arc.
1. buildKnowledgeWhereV2 and buildRawProjectFilterV2 DISAGREED on the
no-activeProjectId case. The raw helper admits scope IN
('user','global') unconditionally there — added 2026-05-12, on the
reasoning that with no active project there is no boundary to
enforce. The Prisma twin gated it on the new opt-in flag. They back
the same visibility rule on different query surfaces, so a caller's
results depended on which one their code path happened to hit. Same
inconsistency class CodeRabbit caught inside the raw helper on #179
— fixed there, and introduced between the two.
2. buildKnowledgeWhereV2 had ZERO test coverage despite backing the
knowledge listing route and action-items, and #177 added an
unreachable branch to it (no caller passes the flag; kra/oracle use
the raw helper). Six tests now pin: closed by default under an
active project, opened when opted in, applied to scope="all",
unconditional with no active project — asserted against the raw
helper in the same test so they cannot drift apart again — and the
disjunct pinned to ownerUserId.
3. Documented the behaviour change the parity fix causes at a
security-reviewed call site. action-items.ts passes projectId as
string | null; with null it now admits the caller's own user-scoped
rows. Acceptable — accessibleProjectIds stays [], the query ANDs
type: action_item, it is pinned to ownerUserId, and action items are
project-scoped by contract so the set is empty — but it is a real
change at a spot with an explicit isolation note, so it is stated
rather than left to be rediscovered.
Also notes on Limit.max that 0 blocks everything and is not a
"disabled" sentinel. Before the atomic rewrite the fresh-bucket path
returned ok:true unconditionally, so max:0 leaked one request per
window; that inconsistency is gone and nothing depended on it.
Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Rqj5y9fT3XPUqnqKyustp9
|
Warning Review limit reached
Next review available in: 38 minutes 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 (4)
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 |
7 tasks
bejranonda
added a commit
that referenced
this pull request
Jul 31, 2026
…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
bejranonda
added a commit
that referenced
this pull request
Jul 31, 2026
…ser owns (#181) * fix(core): scope="all" with no org context must return everything the 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 * fix(core): update the all-scope test to the corrected contract; retract 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 --------- Co-authored-by: Claude <noreply@anthropic.com>
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.
Review pass over everything the v2.3.0 → v2.5.2 arc shipped. Three gaps, all introduced by that arc.
1. The two V2 helpers disagreed
buildRawProjectFilterV2admitsscope IN ('user','global')unconditionally when there's noactiveProjectId— added 2026-05-12, on the reasoning that with no active project there's no boundary to enforce. Its Prisma twinbuildKnowledgeWhereV2gated the same clause behind the new opt-in flag.They back one visibility rule on two query surfaces, so a caller's results depended on which one their code path happened to hit.
This is the same inconsistency class CodeRabbit caught inside the raw helper on #179 — I fixed it there and introduced it between the two.
2.
buildKnowledgeWhereV2had zero testsgrep -c buildKnowledgeWhereV2 scope-filter.test.ts→ 0, despite it backing the knowledge listing route andaction-items.ts. #177 then added a branch to it that no caller can reach (onlykra/oraclepass the flag, and they use the raw helper) — untested dead code on an untested function.Six tests now pin the contract:
scope: "all"ownerUserId3. The parity fix changes behaviour at a security-reviewed call site — stated, not glossed
action-items.tspassesprojectId: string | null. Withnull, it now admits the caller's ownscope:'user'|'global'rows.Acceptable, and documented inline with the reasoning:
accessibleProjectIdsstays[](the 2026-07-10 finding was specifically about org-accessible projects), the query still ANDstype: action_item, it stays pinned toownerUserId, and action items are created project-scoped by contract — so the set is empty in practice. But it is a real change somewhere carrying an explicit isolation note, so it's written down rather than left to be rediscovered.Also
Limit.maxnow documents that0blocks everything and is not a "disabled" sentinel. Worth recording because the atomic rewrite silently changed it: the old fresh-bucket path returnedok: trueunconditionally, somax: 0leaked one request per window. Nothing depended on that, and no "0 disables" convention exists in.env.exampleor the docs — checked.Test plan
buildKnowledgeWhereV2(args(null))→ no user-scope clause while the raw helper had one). Green after.scope-filter+rate-limitvia standalonenpx vitest@2.buildKnowledgeWhereV2/buildRawProjectFilterV2caller again for the blast radius of the parity change;meeting-extract.tsis unaffected (always passes a non-null projectId)..env.exampleand docs for a "0 disables" rate-limit convention before changing that semantic — none exists.cross-user-isolationsuite — the one I most want green, and can't run locally.Storeadapter still has no automated test — it's module-private inapps/weband I can't run that suite locally, so I'd rather report the gap than ship coverage I haven't seen pass. It is exercised in production right now ("redis ready", clean under load).🤖 Generated with Claude Code
https://claude.ai/code/session_01Rqj5y9fT3XPUqnqKyustp9