Skip to content

fix cross-project memory leakage#662

Merged
rohitg00 merged 5 commits into
rohitg00:mainfrom
abhinav-m22:fix/memory-project-scope
May 27, 2026
Merged

fix cross-project memory leakage#662
rohitg00 merged 5 commits into
rohitg00:mainfrom
abhinav-m22:fix/memory-project-scope

Conversation

@abhinav-m22
Copy link
Copy Markdown
Contributor

@abhinav-m22 abhinav-m22 commented May 26, 2026

Memories saved under one project were leaking into completely unrelated projects during tool enrichment and semantic search. this fixes the write path, the read path, and the search index filtering so project boundaries are actually respected.

  • added project field to the Memory type and stamped it on every write path (remember, consolidate, MCP, REST)
  • scoped bug memory filtering in mem::enrich so a Next.js project no longer sees Express middleware debug history just because they share a filename
  • fixed the synthetic sessionId path in mem::search so project-filtered searches correctly include or exclude memories instead of silently dropping them

Demo

before after
memory write project comes back null even when explicitly passed project is stamped correctly on every save
before write after write
enrich web project receives express-jwt bug history it has nothing to do with each project sees only its own bugs, no crossover
before enrich after enrich
search silently dropping scoped memories or returning unfiltered results web search returns empty, api search returns only its own memory
n/a after search

Summary by CodeRabbit

  • New Features

    • Memory project scoping: optional project boundaries for save/search/enrich/consolidate flows with request-level validation.
    • Migration step to infer missing memory projects from session history.
    • Diagnostic check reporting memory-project coverage.
  • Tests

    • Expanded test suites covering project scoping, cross-project isolation, consolidation behavior, remember/enrich/search interactions, and the infer-projects migration.

Review Change Stack

@vercel
Copy link
Copy Markdown

vercel Bot commented May 26, 2026

@abhinav-m22 is attempting to deploy a commit to the rohitg00's projects Team on Vercel.

A member of the Team first needs to authorize it.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 26, 2026

📝 Walkthrough

Walkthrough

This PR implements project scoping for the memory system to prevent memories from leaking across unrelated projects. It adds an optional project field to the Memory type and threads project awareness through remember, consolidate, enrich, search, migration, and API layers, including project-aware dedup isolation, scoped evolution, filtered queries, backfill inference, validation checks, and comprehensive test coverage.

Changes

Project-scoped memory isolation

Layer / File(s) Summary
Memory type with project field
src/types.ts
The Memory interface gains an optional project?: string field, establishing the core type contract for all downstream project-scoping logic.
Remember: save with project and dedup isolation
src/functions/remember.ts, test/remember-project-scope.test.ts
mem::remember accepts optional project, persists it to saved memories (with trimming and blank-as-undefined handling), prevents superseding across different projects while preserving legacy unscoped-memory compatibility, and logs project with completion. Tests validate field stamping and cross-project dedup isolation.
Consolidate: scoped evolution
src/functions/consolidate.ts, test/consolidate-project-scope.test.ts
mem::consolidate derives scopedProject, restricts existing-memory matching and evolution to same-project boundaries, and conditionally assigns project on evolved and newly created memories. Tests cover cross-project protection, in-project evolution, unscoped consolidation fallback, and project stamping.
Enrich: project-scoped bug memory filtering
src/functions/enrich.ts, test/enrich-project-isolation.test.ts
mem::enrich accepts optional project, filters bug memories to same project (with legacy unscoped compatibility), forwards project to mem::search, and logs project. Tests verify scoped vs unscoped visibility, multi-project isolation, latest-only filtering, and search forwarding.
Search: project-scoped candidate filtering
src/functions/search.ts
First-pass filtering extends to memory-indexed observations by introducing per-request cache and loader that probes KV.memories for project when session lookup fails, preserving null/unknown as safe pass-through.
Migration: infer projects from sessions
src/functions/migrate.ts, test/infer-memory-projects.test.ts
Adds inferMemoryProjects that scans unscoped memories, loads referenced sessions, and assigns project via strict-majority voting; updates mem::migrate to accept optional step and dryRun parameters with validation. Tests cover skipping, ambiguity detection, voting logic, dry-run mode, batch processing, and idempotency.
Diagnostics: project coverage check
src/functions/diagnostics.ts, test/diagnostics.test.ts
Adds "memory-project-coverage" diagnostic that evaluates latest unscoped memories and reports pass/warn/fail with migration guidance; updates "memories-ok" message language. Test updates expected check count.
API trigger contracts: enrich, remember, migrate
src/triggers/api.ts
Updates HTTP API request schemas to accept optional project for enrich and remember (with non-empty trimmed validation), adds ttlDays and sourceObservationIds to remember, and reworks migrate to accept optional step and dryRun parameters.
Hook and tool integration: MCP project threading
src/hooks/pre-tool-use.ts, src/mcp/server.ts, src/mcp/tools-registry.ts
pre-tool-use.ts conditionally includes project in enrichment requests, mcp/server.ts threads optional project from memory_save tool into mem::remember payload, and tools-registry.ts adds optional project field to memory_save schema.
End-to-end cross-project isolation tests
test/cross-project-isolation.test.ts
Comprehensive Vitest suite verifies project isolation across remember, enrich, and search: scoped memories hidden from different projects, visible to same project, legacy unscoped memories visible to all, multiple projects with overlapping filenames isolated, and Jaccard dedup respects project boundaries.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Suggested reviewers

  • rohitg00

Poem

🐰 Across the warren of memories stored,
Projects now have their own secret door—
No more whispers from the unrelated heap,
Each codebase's lessons they safely keep.
The leaks are patched, the scopes are clear!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 3.45% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix cross-project memory leakage' clearly and concisely summarizes the main objective of the PR, which is to prevent memories from one project leaking into other projects.
Linked Issues check ✅ Passed The PR comprehensively addresses all key coding requirements from issue #656: adds project field to Memory type, scopes bug memory filtering in mem::enrich, fixes project filtering in mem::search, and prevents cross-project consolidation.
Out of Scope Changes check ✅ Passed All changes are directly scoped to addressing cross-project memory leakage: project field addition, filtering/scoping logic in consolidate/enrich/search/remember, and corresponding test coverage. No unrelated modifications detected.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint skipped: no ESLint configuration detected in root package.json. To enable, add eslint to devDependencies.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Owner

@rohitg00 rohitg00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Direction is right and tests cover the read/write paths well. Three things before merge.

Blocking — test/diagnostics.test.ts:201 expects summary.pass === 14, the new memory-project-coverage check makes it 15. Bump the assertion.

Cross-project supersede in consolidateconsolidate.ts:155 finds existingMatch purely by title across all memories. A project-B consolidation with the same title as a project-A memory will mark project-A's memory non-latest and emit a project-B successor. Same wildcard guard you used on remember should apply here: skip the existingMatch when both sides have an explicit, mismatched project.

Hook fallback uses cwd as projectpre-tool-use.ts:81 falls back to data.cwd when data.project is absent. Sessions register project and cwd as distinct fields (project = name, cwd = absolute path), so this fallback produces a "project" identifier that no future enrich call ever matches. Either omit project on the wire when only cwd is available (let the server resolve from the session) or drop the cwd branch entirely.

After those, happy to merge.

@thekoma
Copy link
Copy Markdown

thekoma commented May 26, 2026

@rohitg00 picking up on point 3 in your review — "project = name, cwd = absolute path" is exactly the framing I just argued for in #656 from a different angle: #656 (comment)

The pre-tool-use fallback you flagged is downstream of a deeper issue: plugin/scripts/session-start.mjs:30 already does project = data.cwd || process.cwd() when registering the session itself. So the session record gets created with the path as its "name", which means even with your suggested "let the server resolve from the session" fix to pre-tool-use, the same repo cloned to two different absolute paths produces two distinct projects in storage. Same root cause, surfacing at a different hook.

What I sketched in #656 lines up with that "project = name" framing: a client-side resolver in a new helper, with AGENTMEMORY_PROJECT env → git remote get-url origincwd fallback, consistent with src/hooks/post-commit.ts:65. pre-tool-use, pre-compact, and session-start all import the same resolver, no per-hook ad-hoc fallback to maintain.

Not asking @abhinav-m22 to fold this into #662 — the PR is already doing enough work and the three review points should land first. Once #662 merges I can put up a follow-up against the project field this patch introduces.

@abhinav-m22 abhinav-m22 marked this pull request as ready for review May 27, 2026 11:13
@abhinav-m22
Copy link
Copy Markdown
Contributor Author

@rohitg00 thanks for the detailed review. PR is now marked ready for review and description is updated with the before/after demo.

all three blockers are addressed:

  • removed the cwd fallback in pre-tool-use.ts. only forwarding explicit project values now so cwd paths can't silently shadow short project names
  • added the same wildcard guard to consolidate.ts existingMatch so a same-title consolidation from project B can no longer mark project A's memory non-latest

also caught one thing while fixing:

  • temporal dead zone in consolidate. scopedProject was referenced before its declaration, would have thrown at runtime

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/functions/search.ts (1)

347-348: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Trim project/cwd filters before applying search scope.

Line 347 and Line 348 treat whitespace-padded values as valid filters (e.g. " api "), which can cause false mismatches and inconsistent scoping.

Proposed fix
-      const projectFilter = typeof data.project === 'string' && data.project.length > 0 ? data.project : undefined
-      const cwdFilter = typeof data.cwd === 'string' && data.cwd.length > 0 ? data.cwd : undefined
+      const projectFilter =
+        typeof data.project === 'string' && data.project.trim().length > 0
+          ? data.project.trim()
+          : undefined
+      const cwdFilter =
+        typeof data.cwd === 'string' && data.cwd.trim().length > 0
+          ? data.cwd.trim()
+          : undefined
🤖 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 `@src/functions/search.ts` around lines 347 - 348, project and cwd filters are
currently accepted with surrounding whitespace, causing mismatches; update the
logic that sets projectFilter and cwdFilter to trim data.project and data.cwd
first and only use the trimmed value if its length > 0 (otherwise set
undefined). Locate the assignments to projectFilter and cwdFilter in the search
function (where data.project and data.cwd are read) and replace the current
checks with trimmed-value checks so downstream scope matching uses the cleaned
strings.
🧹 Nitpick comments (4)
src/functions/search.ts (1)

381-385: ⚡ Quick win

Replace WHAT-style explanatory comments with clearer helper naming.

Line 381–Line 385 and Line 407–Line 420 add long behavior-explainer comments in src/. This should be expressed through function/variable naming and concise intent-only comments.

As per coding guidelines, "Avoid code comments explaining WHAT — use clear naming instead."

Also applies to: 407-420

🤖 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 `@src/functions/search.ts` around lines 381 - 385, The long WHAT-style comments
describing synthetic memory session behavior should be removed and replaced by
clearer naming and small helpers: extract the logic around synthetic session IDs
and project lookups into a well-named helper (e.g., isSyntheticMemorySession or
getMemoryProjectForSession) and rename the cache to something explicit (e.g.,
memoryProjectCache) so the intent is obvious; keep only a brief intent-only
comment (one line) near calls to loadSession and the KV.memories probe that
states why we fall through (e.g., "fall back to KV.memories when session has no
project"), and update usages of sessionId and mem::remember to call the new
helper instead of relying on long block comments.
src/functions/enrich.ts (1)

74-77: ⚡ Quick win

Remove implementation-explainer comments in this block.

Line 74–Line 77 add WHAT-style comments in src/, which conflicts with the repository rule. Prefer self-descriptive naming and keep comments minimal.

As per coding guidelines, "Avoid code comments explaining WHAT — use clear naming instead."

🤖 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 `@src/functions/enrich.ts` around lines 74 - 77, Remove the WHAT-style
explanatory comments in the block that filters memories by project in
src/functions/enrich.ts (the lines describing "Only include memories whose
project matches the caller's..." and the two following lines); instead make the
intent explicit via clearer identifiers and a concise one-line rationale if
needed—e.g., rename variables or helper functions involved in the project-scope
check (the memory filtering logic inside the enrich function that references
"project" and "memories") so the code reads self-documenting, and then delete
the explanatory comment block entirely.
test/consolidate-project-scope.test.ts (1)

213-216: 💤 Low value

Consider asserting the evolved memory's project field.

The test verifies the old memory is marked non-latest but doesn't check whether the newly evolved memory has a project field. Adding this assertion would document the expected behavior (evolved memory is unscoped) and catch any future regressions.

💡 Suggested additional assertions
     // The existing scoped memory should have been evolved (unscoped run is unrestricted)
     const old = await kv.get<Memory>(KV.memories, scopedMemory.id);
     expect(old?.isLatest).toBe(false);
+
+    // The evolved memory should be unscoped (preserves pre-existing behavior)
+    const allMemories = await kv.list<Memory>(KV.memories);
+    const evolved = allMemories.find((m) => m.isLatest && m.parentId === scopedMemory.id);
+    expect(evolved).toBeDefined();
+    expect(evolved?.project).toBeUndefined();
   });
🤖 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 `@test/consolidate-project-scope.test.ts` around lines 213 - 216, Fetch the
newly evolved memory (e.g., using kv.get<Memory>(KV.memories, scopedMemory.id)
or the same read pattern used for `old`) and add an assertion that its `project`
field is unset to document that the evolved memory is unscoped (for example
`expect(newMem?.project).toBeUndefined()`); also ensure you assert the new
memory is marked latest (e.g., `expect(newMem?.isLatest).toBe(true)`) so the
test verifies both the evolved scope and latest flag for `scopedMemory.id`.
src/functions/migrate.ts (1)

53-57: ⚡ Quick win

Parallelize session lookups during project voting.

These KV reads are independent; serial awaits will slow migration significantly on large datasets. Batch them with Promise.all().

♻️ Proposed change
-    const projects: string[] = [];
-    for (const sid of sessionIds) {
-      const session = await loadSession(sid);
-      if (session?.project) projects.push(session.project);
-    }
+    const sessions = await Promise.all(sessionIds.map((sid) => loadSession(sid)));
+    const projects = sessions
+      .map((session) => session?.project)
+      .filter((project): project is string => Boolean(project));

As per coding guidelines: "Use parallel operations with Promise.all() for independent kv writes/reads".

🤖 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 `@src/functions/migrate.ts` around lines 53 - 57, The loop that reads sessions
serially (sessionIds -> await loadSession(...)) is slow; change it to run loads
in parallel by mapping sessionIds to loadSession(sid) promises, await
Promise.all(...) to get all sessions, then extract session.project values into
the projects array (preserve existing behavior of pushing only when
session?.project is present). Update references around the projects variable and
the sessionIds usage so the code uses the resulting sessions array instead of
serial awaits.
🤖 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/remember.ts`:
- Line 23: The dedup/supersede checks use raw data.project (data.project) before
you normalize/validate it later, causing inconsistent matching for
whitespace/blank/non-string inputs; move normalization/validation of the
incoming project into the top of the remember handler (normalize the value
currently referenced as data.project into a local variable like
projectNormalized or project) before any dedupe/supersede logic (the blocks
around the supersede comparison and lines handling 63-73 and 94-97), ensure
non-string or blank values are coerced/treated consistently (e.g., to null or
trimmed empty string) and then use that normalized variable in all subsequent
comparisons and storage operations.

In `@src/mcp/tools-registry.ts`:
- Around line 78-83: The current description for the project field ("Project
identifier this memory belongs to (e.g. the project name or path)...")
encourages using filesystem paths; update the description text for the project
field in the project schema to explicitly request a stable, canonical project
identifier (for example a slug, UUID, or registry key) and discourage using
filesystem paths or volatile names; modify the description string assigned to
the project property so it recommends "a stable canonical ID (e.g. slug or UUID)
— not a filesystem path or ad-hoc display name" and mentions that this ID is
used for scoping across clients (see the project field in the schema).

In `@src/triggers/api.ts`:
- Line 880: The handler is forwarding req.body directly into sdk.trigger (e.g.,
the call with function_id "mem::enrich"), which allows unvalidated keys through;
instead, explicitly construct a whitelisted payload object with only the
expected fields (e.g., { userId, content, metadata } or whatever the handler
validates) and pass that object to sdk.trigger; apply the same pattern to the
other trigger calls (the sdk.trigger invocations for "mem::remember" and
"mem::migrate") so only permitted fields are forwarded and any extra keys on
req.body are dropped.

In `@test/remember-project-scope.test.ts`:
- Around line 1-14: The test fails because src/functions/remember.ts uses
TriggerAction.Void from iii-sdk but the test file doesn't mock iii-sdk; add a
vi.mock("iii-sdk", ...) at the top of test/remember-project-scope.test.ts
alongside the other vi.mock calls that stubs TriggerAction.Void (e.g., provide a
TriggerAction object with a Void function) so the test doesn't call the real SDK
when registerRememberFunction triggers superseding memories.

---

Outside diff comments:
In `@src/functions/search.ts`:
- Around line 347-348: project and cwd filters are currently accepted with
surrounding whitespace, causing mismatches; update the logic that sets
projectFilter and cwdFilter to trim data.project and data.cwd first and only use
the trimmed value if its length > 0 (otherwise set undefined). Locate the
assignments to projectFilter and cwdFilter in the search function (where
data.project and data.cwd are read) and replace the current checks with
trimmed-value checks so downstream scope matching uses the cleaned strings.

---

Nitpick comments:
In `@src/functions/enrich.ts`:
- Around line 74-77: Remove the WHAT-style explanatory comments in the block
that filters memories by project in src/functions/enrich.ts (the lines
describing "Only include memories whose project matches the caller's..." and the
two following lines); instead make the intent explicit via clearer identifiers
and a concise one-line rationale if needed—e.g., rename variables or helper
functions involved in the project-scope check (the memory filtering logic inside
the enrich function that references "project" and "memories") so the code reads
self-documenting, and then delete the explanatory comment block entirely.

In `@src/functions/migrate.ts`:
- Around line 53-57: The loop that reads sessions serially (sessionIds -> await
loadSession(...)) is slow; change it to run loads in parallel by mapping
sessionIds to loadSession(sid) promises, await Promise.all(...) to get all
sessions, then extract session.project values into the projects array (preserve
existing behavior of pushing only when session?.project is present). Update
references around the projects variable and the sessionIds usage so the code
uses the resulting sessions array instead of serial awaits.

In `@src/functions/search.ts`:
- Around line 381-385: The long WHAT-style comments describing synthetic memory
session behavior should be removed and replaced by clearer naming and small
helpers: extract the logic around synthetic session IDs and project lookups into
a well-named helper (e.g., isSyntheticMemorySession or
getMemoryProjectForSession) and rename the cache to something explicit (e.g.,
memoryProjectCache) so the intent is obvious; keep only a brief intent-only
comment (one line) near calls to loadSession and the KV.memories probe that
states why we fall through (e.g., "fall back to KV.memories when session has no
project"), and update usages of sessionId and mem::remember to call the new
helper instead of relying on long block comments.

In `@test/consolidate-project-scope.test.ts`:
- Around line 213-216: Fetch the newly evolved memory (e.g., using
kv.get<Memory>(KV.memories, scopedMemory.id) or the same read pattern used for
`old`) and add an assertion that its `project` field is unset to document that
the evolved memory is unscoped (for example
`expect(newMem?.project).toBeUndefined()`); also ensure you assert the new
memory is marked latest (e.g., `expect(newMem?.isLatest).toBe(true)`) so the
test verifies both the evolved scope and latest flag for `scopedMemory.id`.
🪄 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: b6dce31e-1ee1-4aea-9a12-4e4bafdf0069

📥 Commits

Reviewing files that changed from the base of the PR and between 6939d4a and ee710f5.

📒 Files selected for processing (17)
  • src/functions/consolidate.ts
  • src/functions/diagnostics.ts
  • src/functions/enrich.ts
  • src/functions/migrate.ts
  • src/functions/remember.ts
  • src/functions/search.ts
  • src/hooks/pre-tool-use.ts
  • src/mcp/server.ts
  • src/mcp/tools-registry.ts
  • src/triggers/api.ts
  • src/types.ts
  • test/consolidate-project-scope.test.ts
  • test/cross-project-isolation.test.ts
  • test/diagnostics.test.ts
  • test/enrich-project-isolation.test.ts
  • test/infer-memory-projects.test.ts
  • test/remember-project-scope.test.ts

Comment thread src/functions/remember.ts
Comment thread src/mcp/tools-registry.ts
Comment thread src/triggers/api.ts Outdated
Comment thread test/remember-project-scope.test.ts
@rohitg00
Copy link
Copy Markdown
Owner

Please fix coderabbit issues @abhinav-m22

@abhinav-m22
Copy link
Copy Markdown
Contributor Author

addressed all 4 coderabbit comments:

fixed

  • remember.ts : hoisted project normalization above the dedup loop so raw data.project is never used in comparisons; added vi.mock("iii-sdk") in the test file to stub TriggerAction.Void — 4 tests exercise the supersede path and were silently calling the real SDK
  • api.ts : all three trigger calls now forward explicit whitelisted payloads; req.body is no longer passed through directly

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (3)
src/triggers/api.ts (3)

1023-1040: ⚡ Quick win

Trim validated string fields at the API boundary.

The validation checks that step and dbPath are non-empty when trimmed (lines 1023-1026), but lines 1036-1037 forward them without trimming. Normalize at the API boundary for consistency.

♻️ Trim step and dbPath before forwarding
       const hasStep =
         typeof req.body?.step === "string" && req.body.step.trim().length > 0;
       const hasDbPath =
         typeof req.body?.dbPath === "string" && req.body.dbPath.trim().length > 0;
       if (!hasStep && !hasDbPath) {
         return {
           status_code: 400,
           body: { error: "Either step (string) or dbPath (string) is required" },
         };
       }
+      const step = hasStep ? req.body.step.trim() : undefined;
+      const dbPath = hasDbPath ? req.body.dbPath.trim() : undefined;
       const result = await sdk.trigger({
         function_id: "mem::migrate",
         payload: {
-          ...(req.body.step !== undefined && { step: req.body.step }),
-          ...(req.body.dbPath !== undefined && { dbPath: req.body.dbPath }),
+          ...(step !== undefined && { step }),
+          ...(dbPath !== undefined && { dbPath }),
           ...(req.body.dryRun !== undefined && { dryRun: req.body.dryRun }),
         },
       });

As per coding guidelines: "Perform input validation at system boundaries (MCP handlers and REST endpoints)."

🤖 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 `@src/triggers/api.ts` around lines 1023 - 1040, Validation trims were used for
hasStep/hasDbPath but the original req.body values are forwarded; update the API
boundary to normalize by trimming before sending to sdk.trigger. After
determining hasStep/hasDbPath, derive trimmed variables (e.g., const step =
typeof req.body?.step === "string" ? req.body.step.trim() : undefined; same for
dbPath) and use those trimmed values in the payload passed to sdk.trigger({
function_id: "mem::migrate", payload: { ...(step !== undefined && { step }),
...(dbPath !== undefined && { dbPath }), ...(req.body.dryRun !== undefined && {
dryRun: req.body.dryRun }) } }). Ensure hasStep/hasDbPath checks use the trimmed
variables or remain consistent with them.

920-937: ⚡ Quick win

Trim validated string fields at the API boundary.

Similar to the api::enrich handler, the validation at lines 921-922 checks !req.body.project.trim() to ensure non-empty when trimmed, but line 935 forwards req.body.project without trimming. Apply the same normalization pattern here.

♻️ Trim project before forwarding
       if (
         req.body.project !== undefined &&
         (typeof req.body.project !== "string" || !req.body.project.trim())
       ) {
         return { status_code: 400, body: { error: "project must be a non-empty string" } };
       }
+      const project =
+        typeof req.body.project === "string" && req.body.project.trim()
+          ? req.body.project.trim()
+          : undefined;
       const result = await sdk.trigger({
         function_id: "mem::remember",
         payload: {
           content: req.body.content,
           ...(req.body.type !== undefined && { type: req.body.type }),
           ...(req.body.concepts !== undefined && { concepts: req.body.concepts }),
           ...(req.body.files !== undefined && { files: req.body.files }),
           ...(req.body.ttlDays !== undefined && { ttlDays: req.body.ttlDays }),
           ...(req.body.sourceObservationIds !== undefined && { sourceObservationIds: req.body.sourceObservationIds }),
-          ...(req.body.project !== undefined && { project: req.body.project }),
+          ...(project !== undefined && { project }),
         },
       });

As per coding guidelines: "Perform input validation at system boundaries (MCP handlers and REST endpoints)."

🤖 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 `@src/triggers/api.ts` around lines 920 - 937, The handler validates that
req.body.project is a non-empty trimmed string but then forwards the untrimmed
value to sdk.trigger; modify the API boundary to normalize the field first
(e.g., compute a trimmedProject = req.body.project?.trim()) and use that trimmed
variable when building the payload for sdk.trigger (reference the payload
construction around function_id "mem::remember" and the project validation
block) so the forwarded project is the normalized, trimmed string and you only
include it when trimmedProject is present.

871-889: ⚡ Quick win

Trim validated string fields at the API boundary.

The validation at lines 872-873 checks !req.body.project.trim() to ensure the project is non-empty when trimmed, but line 887 forwards req.body.project without actually trimming it. If the request contains "project": " api ", the validation passes but the untrimmed value is forwarded to mem::enrich. As per coding guidelines, input validation (including normalization) should happen at system boundaries.

♻️ Trim project before forwarding
       if (
         req.body.project !== undefined &&
         (typeof req.body.project !== "string" || !req.body.project.trim())
       ) {
         return {
           status_code: 400,
           body: { error: "project must be a non-empty string" },
         };
       }
+      const project =
+        typeof req.body.project === "string" && req.body.project.trim()
+          ? req.body.project.trim()
+          : undefined;
       const result = await sdk.trigger({
         function_id: "mem::enrich",
         payload: {
           sessionId: req.body.sessionId,
           files: req.body.files,
           ...(req.body.terms !== undefined && { terms: req.body.terms }),
           ...(req.body.toolName !== undefined && { toolName: req.body.toolName }),
-          ...(req.body.project !== undefined && { project: req.body.project }),
+          ...(project !== undefined && { project }),
         },
       });

As per coding guidelines: "Perform input validation at system boundaries (MCP handlers and REST endpoints)."

🤖 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 `@src/triggers/api.ts` around lines 871 - 889, The API validates that
req.body.project is a non-empty trimmed string but forwards the original
untrimmed value; update the request handling so you normalize by trimming
project at the API boundary before calling sdk.trigger (i.e., derive a
trimmedProject = req.body.project.trim() when project is present and use that
trimmedProject in the payload for sdk.trigger with function_id "mem::enrich"),
ensuring any downstream logic (payload construction around project) references
the trimmed value instead of req.body.project.
🤖 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.

Nitpick comments:
In `@src/triggers/api.ts`:
- Around line 1023-1040: Validation trims were used for hasStep/hasDbPath but
the original req.body values are forwarded; update the API boundary to normalize
by trimming before sending to sdk.trigger. After determining hasStep/hasDbPath,
derive trimmed variables (e.g., const step = typeof req.body?.step === "string"
? req.body.step.trim() : undefined; same for dbPath) and use those trimmed
values in the payload passed to sdk.trigger({ function_id: "mem::migrate",
payload: { ...(step !== undefined && { step }), ...(dbPath !== undefined && {
dbPath }), ...(req.body.dryRun !== undefined && { dryRun: req.body.dryRun }) }
}). Ensure hasStep/hasDbPath checks use the trimmed variables or remain
consistent with them.
- Around line 920-937: The handler validates that req.body.project is a
non-empty trimmed string but then forwards the untrimmed value to sdk.trigger;
modify the API boundary to normalize the field first (e.g., compute a
trimmedProject = req.body.project?.trim()) and use that trimmed variable when
building the payload for sdk.trigger (reference the payload construction around
function_id "mem::remember" and the project validation block) so the forwarded
project is the normalized, trimmed string and you only include it when
trimmedProject is present.
- Around line 871-889: The API validates that req.body.project is a non-empty
trimmed string but forwards the original untrimmed value; update the request
handling so you normalize by trimming project at the API boundary before calling
sdk.trigger (i.e., derive a trimmedProject = req.body.project.trim() when
project is present and use that trimmedProject in the payload for sdk.trigger
with function_id "mem::enrich"), ensuring any downstream logic (payload
construction around project) references the trimmed value instead of
req.body.project.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 99900fe7-804f-444d-814e-2c991d2d309a

📥 Commits

Reviewing files that changed from the base of the PR and between ee710f5 and b702f9c.

📒 Files selected for processing (7)
  • src/functions/enrich.ts
  • src/functions/remember.ts
  • src/functions/search.ts
  • src/mcp/tools-registry.ts
  • src/triggers/api.ts
  • test/consolidate-project-scope.test.ts
  • test/remember-project-scope.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
  • src/mcp/tools-registry.ts
  • test/remember-project-scope.test.ts
  • src/functions/search.ts
  • test/consolidate-project-scope.test.ts
  • src/functions/enrich.ts

Copy link
Copy Markdown
Owner

@rohitg00 rohitg00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All three concerns from the prior review verified fixed against head:

  • diagnostics.test.ts:202 bumped to expect(15) — passes
  • consolidate.ts:171 existingMatch now carries the wildcard guard ((!scopedProject || !m.project || m.project === scopedProject)) so project-A and project-B can't supersede each other via title collision
  • pre-tool-use.ts:85 dropped the cwd fallback entirely — only forwards an explicit data.project, which is the right call since cwd is an absolute path and project is a name; mixing them produced an identifier that no future enrich would match

Full suite locally: 1211 passed, 17 skipped, 1 pre-existing integration-test failure (needs running server, unrelated). The 45 new tests across cross-project-isolation, enrich-project-isolation, and remember-project-scope cover the read/write paths.

Ready to merge from my side. @rohitg00 please confirm before I run it.

Copy link
Copy Markdown
Owner

@rohitg00 rohitg00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All three concerns from the prior review verified fixed against head:

  • diagnostics.test.ts:202 bumped to expect(15) — passes
  • consolidate.ts:171 existingMatch now carries the wildcard guard ((!scopedProject || !m.project || m.project === scopedProject)) so project-A and project-B can't supersede each other via title collision
  • pre-tool-use.ts:85 dropped the cwd fallback entirely — only forwards an explicit data.project, which is the right call since cwd is an absolute path and project is a name; mixing them produced an identifier that no future enrich would match

Full suite locally: 1211 passed, 17 skipped, 1 pre-existing integration-test failure (needs running server, unrelated). The 45 new tests across cross-project-isolation, enrich-project-isolation, and remember-project-scope cover the read/write paths.

Ready to merge from my side — waiting on explicit greenlight before running it.

@rohitg00 rohitg00 merged commit e1d96bc into rohitg00:main May 27, 2026
5 of 6 checks passed
@rohitg00
Copy link
Copy Markdown
Owner

Very good work @abhinav-m22

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Memories leak across projects

3 participants