fix(memory): improve error message when node:sqlite is unavailable#69199
fix(memory): improve error message when node:sqlite is unavailable#69199rrrrrredy wants to merge 2 commits into
Conversation
Greptile SummaryThis PR narrows the error-message logic in Confidence Score: 5/5Safe to merge — only changes error-message branching logic with a corresponding test. The sole remaining finding is a P2 style inconsistency (raw No files require special attention. Prompt To Fix All With AIThis is a comment left during a code review.
Path: extensions/memory-core/src/tools.shared.ts
Line: 122-124
Comment:
**Inconsistent string normalization between checks**
`isQuotaError` pipes `reason` through `normalizeLowercaseStringOrEmpty` before testing, but `isSqliteMissing` tests the raw `reason` and relies on the `i` flag instead. Both approaches work, but mixing them is needlessly inconsistent. Consider normalizing `isSqliteMissing` the same way for symmetry, or note the intentional difference with a comment.
```suggestion
const isQuotaError = /insufficient_quota|quota|429/.test(normalizeLowercaseStringOrEmpty(reason));
const normalizedReason = normalizeLowercaseStringOrEmpty(reason);
const isSqliteMissing =
/node:sqlite|no such built-in module.*sqlite|missing node:sqlite/.test(normalizedReason);
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(memory): improve sqlite unavailable ..." | Re-trigger Greptile |
| const isQuotaError = /insufficient_quota|quota|429/.test(normalizeLowercaseStringOrEmpty(reason)); | ||
| const warning = isQuotaError | ||
| ? "Memory search is unavailable because the embedding provider quota is exhausted." | ||
| : "Memory search is unavailable due to an embedding/provider error."; | ||
| const action = isQuotaError | ||
| ? "Top up or switch embedding provider, then retry memory_search." | ||
| : "Check embedding provider configuration and retry memory_search."; | ||
| const isSqliteMissing = | ||
| /node:sqlite|no such built-in module.*sqlite|missing node:sqlite/i.test(reason); |
There was a problem hiding this comment.
Inconsistent string normalization between checks
isQuotaError pipes reason through normalizeLowercaseStringOrEmpty before testing, but isSqliteMissing tests the raw reason and relies on the i flag instead. Both approaches work, but mixing them is needlessly inconsistent. Consider normalizing isSqliteMissing the same way for symmetry, or note the intentional difference with a comment.
| const isQuotaError = /insufficient_quota|quota|429/.test(normalizeLowercaseStringOrEmpty(reason)); | |
| const warning = isQuotaError | |
| ? "Memory search is unavailable because the embedding provider quota is exhausted." | |
| : "Memory search is unavailable due to an embedding/provider error."; | |
| const action = isQuotaError | |
| ? "Top up or switch embedding provider, then retry memory_search." | |
| : "Check embedding provider configuration and retry memory_search."; | |
| const isSqliteMissing = | |
| /node:sqlite|no such built-in module.*sqlite|missing node:sqlite/i.test(reason); | |
| const isQuotaError = /insufficient_quota|quota|429/.test(normalizeLowercaseStringOrEmpty(reason)); | |
| const normalizedReason = normalizeLowercaseStringOrEmpty(reason); | |
| const isSqliteMissing = | |
| /node:sqlite|no such built-in module.*sqlite|missing node:sqlite/.test(normalizedReason); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/memory-core/src/tools.shared.ts
Line: 122-124
Comment:
**Inconsistent string normalization between checks**
`isQuotaError` pipes `reason` through `normalizeLowercaseStringOrEmpty` before testing, but `isSqliteMissing` tests the raw `reason` and relies on the `i` flag instead. Both approaches work, but mixing them is needlessly inconsistent. Consider normalizing `isSqliteMissing` the same way for symmetry, or note the intentional difference with a comment.
```suggestion
const isQuotaError = /insufficient_quota|quota|429/.test(normalizeLowercaseStringOrEmpty(reason));
const normalizedReason = normalizeLowercaseStringOrEmpty(reason);
const isSqliteMissing =
/node:sqlite|no such built-in module.*sqlite|missing node:sqlite/.test(normalizedReason);
```
How can I resolve this? If you propose a fix, please make it concise.|
Status update as of 2026-04-23: this is the active clean respin for #59457, the current checks are green, and the scope is limited to the SQLite-unavailable messaging fix in |
|
Codex review: needs maintainer review before merge. Reviewed June 22, 2026, 2:08 PM ET / 18:08 UTC. Summary PR surface: Source +14, Tests +42. Total +56 across 2 files. Reproducibility: yes. Current main source routes missing Review metrics: none identified. Stored data model 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. Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land this narrow unavailable-payload formatter/test change, and keep degraded keyword fallback behavior in #93150 or #93260 as the separate capability decision. Do we have a high-confidence way to reproduce the issue? Yes. Current main source routes missing Is this the best way to solve the issue? Yes. The narrow maintainable fix is to classify known SQLite-runtime messages inside the existing unavailable-result helper while preserving quota, generic, and explicit override behavior. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 25090056dc22. Label changesLabel justifications:
Evidence reviewedPR surface: Source +14, Tests +42. Total +56 across 2 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 PR egg 🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat. Where did the egg go?
|
|
Heads up: this PR needs to be updated against current |
|
This pull request has been automatically marked as stale due to inactivity. |
7492cf1 to
c4d7092
Compare
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
Thanks @rrrrrredy for the original scoped implementation in #69199. I am closing this because the replacement PR has now landed: #95916, merged as If the landed behavior does not match the original intent or a separate reproduction remains, please reply and we can reopen or split that follow-up. |
Summary
extensions/memory-core.node:sqlite/ missing built-in sqlite runtime errors inbuildMemorySearchUnavailableResult(error, overrides?).warning/actionoverrides authoritative.String#includes()for the sqlite-missing checks.Behavior
When the runtime cannot load
node:sqlite,memory_searchnow returns the same unavailable result shape with a targeted warning/action instead of the generic embedding-provider message.The action matches current OpenClaw support policy: Node.js
>=22.19.0, with Node.js 24 recommended. The doc link uses the valid Memory CLI page: https://docs.openclaw.ai/cli/memory.Real Behavior Proof
memory_searchunavailable payloads now classify missingnode:sqliteruntime errors with targeted warning/action text while preserving quota, generic, and explicit override behavior.node -e "try { require('node:sqlite'); console.log('node:sqlite available'); } catch (error) { console.log(String(error.message)); }";node --import tsx -e "import { buildMemorySearchUnavailableResult } from './extensions/memory-core/src/tools.shared.ts'; ...";node scripts/test-projects.mjs extensions/memory-core/src/tools.test.ts;node node_modules/oxlint/bin/oxlint extensions/memory-core/src/tools.shared.ts extensions/memory-core/src/tools.test.ts;node node_modules/oxlint/bin/oxlint --tsconfig config/tsconfig/oxlint.extensions.json extensions/memory-core.node:sqliteerror returnsdisabled: true,unavailable: true, the original error string, and targeted Node.js upgrade guidance; customwarningandactionoverrides still win; the targeted memory-core tests pass.Old-runtime reproduction of the missing builtin:
Current branch unavailable payload for that error:
Override preservation check:
Targeted test run on Node.js 24.13.0:
Lint checks:
Note:
node scripts/run-bundled-extension-oxlint.mjsandnode scripts/run-oxlint-shards.mjs --threads=8both timed out locally on Windows after 5 minutes with no diagnostics, but the CI failure was the twoprefer-includesfindings inextensions/memory-core/src/tools.shared.ts; the changed file and memory-core extension path now pass oxlint directly.Related