[AI] fix(memory): prevent empty-string expectedModel in resolveMemory…#91691
Conversation
|
Codex review: needs maintainer review before merge. Reviewed June 15, 2026, 5:10 AM ET / 09:10 UTC. Summary PR surface: Source 0, Tests +35. Total +35 across 3 files. Reproducibility: yes. at source level: current main still reads raw Review metrics: 1 noteworthy metric.
Stored data model Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land the narrow empty-expected guard only after memory owners confirm the Do we have a high-confidence way to reproduce the issue? Yes at source level: current main still reads raw Is this the best way to solve the issue? Not fully settled. The PR is a plausible narrow mitigation, but the best merge state needs owner confirmation that unresolved configured providers should compare as AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 501f63443f54. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source 0, Tests +35. Total +35 across 3 files. 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
|
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
@clawsweeper re-review |
|
🦞👀 Command router queued. I will update this comment with the next step. |
|
@clawsweeper re-review |
|
🦞👀 Command router queued. I will update this comment with the next step. Re-review progress:
|
|
@clawsweeper re-review |
…IndexIdentityState Refs openclaw#90787
64feeb5 to
e83e1e5
Compare
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
e83e1e5 to
878bcd8
Compare
878bcd8 to
f30016d
Compare
…ead of empty string Change resolveEmbeddingProviderFallbackModel fallbackSourceModel from empty string to fts-only so unregistered adapters produce a readable expectedModel instead of an empty string that causes permanent identity mismatch. Refs openclaw#90787
f30016d to
1dd9bcc
Compare
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
Prove that existing indexes with empty adapter model metadata transition safely after the backfill: stored meta (written by the identity path) already carries the correct model; after upgrade, provider.model matches the stored meta, preventing spurious identity mismatches on first boot. Ref: openclaw#90042, openclaw#91691
Prove that existing indexes with empty adapter model metadata transition safely after the backfill: stored meta (written by the identity path) already carries the correct model; after upgrade, provider.model matches the stored meta, preventing spurious identity mismatches on first boot. Ref: openclaw#90042, openclaw#91691
Summary
resolveMemoryIndexIdentityStatecomputesexpectedModelfromprovider.model, but whenprovider.modelis an empty string (as happens whenresolveEmbeddingProviderFallbackModelreturns""for an unregistered adapter, or whencreateWithAdapterreturned empty model before PR #90042), the expected model becomes""— causing a permanent identity mismatch that blocks vector search indefinitely.This PR fixes two code paths that could produce empty-string
expectedModel:Identity consumer layer (
manager-reindex-state.ts:123): Replaceparams.provider ? params.provider.model : "fts-only"withparams.provider?.model?.trim() || "fts-only". Empty or whitespace-onlyprovider.modelnow falls back to"fts-only"— the same safe default used when no provider is configured.Identity computation layer (
manager-sync-ops.ts:420): ChangeresolveEmbeddingProviderFallbackModel(provider, "", cfg)toresolveEmbeddingProviderFallbackModel(provider, "fts-only", cfg). When the adapter IS registered (e.g.,"openai"→ defaultModel"text-embedding-3-small"), the resolved default model is used. When the adapter is NOT registered (e.g.,"google"before the Google plugin loads),fallbackSourceModel = "fts-only"is returned instead of"".This two-layer approach addresses the ClawSweeper P1 finding: when a configured provider has an empty model, the resolved adapter default model (e.g.,
"text-embedding-3-small") is used for identity comparison."fts-only"only appears when both the adapter and the configured model are unavailable — matching the no-provider path semantics.Change Type
Scope
Linked Issue
Motivation
Users upgrading from 2026.5.26 to 2026.6.1 who previously used Google/Gemini as their memorySearch provider see
Dirty: yespermanently withreason: "index was built for model gemini-embedding-001, expected "— the emptyexpectedfield makes the index unrecoverable. PR #90042 fixed the root cause atcreateWithAdapter(provider initialization), but the identity check can still produce empty-stringexpectedModelbefore provider init completes, or when the adapter registry hasn't loaded yet.Changes
File:
extensions/memory-core/src/memory/manager-reindex-state.tsparams.provider ? params.provider.model : "fts-only"withparams.provider?.model?.trim() || "fts-only"— guard against empty/whitespaceprovider.modelat the identity comparison consumerFile:
extensions/memory-core/src/memory/manager-sync-ops.tsresolveEmbeddingProviderFallbackModel(this.settings.provider, "", this.cfg)toresolveEmbeddingProviderFallbackModel(this.settings.provider, "fts-only", this.cfg)— use"fts-only"as fallbackSourceModel instead of""so unregistered adapters produce a readable fallback instead of empty stringFile:
extensions/memory-core/src/memory/manager-reindex-state.test.tsNo new imports, no config changes, no default surface changes.
Verification
extensions/memory-core/src/memory/manager-reindex-state.test.ts— 12 passed (9 original + 3 new)extensions/memory-core/src/memory/embeddings.test.ts— 8 passed (original suite, PR Gateway memory_search index stuck dirty: provider.model empty during boot overwrites correct index identity #90042's 2 backfill tests are on a separate branch)Real Behavior Proof
Behavior addressed:
expectedModelinresolveMemoryIndexIdentityStatecan become an empty string""whenprovider.modelis empty, causing permanent identity mismatch ("index was built for model X, expected ") and blocking vector search recovery.Real environment tested: Linux (Ubuntu), Node.js 22.19+, local checkout on branch
fix/memory-identity-empty-model-90787.Exact steps or command run after this patch:
Evidence after fix: Direct runtime proof demonstrating the empty-string
provider.modelpath changed after the fix. Two focused scripts exercise the exact code paths that this PR changes:node /tmp/identity-proof.mjs): proves the identity consumer layer now produces"fts-only"instead of empty string for emptyprovider.model, while preserving behavior for normal values:node /tmp/fallback-proof.mjs): proves the pre-init configured-provider fallback now uses"fts-only"for unregistered adapters instead of empty string, while preserving resolved defaultModel for registered adapters:node scripts/run-vitest.mjs): directly testsresolveMemoryIndexIdentityStatewithprovider.model = ""→expectedModel = "fts-only"(not empty string):Before-fix comparison (from issue #90787 reporter, Rocky Linux, v2026.6.1):
expectedModel""(empty string)"fts-only"(safe fallback)"index was built for model gemini-embedding-001, expected "(trailing empty string)"index was built for model gemini-embedding-001, expected text-embedding-3-small"(adapter default) or"expected fts-only"(unregistered adapter)Observed result after fix: Two-layer protection ensures
expectedModelis never an empty string. When the adapter IS registered (e.g.,"openai"→"text-embedding-3-small"), the resolved default model is used for identity comparison. When the adapter is NOT registered,"fts-only"is used — matching the no-provider path semantics and producing a readable mismatch reason that points the user toward a reindex instead of a broken permanent-stuck state.What was not tested: The provider auto→openai migration decision (separate product issue with
needs-product-decisionlabel). This PR only fixes the identity-check and fallback-source layers; the migration logic that unconditionally rewritesprovider: "auto"to"openai"is not changed and requires maintainer product decision per #90787'sclawsweeper:needs-product-decisionlabel.Verification test output (12 tests, including 3 new):
Risks
provider.modelvalues.provider.modelis empty or whitespace, or whenfallbackSourceModelwould otherwise be"". Previously these produced broken empty-string mismatches. No change for correctly-populatedprovider.model.provider.modelat the creation source (createWithAdapter); this PR protects the identity check at the consumption layer and the fallback-source computation. Either fix alone reduces the bug surface; together they eliminate it.resolveEmbeddingProviderFallbackModel("openai", "fts-only", cfg)still returns"text-embedding-3-small"(adapter defaultModel) when the adapter is registered."fts-only"only appears when the adapter is unregistered — which is the correct semantic for the no-provider path.