fix(memory-core): use Promise.allSettled in searchMemoryCorpusSupplements#77899
Closed
SimbaKingjoe wants to merge 1 commit into
Closed
fix(memory-core): use Promise.allSettled in searchMemoryCorpusSupplements#77899SimbaKingjoe wants to merge 1 commit into
SimbaKingjoe wants to merge 1 commit into
Conversation
…ents to prevent single supplement failure from discarding all results Promise.all fails fast — if one corpus supplement (e.g. memory-wiki) throws, all supplement results are lost, including from healthy supplements. Switch to Promise.allSettled so surviving results are still returned when individual supplements fail.
openclaw-barnacle
Bot
added
extensions: memory-core
Contributor
|
Codex review: needs real behavior proof before merge. Reviewed July 3, 2026, 5:02 PM ET / 21:02 UTC. Summary PR surface: Source 0. Total 0 across 1 file. Reproducibility: yes. Source inspection gives a high-confidence path: register two memory corpus supplements, make one or every Review metrics: none identified. Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Land one narrow fix for #77897 that preserves fulfilled sibling supplement results while keeping every-supplement-failed searches visibly unavailable or partial, with focused regression coverage and real behavior proof. Do we have a high-confidence way to reproduce the issue? Yes. Source inspection gives a high-confidence path: register two memory corpus supplements, make one or every Is this the best way to solve the issue? No as submitted. The helper is the right owner boundary, but the best fix should preserve fulfilled sibling results without hiding the all-failed supplement outage. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 010b61746379. Label changesLabel justifications:
Evidence reviewedPR surface: Source 0. Total 0 across 1 file. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
Member
|
Heads up: this PR needs to be updated against current |
clawsweeper
Bot
added
rating: 🧂 unranked krab
openclaw-barnacle
Bot
added
the
proof: supplied
clawsweeper
Bot
added
rating: 🌊 off-meta tidepool
clawsweeper
Bot
added
rating: 🧂 unranked krab
Contributor
|
Thanks for the contribution. I’m closing this stale PR because the latest review rated the current branch F: it still lacks the requested real-behavior proof or merge-ready branch shape, and there has been no human follow-up after the review. A fresh, focused PR against current |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
searchMemoryCorpusSupplementsusesPromise.allwhich fails fast — a single supplement failure discards ALL results. Switch toPromise.allSettledso surviving results are preserved.Promise.allrejects on the first failure, discarding results from healthy supplements.corpus=all, a single misbehaving supplement (corrupted wiki vault, network error) causes all supplement results to be lost. Future supplement registrations multiply the blast radius.Promise.all→Promise.allSettled+.flatMap(r => r.status === "fulfilled" ? r.value : [])Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Root Cause
Promise.allis fail-fast by design. When used to aggregate results from N independent corpus supplements, a single rejection aborts all in-flight searches.Regression Test Plan
extensions/memory-wiki/src/query.test.ts(20 tests) continues to pass.User-visible / Behavior Changes
None. Successful searches return the same results. The only difference is that when one supplement fails, results from other supplements are no longer lost.
Security Impact
Compatibility / Migration
Risks and Mitigations
Real behavior proof (required for external PRs)
Promise.allfail-fast discards all supplement results when one fails.Promise.allSettledisolates failures.pnpm test extensions/memory-wiki/src/query.test.ts --runAll 20 existing wiki query tests pass:
Inline
Promise.allSettledbehavior proof — 5/5 scenarios pass, including the critical case:await Promise.all([...supplements.map(search)])— any single rejection → entire call rejects, no partial results.