Add project isolation for agentmemory#829
Open
Durocks wants to merge 5 commits into
Open
Conversation
- Added project isolation support across various MCP endpoints and functions. - Introduced `resolveProjectScope` and `requireProjectScope` functions to handle project resolution and validation. - Updated existing endpoints in `server.ts`, `standalone.ts`, and `api.ts` to require a project argument when isolation is enabled. - Enhanced tools registry to include project as an optional parameter for memory operations. - Implemented filtering of memories and sessions based on the project in relevant functions. - Added tests to validate project isolation behavior, including handling of missing project arguments and scoped memory access. - Updated types to include optional project fields in memory interfaces.
|
@Durocks is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
📝 WalkthroughWalkthroughAdds project-scoped resolution and isolation: env/docs and config helper; enforces/resolves project at HTTP and MCP boundaries; threads scope-aware matching through memory functions, smart-search, consolidation; stamps project on persisted memories; updates CLI/MCP templates, plugin hooks, and tests. ChangesProject Isolation Implementation
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 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)
|
Durocks
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
test/mcp-standalone-proxy.test.ts (1)
21-25:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClean up
AGENTMEMORY_PROJECT_NAMEinbeforeEachto prevent test pollution.The
beforeEachhook deletesAGENTMEMORY_SECRETbut notAGENTMEMORY_PROJECT_NAME. If a test (e.g., line 121) setsAGENTMEMORY_PROJECT_NAMEand fails beforeafterEachcleanup runs, subsequent tests may inherit that value.🧹 Add cleanup for consistency
beforeEach(() => { resetHandleForTests(); process.env["AGENTMEMORY_URL"] = BASE; delete process.env["AGENTMEMORY_SECRET"]; + delete process.env["AGENTMEMORY_PROJECT_NAME"]; });🤖 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/mcp-standalone-proxy.test.ts` around lines 21 - 25, The beforeEach hook in test/mcp-standalone-proxy.test.ts currently calls resetHandleForTests() and cleans AGENTMEMORY_SECRET but does not clear AGENTMEMORY_PROJECT_NAME, which can cause cross-test pollution; update the beforeEach to also remove process.env["AGENTMEMORY_PROJECT_NAME"] (similar to the existing delete process.env["AGENTMEMORY_SECRET"]) so tests that set AGENTMEMORY_PROJECT_NAME (e.g., around line 121) cannot leak its value into later tests.src/functions/enrich.ts (1)
28-40:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPass
projectthrough tomem::file-context.
src/functions/file-index.tsnow callsrequireProjectScope(data?.project, "mem::file-context"), but this trigger payload still omitsproject. When isolation is enabled, the call throws and the local catch turns that into an empty file context, so scoped enrich requests quietly lose one of their main context sources.Suggested fix
.trigger<{ sessionId: string; files: string[] }, { context: string }>({ function_id: "mem::file-context", payload: { sessionId: data.sessionId, files: data.files, + ...(project !== undefined && { project }), }, })🤖 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 28 - 40, The mem::file-context trigger is missing the project field in its payload, so when isolation is enabled the call fails; update the sdk.trigger call that creates fileContextPromise to include the project (use the project returned by requireProjectScope) in the payload object passed to function_id "mem::file-context" so the invoked function receives the same project scope; specifically modify the fileContextPromise payload in enrich.ts to add project: project (or data.project if that pattern is used) and keep the existing catch behavior.src/functions/flow-compress.ts (1)
30-57:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winValidate
runIdandactionIdsagainst the resolved project.Only the
kv.list(KV.actions)branch is project-filtered. If a caller passes a foreignrunIdoractionIds, this will still compress those actions and then persist the result under the caller'sproject, which defeats the isolation this PR is introducing.Suggested fix
if (data.runId) { const run = await kv.get<RoutineRun>(KV.routineRuns, data.runId); if (!run) { return { success: false, error: "run not found" }; } for (const id of run.actionIds) { const action = await kv.get<Action>(KV.actions, id); - if (action) actionsToCompress.push(action); + if (action && (!project || action.project === project)) { + actionsToCompress.push(action); + } } } else if (data.actionIds && data.actionIds.length > 0) { for (const id of data.actionIds) { const action = await kv.get<Action>(KV.actions, id); - if (action) actionsToCompress.push(action); + if (action && (!project || action.project === project)) { + actionsToCompress.push(action); + } }🤖 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/flow-compress.ts` around lines 30 - 57, The code currently accepts a resolved project via requireProjectScope but still loads actions from a foreign runId or actionIds; ensure we only compress actions belonging to that resolved project: when handling a runId (RoutineRun), verify the run's project equals the resolved project and return an error if not, or alternatively filter its actionIds by fetching each Action (KV.actions) and only include actions whose action.project === project; when handling explicit actionIds, fetch each Action and only push those where action.project === project (or return an error for any mismatched id based on desired strictness). Update the run branch (where run.actionIds are iterated) and the actionIds branch (where data.actionIds are iterated) to perform this project check before adding to actionsToCompress, referencing requireProjectScope, KV.routineRuns, KV.actions, RoutineRun, and Action.src/functions/reflect.ts (1)
281-287:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winScope the insight fingerprint to the project.
fpis still derived from content alone, so two projects producing the same insight text hit the same KV key. A scopedmem::reflectrun will then reinforce or overwrite the other project's insight instead of creating its own record.Suggested fix
- const fp = fingerprintId("ins", content.trim().toLowerCase()); + const fp = fingerprintId( + "ins", + `${project ?? "__global__"}:${content.trim().toLowerCase()}`, + ); const existing = await kv.get<Insight>(KV.insights, fp);Also applies to: 300-306
🤖 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/reflect.ts` around lines 281 - 287, The fingerprinting is currently based on content only, causing cross-project collisions; update the fingerprint key generation to include the project scope (e.g., incorporate projectId or project slug) when calling fingerprintId so each project gets its own insight record. Concretely, change the fp computation used with kv.get/kv.set for KV.insights (the lines around fingerprintId("ins", content.trim().toLowerCase())) to include the project identifier (for example fingerprintId("ins", `${projectId}:${content.trim().toLowerCase()}`) or pass both projectId and content if fingerprintId supports multiple args), and make the same change for the other occurrence referenced (near reinforceInsight and the block at 300-306) so lookups and writes are consistently namespaced per project.
🧹 Nitpick comments (1)
test/mcp-standalone-proxy.test.ts (1)
79-119: 💤 Low valueClarify why the mocked
/agentmemory/searchresponse containsprojecteven though the request omits itIn the real
mem::searchimplementation, the response can includeprojectwhenAGENTMEMORY_PROJECT_NAMEis set, even if the client doesn’t sendproject(becauseprojectFilteris derived viarequireProjectScope(...)→resolveProjectScope(...)→ env fallback). Since this test stubs the fetch, thatprojectfield is only meaningful if you’re intentionally simulating that env-driven server behavior.Either document that intent in the mock (and/or set
process.env["AGENTMEMORY_PROJECT_NAME"]in this test for consistency), or removeprojectfrom the mocked/agentmemory/searchresponse and drop the corresponding assertion so the test stays focused on forwardingformat/token_budget.🤖 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/mcp-standalone-proxy.test.ts` around lines 79 - 119, The mocked /agentmemory/search response in the test "proxies memory_recall to POST /agentmemory/search and forwards format/token_budget (`#507`)" includes a project field that is only produced server-side when AGENTMEMORY_PROJECT_NAME is set, which makes the test inconsistent; either set process.env["AGENTMEMORY_PROJECT_NAME"] at the top of this test to mirror server behavior (so the mock + assertion on body.project stays valid) or remove the project property from the stubbed response and delete the corresponding assertion (the test should only assert forwarding of format/token_budget and other provided fields via handleToolCall and the calls array). Ensure changes are applied in this test block (function/test name: it(... "proxies memory_recall ...")) and update or remove the expect(body.project) and related mock project value accordingly.
🤖 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 @.env.example:
- Line 110: Add documentation for the AGENTMEMORY_PROJECT_NAME env var to
.env.example: mention that it provides a default project name used by the
project-scoping logic (fallback used in src/functions/project-scope.ts when no
explicit project is provided) and note it only applies when
AGENTMEMORY_PROJECT_ISOLATION is enabled; include an example value and a short
description of its purpose. Ensure the variable name is spelled exactly as
AGENTMEMORY_PROJECT_NAME and place the new line near the
AGENTMEMORY_PROJECT_ISOLATION entry so users discover the related setting.
In `@src/functions/consolidate.ts`:
- Around line 166-170: The current existingMatch selection uses
projectMatchesScope which returns true for undefined m.project when
scopedProject is falsy, allowing a scoped consolidation to pick an unscoped
memory; change the predicate to require strict project equality when
scopedProject is set: in the existingMemories.find call (look around the
existingMatch variable and the projectMatchesScope usage), keep the title
comparison but replace the project check with a conditional that, if
scopedProject is truthy, only matches when m.project === scopedProject,
otherwise allow any project (or undefined); this ensures we never mutate a
shared/unscoped row when consolidating into a specific project.
In `@src/functions/consolidation-pipeline.ts`:
- Around line 64-69: The current scoping uses projectMatchesScope() for mutable
collections (scopedSummaries, scopedSemantic and other places that later
update/write back existingSemantic/procedural sets), which can include
unscoped/global rows; change those filters to require strict project equality
(e.g., compare s.project === project or explicit null/undefined handling)
instead of projectMatchesScope(), so only rows actually owned by the project are
returned before any mutations are applied; update the same pattern where
procedural/semantic sets are filtered elsewhere in this file to avoid modifying
legacy global entries.
In `@src/functions/lessons.ts`:
- Around line 33-36: The lesson fingerprint currently only uses normalized
content (fingerprintId("lsn", data.content.trim().toLowerCase())), causing
cross-project key collisions; update all places that compute the lesson
fingerprint (the fingerprintId call that assigns fp and any subsequent uses
around the kv.get/kv.put logic) to include the project scope (e.g., include
project or project.id in the fingerprint input alongside the normalized content)
so each project's lessons generate distinct KV keys (ensure the same change is
applied wherever fingerprintId("lsn", ...) is used in this file, including the
blocks around lines that perform kv.get<Lesson>(KV.lessons, fp) and
kv.put(KV.lessons, fp, ...)).
In `@src/functions/project-scope.ts`:
- Around line 3-13: The function resolveProjectScope reads
process.env["AGENTMEMORY_PROJECT_NAME"] directly which bypasses the .env loader;
replace that direct access with the project's environment helper by calling
getEnvVar("AGENTMEMORY_PROJECT_NAME") (or the exported name of the env helper)
and use its returned value instead of envProject, preserving the existing
trim/empty-string checks and the function signature; update references in
resolveProjectScope so it prefers the explicit argument, then getEnvVar result,
then undefined.
In `@src/functions/remember.ts`:
- Around line 55-56: The mem::remember flow creates/supersedes project-scoped
memories but never records audits; update the remember handler (the code around
requireProjectScope and the mem::remember write paths, including the logic
covered by the later block at lines ~118-159) to call recordAudit() for every
state-changing event: add a recordAudit call after creating the new memory id
(include action like "memory.create" with the new memory id, project id, and
relevant metadata) and another recordAudit when a previous memory is
superseded/deleted (action like "memory.supersede" with the old memory id and
the new memory id), ensuring payloads include project and user context; follow
existing recordAudit signature and error handling conventions when adding these
calls.
In `@src/functions/search.ts`:
- Around line 348-349: The session-backed branch uses a strict equality check to
decide scope, causing inconsistent filtering between legacy unscoped hits and
KV.sessions-backed hits; update the session-backed hit filtering to use the same
projectMatchesScope logic used by the fallback memory branch. Specifically,
replace the strict equality check that inspects hit.project (in the branch that
calls loadSession() and queries KV.sessions) with a call to
projectMatchesScope(projectFilter, hit.project) (or the equivalent predicate
used for the fallback branch), ensuring both the loadSession()/KV.sessions path
and the memory fallback path use projectMatchesScope to decide whether to keep
or drop a hit.
In `@src/functions/smart-search.ts`:
- Around line 397-407: The current logic trusts a missing session row and
proceeds to read KV.observations(sessionIdHint), allowing observations to bypass
project scoping; update the check in the block that reads KV.sessions for
sessionIdHint so that when project is provided you treat a missing hintedSession
as a mismatch and return null (i.e., after awaiting kv.get for KV.sessions, if
hintedSession is falsy OR hintedSession.project !== project then return null)
before attempting to read KV.observations(sessionIdHint) — this ensures
expandIds cannot return an unscoped observation when the session metadata is
missing.
- Around line 187-193: Import the Session type from ../types.js and use it in
the new sessionCache/loadSession code path so TypeScript can type-check Session;
then tighten findObservation so it never reads KV.observations(sessionIdHint)
when KV.sessions returns no session: modify findObservation to call
loadSession(sessionIdHint) and only proceed to query
KV.observations(sessionIdHint) if the loaded Session is non-null and its project
matches the provided project (otherwise return null or continue other lookup
paths), referencing the symbols Session, sessionCache, loadSession,
findObservation, KV.sessions, KV.observations, and sessionIdHint.
In `@src/mcp/tools-registry.ts`:
- Around line 27-30: Add the missing "project" input schema to the MCP tool
entries for memory_file_history and memory_sessions in tools-registry.ts so they
match the project-scoped handlers enforced in server.ts; specifically, update
the inputSchema objects for the memory_file_history and memory_sessions tool
registrations to include project: { type: "string", description: "Optional
project identifier used to scope recall" } using the same symbol names
(memory_file_history, memory_sessions) so schema-driven MCP clients can supply
project and avoid 400 errors.
In `@src/triggers/api.ts`:
- Around line 879-884: The handler currently forwards the entire req.body into
sdk.trigger (e.g., the block using requireProjectParam(...) and calling
sdk.trigger({ function_id: "mem::file-context", payload: { ...req.body, project
} })), which violates the allowlist rule; replace the spread of req.body with an
explicit, validated payload object that picks only allowed fields (e.g.,
destructure and validate expected keys from req.body, or build payload = {
project, fileId, contextOptions } etc.), use requireProjectParam result for
project, validate/normalize each field, and pass that explicit payload to
sdk.trigger; apply the same change to the other endpoints that call sdk.trigger
with raw req.body (the blocks around lines referencing function_ids like
"mem::file-context" and the other three similar handlers).
---
Outside diff comments:
In `@src/functions/enrich.ts`:
- Around line 28-40: The mem::file-context trigger is missing the project field
in its payload, so when isolation is enabled the call fails; update the
sdk.trigger call that creates fileContextPromise to include the project (use the
project returned by requireProjectScope) in the payload object passed to
function_id "mem::file-context" so the invoked function receives the same
project scope; specifically modify the fileContextPromise payload in enrich.ts
to add project: project (or data.project if that pattern is used) and keep the
existing catch behavior.
In `@src/functions/flow-compress.ts`:
- Around line 30-57: The code currently accepts a resolved project via
requireProjectScope but still loads actions from a foreign runId or actionIds;
ensure we only compress actions belonging to that resolved project: when
handling a runId (RoutineRun), verify the run's project equals the resolved
project and return an error if not, or alternatively filter its actionIds by
fetching each Action (KV.actions) and only include actions whose action.project
=== project; when handling explicit actionIds, fetch each Action and only push
those where action.project === project (or return an error for any mismatched id
based on desired strictness). Update the run branch (where run.actionIds are
iterated) and the actionIds branch (where data.actionIds are iterated) to
perform this project check before adding to actionsToCompress, referencing
requireProjectScope, KV.routineRuns, KV.actions, RoutineRun, and Action.
In `@src/functions/reflect.ts`:
- Around line 281-287: The fingerprinting is currently based on content only,
causing cross-project collisions; update the fingerprint key generation to
include the project scope (e.g., incorporate projectId or project slug) when
calling fingerprintId so each project gets its own insight record. Concretely,
change the fp computation used with kv.get/kv.set for KV.insights (the lines
around fingerprintId("ins", content.trim().toLowerCase())) to include the
project identifier (for example fingerprintId("ins",
`${projectId}:${content.trim().toLowerCase()}`) or pass both projectId and
content if fingerprintId supports multiple args), and make the same change for
the other occurrence referenced (near reinforceInsight and the block at 300-306)
so lookups and writes are consistently namespaced per project.
In `@test/mcp-standalone-proxy.test.ts`:
- Around line 21-25: The beforeEach hook in test/mcp-standalone-proxy.test.ts
currently calls resetHandleForTests() and cleans AGENTMEMORY_SECRET but does not
clear AGENTMEMORY_PROJECT_NAME, which can cause cross-test pollution; update the
beforeEach to also remove process.env["AGENTMEMORY_PROJECT_NAME"] (similar to
the existing delete process.env["AGENTMEMORY_SECRET"]) so tests that set
AGENTMEMORY_PROJECT_NAME (e.g., around line 121) cannot leak its value into
later tests.
---
Nitpick comments:
In `@test/mcp-standalone-proxy.test.ts`:
- Around line 79-119: The mocked /agentmemory/search response in the test
"proxies memory_recall to POST /agentmemory/search and forwards
format/token_budget (`#507`)" includes a project field that is only produced
server-side when AGENTMEMORY_PROJECT_NAME is set, which makes the test
inconsistent; either set process.env["AGENTMEMORY_PROJECT_NAME"] at the top of
this test to mirror server behavior (so the mock + assertion on body.project
stays valid) or remove the project property from the stubbed response and delete
the corresponding assertion (the test should only assert forwarding of
format/token_budget and other provided fields via handleToolCall and the calls
array). Ensure changes are applied in this test block (function/test name:
it(... "proxies memory_recall ...")) and update or remove the
expect(body.project) and related mock project value accordingly.
🪄 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: 10d90049-6c2b-4b82-9fb9-29bc9d6e1fde
📒 Files selected for processing (41)
.env.exampleplugin/.mcp.copilot.jsonplugin/.mcp.jsonplugin/scripts/notification.mjsplugin/scripts/post-commit.mjsplugin/scripts/post-tool-failure.mjsplugin/scripts/post-tool-use.mjsplugin/scripts/pre-compact.mjsplugin/scripts/pre-tool-use.mjsplugin/scripts/prompt-submit.mjsplugin/scripts/session-end.mjsplugin/scripts/session-start.mjsplugin/scripts/stop.mjsplugin/scripts/subagent-start.mjsplugin/scripts/subagent-stop.mjsplugin/scripts/task-completed.mjssrc/cli/connect/util.tssrc/config.tssrc/functions/consolidate.tssrc/functions/consolidation-pipeline.tssrc/functions/enrich.tssrc/functions/file-index.tssrc/functions/flow-compress.tssrc/functions/lessons.tssrc/functions/patterns.tssrc/functions/project-scope.tssrc/functions/reflect.tssrc/functions/remember.tssrc/functions/search.tssrc/functions/smart-search.tssrc/mcp/server.tssrc/mcp/standalone.tssrc/mcp/tools-registry.tssrc/triggers/api.tssrc/types.tstest/consolidation-pipeline.test.tstest/cross-project-isolation.test.tstest/mcp-standalone-proxy.test.tstest/mcp-standalone.test.tstest/project-isolation.test.tstest/smart-search.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/triggers/api.ts (1)
888-899:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject malformed
api::file-contextbodies instead of downgrading them.
{},{ files: [42] }, or a blanksessionIdnow normalize intopayload = { files: [] }and still callmem::file-context. That leaves the boundary-validation gap open and turns invalid client input into ambiguous downstream behavior.Suggested fix
const body = req.body ?? {}; const sessionId = asNonEmptyString(body.sessionId); const files = Array.isArray(body.files) ? body.files .map((file) => (typeof file === "string" ? file.trim() : "")) .filter(Boolean) : []; + if (!sessionId || files.length === 0) { + return { + status_code: 400, + body: { + error: "sessionId (string) and files (string[]) are required", + }, + }; + } const payload: { sessionId?: string; files: string[]; project?: string } = { files, }; - if (sessionId) payload.sessionId = sessionId; + payload.sessionId = sessionId;As per coding guidelines, "REST endpoint registration must: validate and whitelist request body fields (never pass raw body to sdk.trigger)".
🤖 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 888 - 899, The handler currently normalizes invalid bodies (e.g., {} or { files: [42] } or blank sessionId) into payload = { files: [] } and still calls the mem::file-context trigger; change this to strictly validate and whitelist incoming fields: ensure body.files exists as an array of non-empty strings (map/filter as now but reject if any element is not a string) and ensure sessionId is a non-empty string (use asNonEmptyString and treat blank as invalid), and if validation fails return a 400 error instead of constructing payload; perform these checks before building payload and before calling sdk.trigger or mem::file-context to avoid passing downgraded/malformed input downstream.
🤖 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/triggers/api.ts`:
- Around line 1646-1658: Validate that body.tier (after trimming into the local
variable tier) is one of the allowed values
["all","semantic","reflect","procedural","decay"] and return a 400 with a clear
error if it is not; keep the existing non-empty-string check but replace it with
a whitelist check before assigning payload.tier so that invalid values (e.g.,
"foo") are rejected; update the error message to say "tier must be one of: all,
semantic, reflect, procedural, decay" and leave the handling of body.force and
project unchanged.
- Around line 886-899: The handler currently passes req.body?.project directly
into requireProjectParam which lets non-string values (e.g. numbers) be treated
as missing by resolveProjectScope; validate the project input at the HTTP
boundary by checking if req.body.project is present and is a string before
calling requireProjectParam, returning a 400 error for any present non-string
value; update the same validation in all other handlers that call
requireProjectParam (the other instances noted) so requireProjectParam only
receives string|undefined and resolveProjectScope cannot silently fallback.
In `@test/lessons.test.ts`:
- Around line 234-236: The test "includes legacy unscoped lessons in project
filters when isolation is off" relies on AGENTMEMORY_PROJECT_ISOLATION being
disabled; pin that environment variable at the start of the test (e.g., set
process.env.AGENTMEMORY_PROJECT_ISOLATION = "false" or delete it) before calling
sdk.trigger("mem::lesson-list", ...) and restore the previous value in an
after/finally block so other tests aren't affected; update the test to capture
the original process.env.AGENTMEMORY_PROJECT_ISOLATION, set it explicitly for
this case, run the assertion on result.lessons, then restore the original value.
---
Duplicate comments:
In `@src/triggers/api.ts`:
- Around line 888-899: The handler currently normalizes invalid bodies (e.g., {}
or { files: [42] } or blank sessionId) into payload = { files: [] } and still
calls the mem::file-context trigger; change this to strictly validate and
whitelist incoming fields: ensure body.files exists as an array of non-empty
strings (map/filter as now but reject if any element is not a string) and ensure
sessionId is a non-empty string (use asNonEmptyString and treat blank as
invalid), and if validation fails return a 400 error instead of constructing
payload; perform these checks before building payload and before calling
sdk.trigger or mem::file-context to avoid passing downgraded/malformed input
downstream.
🪄 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: 6a21e0dd-fff2-45c9-9bd9-fa0a4b936c0c
📒 Files selected for processing (15)
.env.examplesrc/functions/consolidate.tssrc/functions/consolidation-pipeline.tssrc/functions/lessons.tssrc/functions/project-scope.tssrc/functions/search.tssrc/functions/smart-search.tssrc/mcp/tools-registry.tssrc/triggers/api.tstest/cli-connect.test.tstest/consolidation-pipeline.test.tstest/cross-project-isolation.test.tstest/lessons.test.tstest/mcp-standalone.test.tstest/smart-search.test.ts
✅ Files skipped from review due to trivial changes (2)
- .env.example
- test/mcp-standalone.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- test/cross-project-isolation.test.ts
- test/consolidation-pipeline.test.ts
- src/mcp/tools-registry.ts
- src/functions/project-scope.ts
- src/functions/lessons.ts
- src/functions/search.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/triggers/api.ts (1)
140-145:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon’t silently floor fractional timeline offsets.
parseOptionalNonNegativeInt(0.9)becomes0and1.9becomes1, so/agentmemory/timelinerewrites malformed input instead of rejecting it. That changes the requested window and weakens the boundary validation this PR is tightening.Suggested fix
function parseOptionalNonNegativeInt(value: unknown): number | undefined | null { const parsed = parseOptionalFiniteNumber(value); if (parsed === undefined || parsed === null) return parsed; - const floored = Math.floor(parsed); - return floored >= 0 ? floored : null; + if (!Number.isInteger(parsed) || parsed < 0) return null; + return parsed; }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 140 - 145, The parseOptionalNonNegativeInt function currently floors fractional inputs, which silently rewrites malformed offsets; change it to reject non-integer values instead: after calling parseOptionalFiniteNumber(value) (in parseOptionalNonNegativeInt), if the result is undefined/null return it, then check Number.isInteger(parsed) and that parsed >= 0—return parsed when both true, otherwise return null; this preserves boundary validation and avoids silently altering fractional inputs.
🤖 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/triggers/api.ts`:
- Around line 151-156: The current validation only rejects non-strings but
allows whitespace-only strings to reach resolveProjectScope; update the input
check in the handler so that after confirming value is a string you also reject
strings that are empty after trimming (e.g., if (typeof value === "string" &&
value.trim() === "") return { error: `project must be a string for ${routeName}`
};), returning the same 400-style error before calling
resolveProjectScope(value) so whitespace-only input does not fall through and
pick up AGENTMEMORY_PROJECT_NAME.
---
Outside diff comments:
In `@src/triggers/api.ts`:
- Around line 140-145: The parseOptionalNonNegativeInt function currently floors
fractional inputs, which silently rewrites malformed offsets; change it to
reject non-integer values instead: after calling
parseOptionalFiniteNumber(value) (in parseOptionalNonNegativeInt), if the result
is undefined/null return it, then check Number.isInteger(parsed) and that parsed
>= 0—return parsed when both true, otherwise return null; this preserves
boundary validation and avoids silently altering fractional inputs.
🪄 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: 86deae31-9a77-484a-8007-b2461838953d
📒 Files selected for processing (3)
src/triggers/api.tstest/api-project-validation.test.tstest/lessons.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/lessons.test.ts
Comment on lines
+151
to
+156
| if (value !== undefined && typeof value !== "string") { | ||
| return { | ||
| error: `project must be a string for ${routeName}`, | ||
| }; | ||
| } | ||
| const project = resolveProjectScope(value); |
Contributor
There was a problem hiding this comment.
Reject whitespace-only project values before resolving scope.
This only blocks non-strings. A body/query value like " " still falls through to resolveProjectScope() and can pick up AGENTMEMORY_PROJECT_NAME, so routes using this helper may read or write the default project instead of returning 400 for invalid client input.
Suggested fix
function requireProjectParam(
value: unknown,
routeName: string,
): { project?: string; error?: string } {
if (value !== undefined && typeof value !== "string") {
return {
error: `project must be a string for ${routeName}`,
};
}
+ if (typeof value === "string" && value.trim().length === 0) {
+ return {
+ error: `project must be a non-empty string for ${routeName}`,
+ };
+ }
const project = resolveProjectScope(value);
if (isProjectIsolationEnabled() && !project) {
return {
error: `project is required for ${routeName} when AGENTMEMORY_PROJECT_ISOLATION=true`,
};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 151 - 156, The current validation only
rejects non-strings but allows whitespace-only strings to reach
resolveProjectScope; update the input check in the handler so that after
confirming value is a string you also reject strings that are empty after
trimming (e.g., if (typeof value === "string" && value.trim() === "") return {
error: `project must be a string for ${routeName}` };), returning the same
400-style error before calling resolveProjectScope(value) so whitespace-only
input does not fall through and pick up AGENTMEMORY_PROJECT_NAME.
- Updated the default behavior for AGENTMEMORY_PROJECT_ISOLATION to be enabled, allowing for stricter project scoping. - Modified the TOML configuration to include additional environment variables related to project isolation. - Enhanced the handling of project isolation in various functions, ensuring proper error messages and behavior when isolation is enabled or disabled. - Updated tests to reflect changes in project isolation handling, ensuring backward compatibility and proper functionality. - Improved documentation and descriptions for project-related fields in the tools registry.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/mcp/tools-registry.ts (1)
205-221:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSchema inconsistency: memory_profile marks project as required but description suggests conditional requirement.
The
memory_profiletool includesprojectin therequiredarray (line 220), making it mandatory at the schema level. However, the description (lines 212-213) states "Required when AGENTMEMORY_PROJECT_ISOLATION is enabled (default)", which matches the conditional-requirement pattern used by all other tools in this file (memory_recall, memory_save, memory_smart_search, etc.).For consistency with the rest of the codebase, either:
- Remove
projectfrom therequiredarray and rely on runtime validation viarequireProjectArg()(like other tools), OR- Update the description to state "Project path (required)" if memory_profile genuinely cannot function without a project regardless of the isolation flag.
Based on the PR's isolation-scoping architecture and the uniform handling across other tools, option 1 is likely correct.
Suggested alignment with other tools
inputSchema: { type: "object", properties: { project: { type: "string", description: "Project path. Required when AGENTMEMORY_PROJECT_ISOLATION is enabled (default); use the repo root folder name.", }, refresh: { type: "string", description: "Set to 'true' to force rebuild", }, }, - required: ["project"], },🤖 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/mcp/tools-registry.ts` around lines 205 - 221, The memory_profile tool's JSON schema marks "project" as required but its description indicates it's conditionally required; make it consistent with other tools by removing "project" from the inputSchema.required array of the memory_profile tool (the memory_profile object), and rely on runtime validation via requireProjectArg() where the tool is invoked; ensure the description remains the conditional one ("Required when AGENTMEMORY_PROJECT_ISOLATION is enabled") so behavior and messaging match other tools like memory_recall/memory_save.
🤖 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/state/hybrid-search.ts`:
- Around line 291-314: The code references the Session type (used in loadSession
and resolveProject) but it isn't imported, causing a TS error; update the module
imports at the top of the file to include the Session type (the same module that
exports Memory, KV, etc.) so Session is available for the sessionCache,
loadSession, and resolveProject signatures; ensure the import uses the correct
named export (Session) consistent with how Memory and other types are imported.
---
Outside diff comments:
In `@src/mcp/tools-registry.ts`:
- Around line 205-221: The memory_profile tool's JSON schema marks "project" as
required but its description indicates it's conditionally required; make it
consistent with other tools by removing "project" from the inputSchema.required
array of the memory_profile tool (the memory_profile object), and rely on
runtime validation via requireProjectArg() where the tool is invoked; ensure the
description remains the conditional one ("Required when
AGENTMEMORY_PROJECT_ISOLATION is enabled") so behavior and messaging match other
tools like memory_recall/memory_save.
🪄 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: d774cb20-d0ef-4f7e-8cd8-56c0975772bf
📒 Files selected for processing (29)
README.mdplugin/.mcp.copilot.jsonplugin/.mcp.jsonsrc/cli/connect/codex.tssrc/cli/connect/util.tssrc/config.tssrc/functions/project-scope.tssrc/functions/search.tssrc/functions/smart-search.tssrc/mcp/rest-proxy.tssrc/mcp/server.tssrc/mcp/standalone.tssrc/mcp/tools-registry.tssrc/state/hybrid-search.tssrc/triggers/api.tssrc/types.tstest/agent-id-scope.test.tstest/cli-connect.test.tstest/connect-new-agents.test.tstest/enrich-project-isolation.test.tstest/enrich.test.tstest/lessons.test.tstest/mcp-standalone-proxy.test.tstest/mcp-standalone.test.tstest/project-isolation.test.tstest/remember-project-scope.test.tstest/search.test.tstest/smart-search.test.tstest/vector-index-populate.test.ts
✅ Files skipped from review due to trivial changes (3)
- src/cli/connect/codex.ts
- README.md
- plugin/.mcp.copilot.json
🚧 Files skipped from review as they are similar to previous changes (11)
- plugin/.mcp.json
- src/config.ts
- src/functions/project-scope.ts
- src/cli/connect/util.ts
- test/smart-search.test.ts
- src/mcp/standalone.ts
- test/project-isolation.test.ts
- src/functions/search.ts
- src/mcp/server.ts
- src/functions/smart-search.ts
- src/triggers/api.ts
Comment on lines
+291
to
+314
| const sessionCache = new Map<string, Session | null>(); | ||
| const memoryProjectCache = new Map<string, string | null>(); | ||
| const loadSession = async (sessionId: string): Promise<Session | null> => { | ||
| if (sessionCache.has(sessionId)) return sessionCache.get(sessionId)!; | ||
| const session = await this.kv.get<Session>(KV.sessions, sessionId).catch(() => null); | ||
| sessionCache.set(sessionId, session); | ||
| return session; | ||
| }; | ||
| const loadMemoryProject = async (obsId: string): Promise<string | null> => { | ||
| if (memoryProjectCache.has(obsId)) return memoryProjectCache.get(obsId)!; | ||
| const mem = await this.kv.get<Memory>(KV.memories, obsId).catch(() => null); | ||
| const project = | ||
| typeof mem?.project === "string" && mem.project.trim().length > 0 | ||
| ? mem.project.trim() | ||
| : null; | ||
| memoryProjectCache.set(obsId, project); | ||
| return project; | ||
| }; | ||
| const resolveProject = async (obsId: string, sessionId: string): Promise<string | undefined> => { | ||
| const session = await loadSession(sessionId); | ||
| if (session) return session.project; | ||
| const memProject = await loadMemoryProject(obsId); | ||
| return memProject ?? undefined; | ||
| }; |
Contributor
There was a problem hiding this comment.
Missing import for Session type.
Lines 291 and 295 reference the Session type, but it is not included in the import statement (lines 3-9). This will cause a TypeScript compilation error.
📦 Proposed fix to add the missing import
import type {
EmbeddingProvider,
HybridSearchResult,
CompressedObservation,
Memory,
QueryExpansion,
+ Session,
} from "../types.js";🤖 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/state/hybrid-search.ts` around lines 291 - 314, The code references the
Session type (used in loadSession and resolveProject) but it isn't imported,
causing a TS error; update the module imports at the top of the file to include
the Session type (the same module that exports Memory, KV, etc.) so Session is
available for the sessionCache, loadSession, and resolveProject signatures;
ensure the import uses the correct named export (Session) consistent with how
Memory and other types are imported.
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.
What changed
AGENTMEMORY_PROJECT_ISOLATIONto require project scoping for memory reads and writes when enabled.Why
Behavior
AGENTMEMORY_PROJECT_ISOLATION=true, memory operations require a project scope.Verification
npm run buildnpx vitest run test/project-isolation.test.ts test/smart-search.test.ts test/mcp-standalone-proxy.test.tsmemory_save,memory_recall, andmemory_smart_searchSummary by CodeRabbit
New Features
Bug Fixes / Validation
Documentation
Tests