fix(memory-wiki): skip bridge pruning when public artifacts list is empty#71764
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
openclaw-barnacle
Bot
added
docs
Greptile SummaryThis PR fixes a destructive pruning race condition in
Confidence Score: 3/5Safe to merge with caution — the guard prevents the reported data-loss bug, but introduces a secondary correctness gap when memory-core is loaded with zero artifacts. One P1 finding: the
Prompt To Fix All With AIThis is a comment left during a code review.
Path: extensions/memory-wiki/src/bridge.ts
Line: 254-262
Comment:
**Guard conflates "plugin not loaded" with "plugin loaded, no artifacts"**
`listActiveMemoryPublicArtifacts` returns `[]` both when `memoryPluginState.capability` is `undefined` (memory-core not registered) **and** when memory-core is registered but `listArtifacts` returns an empty list (user deleted all artifacts). The `publicArtifacts.length > 0` guard therefore skips pruning in both cases, leaving stale bridge entries in the wiki indefinitely whenever all artifacts are genuinely removed while memory-core is still loaded.
A safer discriminator would check whether the capability is registered at all, rather than relying on the artifact count as a proxy. For example, exporting `isMemoryCapabilityRegistered()` from `memory-state.ts` and threading it through the plugin-sdk interface would let the guard be `const shouldPrune = isMemoryCapabilityRegistered()` instead of `publicArtifacts.length > 0`.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: extensions/memory-core/src/dreaming.ts
Line: 724-727
Comment:
**Closure captures a local variable, not the resolved value**
`resolveStartupCron = () => cron` closes over the `let cron` local variable from this invocation of `reconcileManagedDreamingCron`. In JavaScript, closures capture the variable binding, so if `cron` were reassigned after this line within the same call, `resolveStartupCron()` would return the new value. While `cron` is not reassigned here after this point, an explicit capture is safer and makes the intent clearer:
```suggestion
resolveStartupCron = ((): (() => CronServiceLike | null) => {
const captured = cron;
return () => captured;
})();
```
This eliminates any ambiguity about whether the cached reference is the `cron` value at assignment time.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(memory-wiki): skip bridge pruning wh..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const shouldPrune = publicArtifacts.length > 0; | ||
| const removedCount = shouldPrune | ||
| ? await pruneImportedSourceEntries({ | ||
| vaultRoot: params.config.vault.path, | ||
| group: "bridge", | ||
| activeKeys, | ||
| state, | ||
| }) | ||
| : 0; |
There was a problem hiding this comment.
listActiveMemoryPublicArtifacts returns [] both when memoryPluginState.capability is undefined (memory-core not registered) and when memory-core is registered but listArtifacts returns an empty list (user deleted all artifacts). The publicArtifacts.length > 0 guard therefore skips pruning in both cases, leaving stale bridge entries in the wiki indefinitely whenever all artifacts are genuinely removed while memory-core is still loaded.
A safer discriminator would check whether the capability is registered at all, rather than relying on the artifact count as a proxy. For example, exporting isMemoryCapabilityRegistered() from memory-state.ts and threading it through the plugin-sdk interface would let the guard be const shouldPrune = isMemoryCapabilityRegistered() instead of publicArtifacts.length > 0.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/memory-wiki/src/bridge.ts
Line: 254-262
Comment:
**Guard conflates "plugin not loaded" with "plugin loaded, no artifacts"**
`listActiveMemoryPublicArtifacts` returns `[]` both when `memoryPluginState.capability` is `undefined` (memory-core not registered) **and** when memory-core is registered but `listArtifacts` returns an empty list (user deleted all artifacts). The `publicArtifacts.length > 0` guard therefore skips pruning in both cases, leaving stale bridge entries in the wiki indefinitely whenever all artifacts are genuinely removed while memory-core is still loaded.
A safer discriminator would check whether the capability is registered at all, rather than relying on the artifact count as a proxy. For example, exporting `isMemoryCapabilityRegistered()` from `memory-state.ts` and threading it through the plugin-sdk interface would let the guard be `const shouldPrune = isMemoryCapabilityRegistered()` instead of `publicArtifacts.length > 0`.
How can I resolve this? If you propose a fix, please make it concise.| if (cron) { | ||
| // Refresh the startup capture so subsequent calls resolve immediately. | ||
| resolveStartupCron = () => cron; | ||
| } |
There was a problem hiding this comment.
resolveStartupCron = () => cron closes over the let cron local variable from this invocation of reconcileManagedDreamingCron. In JavaScript, closures capture the variable binding, so if cron were reassigned after this line within the same call, resolveStartupCron() would return the new value. While cron is not reassigned here after this point, an explicit capture is safer and makes the intent clearer:
| if (cron) { | |
| // Refresh the startup capture so subsequent calls resolve immediately. | |
| resolveStartupCron = () => cron; | |
| } | |
| resolveStartupCron = ((): (() => CronServiceLike | null) => { | |
| const captured = cron; | |
| return () => captured; | |
| })(); |
This eliminates any ambiguity about whether the cached reference is the cron value at assignment time.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/memory-core/src/dreaming.ts
Line: 724-727
Comment:
**Closure captures a local variable, not the resolved value**
`resolveStartupCron = () => cron` closes over the `let cron` local variable from this invocation of `reconcileManagedDreamingCron`. In JavaScript, closures capture the variable binding, so if `cron` were reassigned after this line within the same call, `resolveStartupCron()` would return the new value. While `cron` is not reassigned here after this point, an explicit capture is safer and makes the intent clearer:
```suggestion
resolveStartupCron = ((): (() => CronServiceLike | null) => {
const captured = cron;
return () => captured;
})();
```
This eliminates any ambiguity about whether the cached reference is the `cron` value at assignment time.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
224036f to
cbc9c2d
Compare
openclaw-barnacle
Bot
added
size: XS
and removed
docs
cbc9c2d to
f473616
Compare
When memory-core plugin is not registered (e.g. CLI context), listActiveMemoryPublicArtifacts returns an empty array. The previous code would then call pruneImportedSourceEntries with an empty activeKeys Set, which removes ALL bridge-imported entries. Now checks getMemoryCapabilityRegistration() instead of relying on artifact count as a proxy, correctly distinguishing between 'plugin not loaded' and 'plugin loaded with no artifacts'. Fixes openclaw#68373
f473616 to
3d3b195
Compare
fabianwilliams
left a comment
fabianwilliams
left a comment
There was a problem hiding this comment.
Targeted fix — guarding the prune step on getMemoryCapabilityRegistration prevents a CLI-context invocation from interpreting an empty publicArtifacts list as 'all bridges should be removed.' Comment + #68373 link make the trap clear. Export added cleanly via memory-host-sdk runtime-core. LGTM.
…penclaw#71764) When memory-core plugin is not registered (e.g. CLI context), listActiveMemoryPublicArtifacts returns an empty array. The previous code would then call pruneImportedSourceEntries with an empty activeKeys Set, which removes ALL bridge-imported entries. Now checks getMemoryCapabilityRegistration() instead of relying on artifact count as a proxy, correctly distinguishing between 'plugin not loaded' and 'plugin loaded with no artifacts'. Fixes openclaw#68373
…penclaw#71764) When memory-core plugin is not registered (e.g. CLI context), listActiveMemoryPublicArtifacts returns an empty array. The previous code would then call pruneImportedSourceEntries with an empty activeKeys Set, which removes ALL bridge-imported entries. Now checks getMemoryCapabilityRegistration() instead of relying on artifact count as a proxy, correctly distinguishing between 'plugin not loaded' and 'plugin loaded with no artifacts'. Fixes openclaw#68373
…penclaw#71764) When memory-core plugin is not registered (e.g. CLI context), listActiveMemoryPublicArtifacts returns an empty array. The previous code would then call pruneImportedSourceEntries with an empty activeKeys Set, which removes ALL bridge-imported entries. Now checks getMemoryCapabilityRegistration() instead of relying on artifact count as a proxy, correctly distinguishing between 'plugin not loaded' and 'plugin loaded with no artifacts'. Fixes openclaw#68373
…penclaw#71764) When memory-core plugin is not registered (e.g. CLI context), listActiveMemoryPublicArtifacts returns an empty array. The previous code would then call pruneImportedSourceEntries with an empty activeKeys Set, which removes ALL bridge-imported entries. Now checks getMemoryCapabilityRegistration() instead of relying on artifact count as a proxy, correctly distinguishing between 'plugin not loaded' and 'plugin loaded with no artifacts'. Fixes openclaw#68373
…penclaw#71764) When memory-core plugin is not registered (e.g. CLI context), listActiveMemoryPublicArtifacts returns an empty array. The previous code would then call pruneImportedSourceEntries with an empty activeKeys Set, which removes ALL bridge-imported entries. Now checks getMemoryCapabilityRegistration() instead of relying on artifact count as a proxy, correctly distinguishing between 'plugin not loaded' and 'plugin loaded with no artifacts'. Fixes openclaw#68373
…penclaw#71764) When memory-core plugin is not registered (e.g. CLI context), listActiveMemoryPublicArtifacts returns an empty array. The previous code would then call pruneImportedSourceEntries with an empty activeKeys Set, which removes ALL bridge-imported entries. Now checks getMemoryCapabilityRegistration() instead of relying on artifact count as a proxy, correctly distinguishing between 'plugin not loaded' and 'plugin loaded with no artifacts'. Fixes openclaw#68373
…penclaw#71764) When memory-core plugin is not registered (e.g. CLI context), listActiveMemoryPublicArtifacts returns an empty array. The previous code would then call pruneImportedSourceEntries with an empty activeKeys Set, which removes ALL bridge-imported entries. Now checks getMemoryCapabilityRegistration() instead of relying on artifact count as a proxy, correctly distinguishing between 'plugin not loaded' and 'plugin loaded with no artifacts'. Fixes openclaw#68373
2 participants
Problem
When memory-core plugin is not loaded (e.g. CLI context), listActiveMemoryPublicArtifacts returns an empty array. The syncMemoryWikiBridgeSources function then calls pruneImportedSourceEntries with an empty activeKeys Set, which removes ALL bridge-imported entries.
This causes a race condition: any CLI wiki command after a gateway bridge import destroys all imported artifacts.
Fix
Skip pruning when publicArtifacts is empty, since that means memory-core is not loaded and we cannot know the actual artifact list.
Fixes #68373