fix(smart-search): apply the project filter to results, not just lessons#869
fix(smart-search): apply the project filter to results, not just lessons#869jwfaye wants to merge 3 commits into
Conversation
mem::smart-search accepts a `project` argument but only forwards it to mem::lesson-recall — the primary observation results are filtered by agentId alone. A project-scoped search therefore returns observations from every other project, so callers cannot use `project` to isolate memory per concern. Apply the project filter wherever the agentId filter already runs: the expand-ids branch, the hybrid-results branch, and the over-fetch trigger (the BM25/vector index carries neither agentId nor project, so both need post-filter headroom). When `project` is omitted behaviour is unchanged. Adds two tests: a project-scoped search returns only the matching project's observations, and an unscoped search still returns all. Signed-off-by: Joseph Faye <josephwfaye@gmail.com>
|
@jwfaye is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughAdds optional project-based filtering to mem::smart-search: derives a trimmed ChangesProject-scoped observation filtering
Sequence Diagram(s)sequenceDiagram
participant Client
participant SmartSearch
participant KV_Session
participant KV_Memory
participant ResultsFilter
Client->>SmartSearch: POST /agentmemory/smart-search { query, project?, agentId? }
SmartSearch->>KV_Session: resolve hit.project (cached lookup)
KV_Session-->>SmartSearch: project or not found
SmartSearch->>KV_Memory: resolve hit.project (fallback)
KV_Memory-->>SmartSearch: project or null
SmartSearch->>ResultsFilter: apply filterAgentId and filterProject to hits
ResultsFilter-->>Client: filtered results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
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 |
![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
🤖 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 `@src/functions/smart-search.ts`:
- Around line 118-121: The trimmed/normalized variable filterProject is used for
observation filtering but the lesson recall call still passes raw data.project,
causing divergence on whitespace inputs; update the lesson recall invocation(s)
to pass filterProject instead of data.project (and similarly replace any raw
uses of data.project in the nearby recall logic) so both observation filtering
and lesson recall use the same normalized project value.
🪄 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
Run ID: c59ac139-a266-46ba-9cd9-16635e31ffb3
📒 Files selected for processing (2)
src/functions/smart-search.tstest/smart-search.test.ts
…cope The first pass filtered on r.observation.project, but the observation returned by the hybrid index does not carry the resolved project. Live testing showed a project-scoped search dropping ALL results — including those of the requested project. Resolve project the same way mem::search does: the session's project when the session exists, otherwise the memory's own KV.memories record (entries indexed via mem::remember use a synthetic sessionId with no session row). A null result means the project is unknown and the hit passes through (matches search.ts, backward compatible). Both lookups are cached. Tests now seed KV.memories and use the synthetic 'memory' sessionId so they exercise the real resolver path instead of a synthetic observation that already carries project. Addresses the CodeRabbit review: pass the normalized filterProject (not raw data.project) to recallLessons so observation filtering and lesson scoping agree on whitespace input. Signed-off-by: Joseph Faye <josephwfaye@gmail.com>
|
Follow-up commit ( What was wrong with the first commit. It post-filtered on The fix. Resolve project per hit the same way The tests now seed Verified live on a running instance after the fix:
@coderabbitai — addressed:
|
|
Tip For best results, initiate chat on the files or code changes.
Also noted on the original review point: passing normalized 🐇✨ ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
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 `@src/functions/smart-search.ts`:
- Around line 198-201: The loop calling resolveProject for each entry in
agentScoped is doing sequential KV reads; change it to run resolveProject in
parallel using Promise.all (e.g., map agentScoped to promises of
resolveProject(sessionId, observation.id)), await the array, then iterate
results to decide whether to push into kept (checking for null or equality with
filterProject); apply the same refactor for the similar block that references
resolveProject at the second occurrence (lines 255-257) so both sets of lookups
execute concurrently.
- Around line 90-92: The session KV read at sessionCache.set(sessionId, (await
kv.get<Session>(KV.sessions, sessionId)) ?? null) can reject and should degrade
to null; update the call that uses kv.get with error handling (e.g., wrap the
await in try/catch or use .catch(() => null)) so any rejection returns null
before the ?? runs, and keep using sessionCache, sessionId, kv.get and
KV.sessions as the referenced symbols so project-scoped search remains
resilient.
🪄 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
Run ID: 1655e671-c159-4fa9-8b33-180adee6fed6
📒 Files selected for processing (2)
src/functions/smart-search.tstest/smart-search.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/smart-search.test.ts
Address the CodeRabbit review on the project-resolution helper: - the session KV read now degrades to null on rejection (matching the memory lookup), so a transient state::get failure no longer fails the whole project-scoped search. - resolve each hit's project concurrently via Promise.all instead of an awaited serial loop, in both the expand-ids and hybrid branches; the lookups are independent and the resolver caches both reads. Signed-off-by: Joseph Faye <josephwfaye@gmail.com>
1 participant
What
mem::smart-searchaccepts aprojectargument but never applies it to its observation results — a project-scoped search returns observations from every project (the cross-project leak reported in #787).mem::searchalready scopes correctly; this bringssmart-searchin line, which also covers the MCPmemory_smart_search/memory_recalltools and/smart-searchthat route through it.How
Resolve each hit's project the way
mem::searchdoes — the session's project when the session exists, otherwise the memory's ownKV.memoriesrecord (entries indexed viamem::rememberuse a synthetic sessionId with no session row). Anullresult means the project is unknown and the hit passes through (backward-compatible). Applied in the expand-ids branch, the hybrid/compact path, and the over-fetch trigger; the normalizedfilterProjectis also passed to lesson recall. Resolution is cached and runs concurrently.Verify
npm run buildclean;test/smart-search.test.ts+test/agent-isolation-search.test.tsgreen (the tests seedKV.memoriesand use a syntheticmemorysessionId so they exercise the real resolver path, not a synthetic observation that already carriesproject).smart-search {project: "review"}returns the review observation;{project: "builder"}excludes it.Fixes #787