Repository navigation
Conversation
Memtensor-AI
added
area:plugin
40baa3b to
a347647
Compare
🤖 Open Code ReviewTarget: PR #2562 🔍 OpenCodeReview found 6 issue(s) in this PR.
1.
|
|
a347647 to
53764d2
Compare
|
Thanks for the review — findings #1 and #7 were correct and critical, and they exposed a gap my own verification had missed. The first revision scoped only Changes in
The reason my five original tests did not catch this is that they exercised Current verification: Regarding the automated test result: the Thanks again for the thorough review; #1/#7 materially improved the fix. |
|
53764d2 to
e334e0c
Compare
|
Thanks for the second pass — finding #2 was the important one, and it identified a defect my first fix introduced rather than one it missed.
Changes in
Current verification: The repeated Thanks again; the second round caught a real regression in my own fix. |
|
Automatic turn-start recall selected every trace visible to the profile. A trace stores its transcript verbatim, so a second Session's assistant text reached the first Session's prompt with nothing marking it as foreign (MemOS#1532). Add algorithm.retrieval.recallScope (default session). Only the turn-start injection path is pinned: explicit searches, decision repair and sub-agent retrieval keep profile-wide recall, because they deliberately look across history. The pin is pushed into the SQL WHERE so foreign rows cannot consume the LIMIT budget, and tier-2 drops the legacy same-session exclusion when pinned so the two predicates cannot AND into an always-empty result. L2/L3 stay unconfined. Session key comes from the existing RuntimeNamespace.sessionKey; no new config or runtime channel is introduced. A missing key fails open to profile-wide recall.
e334e0c to
db11940
Compare
|
Thanks — finding #1 was correct and it invalidated the approach rather than a parameter. I verified it against the source: I did not patch that call site. I restructured the change in Scope narrowed to the turn-start injection path. The Session key no longer travels through a new channel. It is read from the existing #3 — #4 — #5 — parameter renamed to #7 — the synthetic-id hack is gone, so the two paths cannot disagree; there is only one path that pins now. On patch payloads (#2 in round three): I checked On tests: the previous suites were tautological, as you said — Verification, actually run: Two probes: disabling the SQL pin → 3 failures (the confinement cases); disabling tier-2 cooperation → 1 failure (the I also agree on the Given three rounds of automated findings, I would value a human review pass before merge — particularly on whether narrowing to the turn-start path is the right product decision. |
|
Description
Automatic turn-start recall selected every trace visible to the profile. A trace stores its transcript verbatim (
[user] … / [assistant] …), so with two Sessions open under oneprofileId, one Session's assistant text reached another Session's prompt with nothing distinguishing it from that Session's own history. Observed on DSH0.2.1-rc.5: Session08a39873recalled a trace owned by Sessionf03c9595, whose assistant field opened with a first-person self-description.Root cause:
core/runtime/namespace.ts::visibilityWhere()filters onowner_agent_kindandowner_profile_idonly; it has no session dimension. The storage layer already acceptsfilter.sessionId— retrieval never passed it down.What this changes
algorithm.retrieval.recallScope(defaultsession) is applied only to the turn-start automatic injection path.onTurnStart→recallTurn→retrieveTurnStart(automatic prompt injection)memos_searchdecision_repairskill_invoke,sub_agentpolicies, L3world_model, skillsPinning the explicit paths would be a functional regression, not a fix:
decision_repairexists to find historical anti-patterns across Sessions, andmemos_searchis a deliberate user request. This PR therefore narrows the scope to the injection path, which is the one that corrupts agent identity.Mechanics:
RuntimeNamespace.sessionKey(adapters/deepseek-harness/namespace.ts), which both adapters already populate. No new config channel, no new runtime field, no synthetic-id heuristics.WHERE(session_id = @vis_recall_session_key) rather than applied after Top-K, so foreign rows cannot consume the LIMIT budget.session_id != Xpredicate; otherwise the two AND into an always-empty result (session_id = X AND session_id != X). Thets/user_texthalves of those predicates are preserved.sessionKeyfails open to profile-wide recall — a Session id that does not exist must never confine recall to zero rows.getManyByIdskeeps itsisVisibleTonamespace guard and is not session-filtered: L2/L3 artifacts cite trace ids as evidence, and dropping a foreign row there would inject an unsupported citation.A Viewer toggle exposes the same key under Settings → General.
Scope and limitations
recallScopeon disk, so they inheritsessionand lose cross-session transcript recall from automatic injection. Setalgorithm.retrieval.recallScope: profileto opt back in. Documented indocs/CONFIG-ADVANCED.md.timeline,memos_get,listTracesand the Team Hub path do not go through the retrieval wrapper and are unaffected by this switch. They require an explicit id and do not inject into the prompt automatically. This PR fixes the injection path only; it does not claim to make stored memory unreadable across Sessions.apps/memos-local-plugin/; production dependencies are unchanged.Related Issue: #1532 (same failure mode; #1869 addressed it for the OpenClaw adapter, whose change set touches only
apps/memos-local-openclaw/), #2520 (DSH 0.2.x support; this does not change peer ranges or the Cloud plugin).Type of change
recallScopechanges from profile-wide to session-scoped for turn-start recallHow Has This Been Tested?
Environment: Windows 11 x64, Node 24.13.1, pnpm 10.10.0, DSH
0.2.1-rc.5, dependencies installed withpnpm install --ignore-scripts.The regression suite is
tests/unit/pipeline/recall-scope.test.ts(9 cases). It runs against a real temporary SQLite database through the realwrapRetrievalReposand, for two cases, throughcreatePipeline+createMemoryCore. It supersedes three earlier test files that asserted on mocked arguments and an empty database — those were tautological and are not part of this revision.Coverage:
k=1returns the pinned Session's row, proving the pin is in SQL and not a post-filter;scope=profileand a missingsessionKeyboth restore profile-wide recall;getManyByIdsstill rejects a row from another profile;onTurnStartinjection contains the calling Session and not the other;searchMemorystill returns both Sessions — the functional-regression guard;lightweightMemory: false(which suppliesexcludeSessionId), the pin is not cancelled by the legacy exclusion.Verification actually run:
Two independent regression probes confirm the tests bind to the implementation rather than to themselves:
excludeSessionIdcase (context comes back empty).Note:
pnpm lint'stsconfig.jsondoes not includetests/**and excludesviewer, so it does not cover the two areas this PR touches most. That is why the viewer typecheck is run separately, and why the new test file was type-checked on its own (tsconfigincluding onlytests/unit/pipeline/recall-scope.test.ts): 0 errors. The repository's other test files do report pre-existing type errors under that config; they are untouched by this PR.Full suite on this machine is not clean for environment reasons I did not triage exhaustively (
tests/unit/config/paths.test.tsasserts POSIX separators,tests/unit/embedding/*needs network). A Linux full-suite run is still required before merge.No live profile, credentials, or customer sessions were used.
Checklist
docs/CONFIG-ADVANCED.mdand the Viewer toggleReviewer Checklist
apps/memos-local-plugin/; the default-value change and the non-guarantee are both called out