fix(core): let scope:user knowledge follow the user across projects (#174) - #177
Merged
Conversation
…174) brain_teach_knowledge defaults scope to "user", but a row written from inside a project also gets that project's ownerProjectId and a default visibility of "project" — so it was invisible from every other project. Symptom: "the brain has knowledge but retrieval returns nothing." This turned out NOT to be a new design question. scope-filter.ts's no-active-project branch already carries exactly this fix, added 2026-05-12 after 5/5 retrieval misses traced to it. The two active-project branches never got it. The project had already decided; the decision was applied to one branch out of three. Fix: the same `scope IN ('user','global') AND ownerUserId = $user` disjunct, behind a new opt-in includeUserScopeAcrossProjects flag (default false), enabled only at the two personal-rule retrieval sites (kra.ts, oracle.ts). Opt-in is load-bearing, not ceremony. Two other callers of the shared helper rely on the project edge holding: - action-items.ts treats the project boundary as the isolation line for tasks (2026-07-10 review, finding 1). - meeting-extract.ts's supersession search is deliberately project-wide but NOT owner-scoped (2026-07-17 finding I2); widening it would drag the caller's other projects' decisions into a shared team surface. Changing the shared helper for everyone is the exact mistake that file's own comment warns about (GUIDELINES §7). Cross-project reach never becomes cross-user reach: the new disjunct carries its own ownerUserId predicate, and both callers already hard-filter ownerUserId in the outer query. Tests pin both properties plus the closed-by-default behaviour. Measured read-only against the live corpus: rows visible to a Brain Platform session go 104 -> 141 (+37), and the 0.9009-similarity item that motivated the investigation now matches. +37 rather than +101 because the remaining Default rows are scope='project' and correctly stay bound — narrow by design, not a bucket merge. Also corrects KNOWN_ISSUES §0p, whose first version named the wrong mechanism: it cited buildRawProjectFilter (V1) and claimed Knowledge.visibility does not govern this path. Production uses buildRawProjectFilterV2, which is visibility-driven. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rqj5y9fT3XPUqnqKyustp9
|
Warning Review limit reached
Next review available in: 51 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 (2)
📝 WalkthroughWalkthroughAdds an opt-in ChangesUser-scope retrieval
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant fetchCandidates
participant buildContext
participant buildRawProjectFilterV2
participant KnowledgeDatabase
fetchCandidates->>buildRawProjectFilterV2: enable cross-project user scope
buildContext->>buildRawProjectFilterV2: enable cross-project user scope
buildRawProjectFilterV2->>KnowledgeDatabase: query owner-matched user/global rows
KnowledgeDatabase-->>fetchCandidates: candidate rows
KnowledgeDatabase-->>buildContext: context rows
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/core/src/__tests__/scope-filter.test.ts (1)
172-242: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a regression test for
scope="all"in the raw-SQL filter.
buildKnowledgeWhereV2supportsincludeUserScopeAcrossProjectsforscope="all", butbuildRawProjectFilterV2ignores the flag in that branch. There is no current production call site that passesscope: "all"with that opt-in, but the publicDataScopetype allows it — a small regression assertion documenting the intended raw-SQL behavior would prevent API drift.🤖 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/src/__tests__/scope-filter.test.ts` around lines 172 - 242, The tests only cover project scope, so add a regression case in the buildRawProjectFilterV2 suite using args with scope set to "all" and includeUserScopeAcrossProjects enabled. Assert the generated SQL includes the USER_SCOPE pattern, confirming the opt-in admits user/global rows for the public DataScope variant.
🤖 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/src/scope-filter.ts`:
- Line 318: Apply includeUserScopeAcrossProjects consistently across every
raw-filter branch in the scope-filter query builder, including the branches
around userScopeSql and the additional branches near the other referenced
conditions. Ensure opted-in user/global rows are included for scope "all" with
accessible projects, while scope "project" without an active project excludes
them unless the flag is true, matching buildKnowledgeWhereV2 behavior.
---
Nitpick comments:
In `@packages/core/src/__tests__/scope-filter.test.ts`:
- Around line 172-242: The tests only cover project scope, so add a regression
case in the buildRawProjectFilterV2 suite using args with scope set to "all" and
includeUserScopeAcrossProjects enabled. Assert the generated SQL includes the
USER_SCOPE pattern, confirming the opt-in admits user/global rows for the public
DataScope variant.
🪄 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 Plus
Run ID: 3f900e58-139b-451d-8b93-c0894f46a59c
📒 Files selected for processing (5)
docs/KNOWN_ISSUES.mdpackages/core/src/__tests__/scope-filter.test.tspackages/core/src/kra.tspackages/core/src/oracle.tspackages/core/src/scope-filter.ts
…oundary CodeRabbit review on #177 flagged the opt-in as inconsistent. Half right, and the half that isn't matters. Accepted: scope="all" with an accessible-project list DID omit opted-in user/global rows. That branch has a project boundary, so the flag should reach it. Fixed, with a test. Rejected: gating the no-activeProjectId branch. That branch includes user/global rows unconditionally, and deliberately — with no active project there is no boundary to enforce, and the behaviour predates this flag (added 2026-05-12 after 5/5 retrieval misses traced to its absence). Gating it would resurrect that bug for every caller that does not opt in. The flag's doc now states exactly which branches it governs and why the other two are excluded, rather than overclaiming a blanket default the code never had. Also replaces the sentinel-string construction with a small `userScope(pUser, indent)` helper so all three call sites emit the identical, always-user-pinned disjunct. Verified the generated SQL for all four shapes: parens balanced, IN-list intact, and the bind index correctly differs between the "all" ([userId, ...projects]) and "project" ([projectId, userId, ...]) param orderings. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rqj5y9fT3XPUqnqKyustp9
5 tasks
6 tasks
6 tasks
bejranonda
added a commit
that referenced
this pull request
Jul 31, 2026
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.
Claude-Session: https://claude.ai/code/session_01Rqj5y9fT3XPUqnqKyustp9
Co-authored-by: Claude <noreply@anthropic.com>
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.
Closes #174.
What
brain_teach_knowledgedefaultsscopeto"user", but a row written frominside a project also gets that project's
ownerProjectIdand a defaultvisibilityof"project"— so it was invisible from every other project.Symptom: "the brain has knowledge but retrieval returns nothing."
This was not a new design question
I opened #174 saying the fix needed a design call. Reading further, it didn't:
scope-filter.ts's no-active-project branch already carries exactly thisfix, added 2026-05-12 after "5/5 retrieval misses traced to this branch".
The two active-project branches never got it.
The project had already decided. The decision was applied to one branch out of three.
Fix
The same
scope IN ('user','global') AND "ownerUserId" = $userdisjunct, gatedbehind a new opt-in
includeUserScopeAcrossProjectsflag (defaultfalse),enabled only at the two personal-rule retrieval sites (
kra.ts,oracle.ts).The opt-in is load-bearing, not ceremony. Two other callers of the same
shared helper depend on the project edge holding:
action-items.tsmeeting-extract.tsFlipping the shared helper for every caller is the exact mistake that file's own
comment warns about (GUIDELINES §7: give cross-scope behaviour an explicit path
rather than quietly changing a shared function).
Cross-project reach never becomes cross-user reach
ownerUserIdpredicate — it sits insidean
OR, so an outerANDwould not constrain the sibling arms. A test pins this.WHERE "ownerUserId" = $2in theouter query (
kra.ts,oracle.ts).its top-level
AND "ownerUserId" = $n.Measured on the live corpus (read-only)
Brain PlatformsessionThe 0.9009-similarity item that motivated the whole investigation
(
"Adding a user-facing concept or glossary page", 100% success over 11 uses)now matches. +37 rather than +101 because the remaining
Defaultrows arescope='project'and correctly stay bound — narrow by design, not a bucket merge.Also corrects the record
KNOWN_ISSUES §0p's first version named the wrong mechanism: it citedbuildRawProjectFilter(V1) and asserted that "Knowledge.visibilityis aseparate Phase-4 field and does not govern this path." Production uses
buildRawProjectFilterV2, which is visibility-driven. Corrected here, with thecorrection called out rather than quietly rewritten.
Test plan
cases had no
scope INclause while the no-project case already did — theprecedent and the gap, in one run. 23/23 green after.
packages/core/src/__tests__/scope-filter.test.ts— 23 passing vianpx vitest@2 run <file>(standalone; no workspace resolution needed).The file had zero coverage of
buildRawProjectFilterV2before this PR,despite it being the filter production retrieval actually uses.
and the disjunct pinned to
ownerUserId.org, default) — parens balanced, params correct,
$5,$6IN-list intact, andthe default branch verified to contain no
scope INclause.indices in the calling queries can't drift. Pinned by a test.
specific missing row is
scope=user, visibility=project, project=Default.buildRawProjectFilterV2/buildKnowledgeWhereV2callerbefore choosing opt-in over a blanket change.
for
cross-user-isolation.test.ts— the DB-gated IDOR regression net. Thatsuite is the one I most want green here and cannot run locally.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Rqj5y9fT3XPUqnqKyustp9
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests