fix(memory-wiki): skip bridge prune when capability unavailable but sources intact [AI-assisted]#67711
Conversation
Greptile SummaryAdds a guard in
Confidence Score: 4/5Safe to merge for the primary bug; the heuristic guard has a known false-negative edge case (stale pages not cleaned up when workspaces are legitimately removed but files remain) that should be documented or followed up. The fix correctly addresses the critical data-loss scenario (#67658) and is well-tested. The one remaining concern — the guard's inability to distinguish transient outage from legitimate empty results — is a correctness issue (wrong pruning decision in an edge case) that doesn't block the primary fix but warrants acknowledgment before merging. extensions/memory-wiki/src/bridge.ts lines 259–266 (the shouldSkipPrune condition and its comment) Prompt To Fix All With AIThis is a comment left during a code review.
Path: extensions/memory-wiki/src/bridge.ts
Line: 259-266
Comment:
**Guard conflates "unavailable capability" with "empty artifact list"**
`artifacts.length === 0` is used as a proxy for "capability temporarily unavailable," but this condition is also true when the capability IS available and legitimately returns zero results — e.g., the user removed all workspaces from `appConfig.agents.list` while source files remain on disk. In that scenario `shouldSkipPrune` becomes `true`, so stale bridge pages are never pruned even though all artifacts are genuinely gone and the prune is correct.
A more precise signal would be whether `listActiveMemoryPublicArtifacts` indicated unavailability (e.g., via a returned flag or a thrown/caught error) rather than inferring it from the result count. With the current SDK surface that may not be possible without an API change, but the comment should at minimum call out this known limitation so future readers understand the tradeoff.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(memory-wiki): skip bridge prune when..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const shouldSkipPrune = | ||
| artifacts.length === 0 && | ||
| existingBridgeSourcePaths.length > 0 && | ||
| (await Promise.any( | ||
| existingBridgeSourcePaths.map((sourcePath) => | ||
| fs.access(sourcePath).then(() => true), | ||
| ), | ||
| ).catch(() => false)); |
There was a problem hiding this comment.
artifacts.length === 0 is used as a proxy for "capability temporarily unavailable," but this condition is also true when the capability IS available and legitimately returns zero results — e.g., the user removed all workspaces from appConfig.agents.list while source files remain on disk. In that scenario shouldSkipPrune becomes true, so stale bridge pages are never pruned even though all artifacts are genuinely gone and the prune is correct.
A more precise signal would be whether listActiveMemoryPublicArtifacts indicated unavailability (e.g., via a returned flag or a thrown/caught error) rather than inferring it from the result count. With the current SDK surface that may not be possible without an API change, but the comment should at minimum call out this known limitation so future readers understand the tradeoff.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/memory-wiki/src/bridge.ts
Line: 259-266
Comment:
**Guard conflates "unavailable capability" with "empty artifact list"**
`artifacts.length === 0` is used as a proxy for "capability temporarily unavailable," but this condition is also true when the capability IS available and legitimately returns zero results — e.g., the user removed all workspaces from `appConfig.agents.list` while source files remain on disk. In that scenario `shouldSkipPrune` becomes `true`, so stale bridge pages are never pruned even though all artifacts are genuinely gone and the prune is correct.
A more precise signal would be whether `listActiveMemoryPublicArtifacts` indicated unavailability (e.g., via a returned flag or a thrown/caught error) rather than inferring it from the result count. With the current SDK surface that may not be possible without an API change, but the comment should at minimum call out this known limitation so future readers understand the tradeoff.
How can I resolve this? If you propose a fix, please make it concise.![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f193e09f3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| .map((entry) => entry.sourcePath); | ||
|
|
||
| const shouldSkipPrune = | ||
| artifacts.length === 0 && |
There was a problem hiding this comment.
The new guard uses artifacts.length === 0 to infer a capability outage, but artifacts is post-filtered by bridge config (shouldImportArtifact in collectBridgeArtifacts). If a user disables an index flag (for example indexMemoryRoot: false) while the memory capability is healthy, artifacts becomes empty and this branch skips pruning, leaving previously imported bridge pages and sync-state entries that should be removed. This regresses config-driven cleanup behavior and can keep stale content visible indefinitely.
Useful? React with 👍 / 👎.
|
Addressed in the latest commit (4720a27): expanded the guard comment to explicitly document this known tradeoff. The heuristic intentionally favors data preservation over exact correctness: stale pages from a legitimate workspace removal will persist until the next sync cycle where the capability is available. This is acceptable because:
|
…ources intact When syncMemoryWikiBridgeSources runs with an empty artifacts list (e.g. the memory capability is temporarily unregistered), activeKeys is an empty Set. The subsequent pruneImportedSourceEntries call then treats every existing group=bridge entry as stale and deletes the corresponding wiki pages, even though the underlying source files are still present on disk. Fix: before calling pruneImportedSourceEntries, check whether the empty artifacts list is due to a transient capability outage. When no artifacts are returned but at least one historical bridge entry still has its source file accessible on disk, skip the prune entirely (removedCount=0) to avoid data loss. Pruning still proceeds normally when source files are genuinely gone. Adds two regression tests: - capability lost, source files intact: removedCount=0, pages preserved - capability lost AND source files deleted: removedCount>0, pages removed Fixes openclaw#67658
Address Greptile review: explicitly note in the guard comment that artifacts.length === 0 cannot distinguish a transient capability outage from a legitimate empty result (e.g. all workspaces removed). Stale pages may persist in the edge case until the next successful sync.
4720a27 to
46db3c3
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 46db3c3bf7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| .map((entry) => entry.sourcePath); | ||
|
|
||
| const shouldSkipPrune = | ||
| artifacts.length === 0 && |
There was a problem hiding this comment.
The new shouldSkipPrune heuristic uses artifacts.length === 0, but artifacts is already filtered by shouldImportArtifact; this makes normal config choices look like a capability outage. For example, when bridge index flags filter out all artifact kinds while legacy bridge entries still point to existing source files, this branch skips pruning and leaves stale bridge pages/state entries that should be removed to match current config. The outage check should key off an unfiltered signal (for example capability registration or publicArtifacts) instead of the post-filter list.
Useful? React with 👍 / 👎.
The shouldSkipPrune heuristic was checking artifacts.length (post-filter) which made normal config choices (e.g. disabling all bridge index flags) look like a capability outage, leaving stale bridge pages unpruned. Now checks publicArtifacts.length (pre-filter) so that: - Capability outage (zero raw artifacts + files on disk) → skip prune - Config filtering (raw artifacts exist but all filtered) → prune normally Addresses Codex P2 review on openclaw#67711.
|
Fixed in 88dff87 — the prune-skip guard now checks publicArtifacts.length (unfiltered, pre-shouldImportArtifact) instead of �rtifacts.length (filtered). This correctly distinguishes:
Added a regression test that imports with indexMemoryRoot: true, then re-syncs with indexMemoryRoot: false while the capability still returns the artifact — confirms stale pages are pruned. |
|
Closing this as implemented after Codex automated review. Current main already fixes the central data-loss path targeted by PR #67711. Best possible solution: Close PR #67711 as implemented on current main. Keep the mainline capability-registration guard as the product behavior; any desire for the PR branch's narrower source-file-existence heuristic or extra bridge-specific regression coverage should be handled as a new follow-up, not by keeping this stale PR open. What I checked:
So I’m closing this as already implemented rather than keeping a duplicate issue open. Codex Review notes: model gpt-5.5, reasoning high; reviewed against d54d2d6b9b8a; fix evidence: commit d54d2d6b9b8a. |
1 participant
AI-assisted: yes (Antigravity / Gemini).
Summary
Fixes #67658.
When syncMemoryWikiBridgeSources runs and listActiveMemoryPublicArtifacts returns an empty array (because the memory plugin capability is temporarily unregistered or unavailable), �ctiveKeys ends up as an empty Set. The subsequent call to pruneImportedSourceEntries then marks every existing group=bridge state entry as stale and deletes the corresponding wiki pages — even though the underlying source files are still on disk.
Root cause
�ridge.ts conflates two distinct situations:
Both produce an empty �rtifacts list, but only case 2 should trigger pruning.
Fix
Before calling pruneImportedSourceEntries, check whether the empty artifacts list coincides with existing bridge entries whose source files are still accessible on disk. If at least one source file is reachable via s.access, skip the prune entirely (
emovedCount = 0). When all source files are also gone, pruning proceeds as before.
` s
// Guard: skip pruning when capability is temporarily unavailable but
// source files still exist on disk. (#67658)
const existingBridgeSourcePaths = Object.values(state.entries)
.filter((entry) => entry.group === 'bridge')
.map((entry) => entry.sourcePath);
const shouldSkipPrune =
artifacts.length === 0 &&
existingBridgeSourcePaths.length > 0 &&
(await Promise.any(
existingBridgeSourcePaths.map((p) => fs.access(p).then(() => true)),
).catch(() => false));
`
Tests
Two new regression tests added to �ridge.test.ts:
| Scenario |
emovedCount | Pages on disk |
|---|---|---|
| Capability lost, source files still exist |