Repository navigation
Conversation
40baa3b to
a347647
Compare
🤖 Open Code ReviewTarget: PR #2562 🔍 OpenCodeReview found 7 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. |
|
Description
Automatic 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.Add
algorithm.retrieval.recallScope:session(new default) — only traces written by the calling Session may be injected.profile— previous behaviour; every Session sharing this home andprofileIdrecalls every other Session's transcript.Both retrieval entry points are scoped through one shared helper,
resolveRecallSessionKey():orchestrator.retrievalDepsFor()for turn-start / skill / sub-agent / repair recall, andmemory-core.searchMemory()for tool-driven recall. The resolved pin is pushed into SQL so foreign rows cannot consume the candidate budget, and is also exposed asRetrievalConfig.sessionKeyso tier-2 filter builders suppress contradictory same-session exclusions. A missing Session id now logsrecall.scope_unscopedinstead of silently widening to profile-wide recall.policies/world_modelstay profile-wide: they are statement-shaped rather than transcript-shaped, so cross-session sharing there is the intended design.getManyByIdsremains a pure id lookup so L2/L3 evidence citations cannot lose their support rows. A Viewer toggle exposes the same key under Settings → General.This changes the default and is therefore breaking. Users who rely on cross-session recall must set
algorithm.retrieval.recallScope: profile. The previous behaviour silently corrupts agent identity, which is worse than losing recall breadth, but the choice belongs in release notes. All changes are underapps/memos-local-plugin/; production dependencies are unchanged.A new Session now starts with no recall until it has written its first trace. That is the intended trade-off, not an oversight. When
algorithm.lightweightMemoryis disabled, the L2/L3 path remains the cross-session channel.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).Review feedback addressed
Two rounds of Open Code Review. Both rounds found real defects; the fixes are cumulative in this revision.
Round 1 (
a347647→53764d2) — the original revision scoped only the tool-driven path:turn_startrecall bypassed the scopeddepsviaorchestrator.retrievalDepsFor()retrievalDepsFor(namespace, sessionKey)now takes the key; all six retrieval entry points thread it throughrecallScopeatalgorithmtop level created two sources of truthalgorithm.retrieval.recallScopeisVisibleTo(row as never, …)erased the row typeParameters<typeof isVisibleTo>[0] & { sessionId?: string }searchByErrorSignaturedouble-filtered?? "session"fallbackRetrievalConfig.recallScopecommentRound 2 (
53764d2→e334e0c):RetrievalConfig.recallScopebreaks existing test fixturessessionKey?: stringadded as the runtime pin.session_id = X AND session_id != X— tier-2excludeSessionId/visibleContextpredicates contradict the pin, zeroing out trace recallbuildTraceSearchFilters()readsdeps.config.sessionKeyand drops same-session exclusions when pinned, keeping thets/user_textconstraintsresolveRecallSessionKey(recallScope, sessionId, onUnscoped)intoretrieval-repos.ts; both call sites use it and logrecall.scope_unscopedgetManyByIdssilently drops L2/L3 evidence rowspatch("algorithm")merges one level deep, overwriting siblingretrieval.*onPatchAlgorithm({ retrieval: { ...algorithm.retrieval, recallScope } })New tests for round 2:
recall-scope-tier2.test.ts(3 cases) verifies that a pinned Session emits nosession_id != @exclude_session_id, that an unpinned one still does, and that the two predicates never co-occur.recall-scope.test.tswas updated to assertgetManyByIdsstays pure while search channels stay scoped.Type of change
recallScopechanges from profile-wide to session-scopedHow 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.pnpm lint(tsc -p tsconfig.json --noEmit) — passes, no output.npx vitest run tests/unit/retrieval tests/unit/pipeline— 27 files passed, 234 tests passed, 0 failed. This includes thetier1/2/3,rankerandintegrationfixtures called out in review, confirming the optional field restores type compatibility.Regression proofs (each verified by temporarily reverting the specific change and re-running):
sessionKeyentirelyrecall-scope.test.tsscopedSessionKey)recall-scope-turnstart.test.tsscoped = false)recall-scope-tier2.test.tsThe wrapper tests cover
searchByVector/ text / pattern / error-signature channels, the SQL predicate with a deliberately better-scored foreign trace,profilecompatibility, L2/L3 staying unconfined, and pure id hydration. The spy test covers orchestrator wiring; the tier-2 test covers filter cooperation.Full suite on this machine (first revision): 191 files, 13 failed / 177 passed / 1 skipped; 49 tests failed / 1592 passed. I did not triage every failure. Spot checks place them in two environment-only buckets —
tests/unit/config/paths.test.tsasserts POSIX separators and fails on Windows regardless of this change, andtests/unit/embedding/*requires network access to Hugging Face, which this sandbox does not have. Neither module is touched by this PR. A full-suite pass on Linux CI is still required before merge.A/B smoke on DSH
0.2.1-rc.5, same home andprofileId, two concurrent Sessions: before, Session08a39873recalled a trace owned byf03c9595. After loading the same change into a local build of2.0.21-beta.1and restarting DSH, Session2169d99erecalled only traces it owned. The "after" side is a local build, not a published artifact, and this smoke alone was not sufficient proof — the review correctly identified gaps that the new tests now cover.No live profile, credentials, or customer sessions were used for the smoke.
Checklist
Reviewer Checklist
apps/memos-local-plugin/; default-value change called out explicitly