-
Notifications
You must be signed in to change notification settings - Fork 0
feat: memory retrieval quality sprint — metadata strip, trivial augmentation, extraction window, preamble #2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
3d22b2b
d6452ca
cc544fa
3184d96
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,5 +2,3 @@ node_modules/ | |
| *.tsbuildinfo | ||
| .env | ||
| .DS_Store | ||
| src/ | ||
| tsconfig.json | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -28,6 +28,9 @@ import type { OpenClawPluginApi } from "openclaw/plugin-sdk"; | |||||
| import { mkdirSync, existsSync } from "node:fs"; | ||||||
| import { dirname, join } from "node:path"; | ||||||
|
|
||||||
| // Module-level flag: emit hardcoded-key warning only once per process lifetime | ||||||
| let _apiKeyWarningEmitted = false; | ||||||
|
|
||||||
| // Dynamic require for node:sqlite (available in Node 22+, avoids TS import issues) | ||||||
| let NodeDatabaseSync: any; | ||||||
| try { | ||||||
|
|
@@ -87,6 +90,10 @@ function resolveEnv(value: string): string { | |||||
| return value.replace(/\$\{([^}]+)\}/g, (_, key) => process.env[key] ?? ""); | ||||||
| } | ||||||
|
|
||||||
| function isEnvInterpolation(value: unknown): value is string { | ||||||
| return typeof value === "string" && /^\$\{[^}]+\}$/.test(value.trim()); | ||||||
| } | ||||||
|
|
||||||
| function parseConfig(raw: unknown): EvaMemoryConfig { | ||||||
| const defaults: EvaMemoryConfig = { | ||||||
| cortexUrl: "http://localhost:8000", | ||||||
|
|
@@ -98,7 +105,7 @@ function parseConfig(raw: unknown): EvaMemoryConfig { | |||||
| retrievalBudget: 2000, | ||||||
| maxInjectionChars: 8000, | ||||||
| maxInjectedMemories: 8, | ||||||
| minRelevanceScore: 0.25, | ||||||
| minRelevanceScore: 0.30, | ||||||
| retrievalMode: "fast", | ||||||
| recencyFilterMinutes: 15, | ||||||
| injectCornerstones: false, | ||||||
|
|
@@ -780,10 +787,29 @@ async function syncMemoryCache( | |||||
| } | ||||||
| } | ||||||
|
|
||||||
| // --- Inbound metadata stripper --- | ||||||
|
|
||||||
| function stripInboundMetadata(prompt: string): string { | ||||||
| let text = prompt; | ||||||
| // Strip Sender/Conversation info metadata blocks | ||||||
| text = text.replace(/(?:Sender|Conversation info) \(untrusted metadata\):\n```json\n[\s\S]*?```\n?/g, ""); | ||||||
| // Strip only OpenClaw-decorated system lines (not bare "System:" which could be user content) | ||||||
| text = text.replace(/^System \(untrusted\):.*$/gm, ""); | ||||||
| // Strip [Thread history ...] block: header line + indented/prefixed lines until blank line | ||||||
| text = text.replace(/^\[Thread history[^\n]*\n(?:(?: |\w+:)[^\n]*\n)*\n?/gm, ""); | ||||||
| // Strip timestamp prefixes at start of text like [Mon 2026-04-02 14:51 GMT+7] | ||||||
| text = text.replace(/^\[[A-Za-z]{3}\s+\d{4}-\d{2}-\d{2}\s+\d{2}:\d{2}[^\]]*\]\s*/gm, ""); | ||||||
| // Strip <relevant-memories>...</relevant-memories> blocks | ||||||
| text = text.replace(/<relevant-memories>[\s\S]*?<\/relevant-memories>/g, ""); | ||||||
| // Collapse multiple newlines to single | ||||||
| text = text.replace(/\n{2,}/g, "\n"); | ||||||
| return text.trim(); | ||||||
| } | ||||||
|
|
||||||
| // --- Memory-relevance heuristic --- | ||||||
|
|
||||||
| const MEMORY_KEYWORDS = /\b(remember|forgot|recall|last time|previously|before|earlier|you said|you told|we discussed|we decided|my preference|my name|who am i|what do i|do you know|history|past|memory|memorize|don'?t forget)\b/i; | ||||||
| const TRIVIAL_PATTERNS = /^(hi|hello|hey|thanks|ok|yes|no|sure|bye|good morning|good night|👍|😊)[\s!?.]*$/i; | ||||||
| const TRIVIAL_PATTERNS = /^(hi|hello|hey|thanks|thank you|ok|okay|yes|no|sure|bye|good morning|good night|go ahead|sounds good|do it|got it|lets do it|let's do it|lets go|let's go|cool|nice|great|agreed|done|will do|on it|yep|nope|nah|alright|perfect|roger|absolutely|definitely|for sure|ship it|merge it|deploy it|👍|😊|👌|🙏|✅)[\s!?.]*$/i; | ||||||
|
|
||||||
| // Sub-agent completion events and runtime system events should not trigger memory retrieval | ||||||
| const SYSTEM_EVENT_PATTERNS = /\[Internal task completion event\]|<<<BEGIN_UNTRUSTED_CHILD_RESULT>>>|source: subagent|type: subagent task|^\[.*\] Exec (?:completed|failed)|^\[.*\] OpenClaw runtime context/m; | ||||||
|
|
@@ -897,7 +923,24 @@ function filterEchoMemories( | |||||
| }); | ||||||
| } | ||||||
|
|
||||||
| function formatMemoryContext(items: RetrievedItem[], maxChars: number, maxCount = 8, minScore = 0.25): string { | ||||||
| const MEMORY_PREAMBLE = `These are long-term memories retrieved from your memory system based on this conversation. | ||||||
| Each has an [item_id], [date], and [salience/category] label. | ||||||
|
|
||||||
| How to use these memories: | ||||||
| - identity/preferences/decisions: Trust as stable context. Reference when relevant. | ||||||
| - episodic: Specific past events. Check the date — may be outdated. | ||||||
| - goals: Strategic direction. Verify if context has changed since the date. | ||||||
| - behavioral/relational: Patterns and dynamics. Descriptive, not prescriptive. | ||||||
| - high salience: Identity-defining or emotionally significant. Handle with care. | ||||||
|
|
||||||
| Guidelines: | ||||||
| - Surface relevant preferences or prior decisions naturally in your response. | ||||||
| - If a memory might change your approach, ask a clarifying question before proceeding. | ||||||
| - If memories contradict the current request, flag the contradiction. | ||||||
| - Never expose raw item_ids, scores, or metadata to the user. | ||||||
| - To look up more context on any memory, use cortex_search with the item_id.`; | ||||||
|
||||||
| - To look up more context on any memory, use cortex_search with the item_id.`; | |
| - To look up more context on any memory, use cortex_search with a semantic query based on its content, date, salience, or category; do not rely on item_id.`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 Info: MEMORY_PREAMBLE consumes ~1211 of 8000 char budget
The MEMORY_PREAMBLE is 1,211 characters long, consuming roughly 15% of the default maxInjectionChars budget (8000). The charCount in formatMemoryContext at src/index.ts:958 is initialized to MEMORY_PREAMBLE.length, and the overhead of the <relevant-memories> tag (~20 chars), newlines between elements, the optional footer line, and the closing tag (~21 chars) are not included in charCount. This means the actual injected text slightly exceeds maxInjectionChars by roughly 60-100 characters. With the default 8000 budget this is negligible (<2%), but users who set a tight maxInjectionChars value close to the preamble length would get no memories injected without understanding why.
Was this helpful? React with 👍 or 👎 to provide feedback.
Copilot
AI
Apr 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The maxChars budget accounting starts at MEMORY_PREAMBLE.length and then adds line.length, but it does not include the <relevant-memories> header line or newline separators between lines. With the new ~1KB preamble this can overshoot the intended maxInjectionChars budget by a non-trivial amount. Consider including the header + newline characters in charCount (or compute the final string length before returning).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 Info: extractMessages reduced from 10 to 5 messages affects capture context
The extractMessages function was changed from rawMessages.slice(-10) (last 10 messages of any type, filtered for user/assistant) to walking backwards for the last 5 user/assistant messages specifically. This is an intentional optimization for large sessions (comment references 30K+ messages / 70MB+), but it reduces the conversation context sent to the Cortex remember API. In conversations with many interleaved tool calls, the new code skips further back to find user/assistant messages (good), but the reduced count means less context for memory extraction. For most conversations this is fine, but for complex multi-step workflows the 5-message window might miss important earlier context that the 10-message window would have captured.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 Info: API key warning fix correctly addresses pre-existing bug
The old code at lines 1017-1018 (old) checked !cfg.apiKey.startsWith("${") on the resolved value. Since resolveEnv replaces ${CORTEX_API_KEY} with the actual env var value (e.g., sk-abc123), the old check would incorrectly warn even when the API key was properly configured via environment variable. The new code (src/index.ts:1070) correctly checks the raw config value using isEnvInterpolation(rawConfig?.apiKey), which tests whether the original config string was a ${...} interpolation before resolution. This is a correct fix. The old code also had a duplicate warning (lines 1017 and 1044 in old code); the new code consolidates into a single check with _apiKeyWarningEmitted deduplication.
Was this helpful? React with 👍 or 👎 to provide feedback.
Copilot
AI
Apr 2, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isRelevant is assigned using cleanedPrompt && isMemoryRelevant(cleanedPrompt), which yields "" | boolean rather than a boolean. While it works in the current else if (isRelevant) check, it’s easy to trip over later (e.g., if passed somewhere that expects boolean). Consider making this explicitly boolean (e.g., const isRelevant = cleanedPrompt.length > 0 && isMemoryRelevant(cleanedPrompt)).
| const isRelevant = cleanedPrompt && isMemoryRelevant(cleanedPrompt); | |
| const isRelevant = cleanedPrompt.length > 0 && isMemoryRelevant(cleanedPrompt); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 Info: Question mark gate is very broad — URLs and code in prompts trigger full retrieval
The hasQuestion gate at src/index.ts:1495 uses cleanedPrompt.includes('?') which matches any ? character, including those inside URLs (e.g. https://example.com?key=val), code snippets, or escaped characters. Since hasQuestion takes priority over the trivial and relevance gates, prompts containing URLs with query strings would always trigger full memory retrieval with the maximum memory count. This is unlikely to cause harm (extra retrieval is just a small latency cost), but it's worth noting as a heuristic gap.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 Info: lastAssistantTurnCache only populated when autoCapture=true
The lastAssistantTurnCache (line 1459) is read in the before_agent_start handler (line 1494, inside the autoRecall block) but only populated in the agent_end handler (line 1618-1628), which is registered only when cfg.autoCapture is true (src/index.ts:1606). If a user configures autoRecall=true but autoCapture=false, the trivial-query augmentation feature silently degrades — trivial prompts will never find a cached assistant turn, so retrieval is skipped. This isn't a bug (the system degrades gracefully), but the coupling between these two independent config flags is implicit and could surprise operators.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚩 Server HTTP error (non-200) silently skips local cache fallback
When client.retrieve() gets a non-200 HTTP response (e.g. 500), the post() method at src/index.ts:178 returns null — the Promise resolves to null rather than rejecting. In the Promise.allSettled result at src/index.ts:1542, this means retrieveResult.status === "fulfilled" with value === null, so retrieveResult.value?.items?.length is falsy and the items branch is skipped. The else if (retrieveResult.status === "rejected") branch at src/index.ts:1550 also doesn't trigger because the status is "fulfilled". The net effect: a server 500 error silently produces zero memories with no fallback to local cache and no warning logged. Only true network errors (fetch rejection, abort signal) trigger the fallback path. This is a pre-existing issue not introduced by this PR, but the new three-gate retrieval logic routes more queries through this path (e.g. trivial-augmented queries), making it slightly more impactful.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 Info: lastAssistantTurnCache eviction is FIFO not LRU — oldest inserted session evicted even if recently active
The eviction logic at src/index.ts:1624-1627 deletes the first key from the Map when size exceeds 100. JavaScript Maps iterate in insertion order, and Map.set() on an existing key updates the value but does NOT change its insertion order. So if session A was inserted first and is still very active (being .set() on every turn), it remains in its original insertion position and would be evicted first once the cache reaches 101 entries, despite being the most recently updated. For true LRU behavior, the code would need to delete then set on each update to move the key to the end. With 100 slots this is unlikely to cause visible problems, but it's worth noting if the cache size is ever reduced.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 Info: Expanded TRIVIAL_PATTERNS includes actionable commands like 'deploy it' and 'ship it'
The
TRIVIAL_PATTERNSregex atsrc/index.ts:812was expanded to include actionable phrases like "do it", "ship it", "merge it", "deploy it". These patterns are used in bothisMemoryRelevant(skips retrieval) and the newisTrivialbranch (attempts augmentation with last assistant turn). In the old code, these short prompts without memory keywords would also skip retrieval (via theprompt.length < 40check inisMemoryRelevant). The new code is marginally better: it at least attempts to augment with the last assistant turn for context. The key observation is that these actionable commands are typically confirmations of something the assistant proposed, so the assistant turn context is the most relevant signal for retrieval.Was this helpful? React with 👍 or 👎 to provide feedback.