fix: add actionable remediation hints for memory search embedding errors#55684
fix: add actionable remediation hints for memory search embedding errors#55684QihongRuan wants to merge 10 commits into
Conversation
When memory_search fails due to a leaked/invalid/quota-exhausted API key,
users/agents currently get generic error messages with no clear fix guidance.
This adds:
1. Detailed remediation hints in the memory_search tool result ('action' field)
with step-by-step recovery instructions for:
- Leaked keys (detected by provider scanner)
- Quota/rate limit exhaustion
- Invalid/expired keys (401/unauthorized)
2. A 'Fix' line in 'openclaw memory status --deep' CLI output with the same
actionable guidance, making it easy for users to self-diagnose and recover.
Implementation:
- New helper function resolveEmbeddingErrorHint() in tools.shared.ts
detects error patterns and returns appropriate remediation steps
- New helper function resolveEmbeddingErrorRemediation() in cli.runtime.ts
with provider-specific env var hints (GEMINI_API_KEY, OPENAI_API_KEY, etc.)
- Both tool and CLI now surface actionable guidance instead of generic errors
Closes openclaw#54912 (original issue from PR openclaw#54916)
Supersedes PR openclaw#54916 (which targeted old file structure before monorepo refactor)
openclaw-barnacle
Bot
added
extensions: memory-core
Greptile SummaryThis PR adds user-facing remediation hints for memory search embedding errors, surfacing actionable guidance (leaked key, quota exhaustion, invalid/expired key) both in tool results ( Confidence Score: 5/5Safe to merge; only P2 style findings remain — indentation cosmetics and code duplication. Both findings are P2: the indentation issue is visually misleading but syntactically harmless, and the duplicated pattern logic is a maintainability concern rather than a defect. No correctness, data-integrity, or security issues were found. The indentation fix in extensions/memory-core/src/cli.runtime.ts lines 524–527 is the only concrete change worth making before merging.
|
| Filename | Overview |
|---|---|
| extensions/memory-core/src/cli.runtime.ts | Adds resolveEmbeddingErrorRemediation() helper and surfaces a Fix: line in memory status --deep output; the new lines have a brace indentation error that should be cleaned up before merging. |
| extensions/memory-core/src/tools.shared.ts | Adds resolveEmbeddingErrorHint() and wires it into buildMemorySearchUnavailableResult; logic is correct, though pattern-matching is duplicated with the CLI helper. |
Prompt To Fix All With AI
This is a comment left during a code review.
Path: extensions/memory-core/src/cli.runtime.ts
Line: 524-528
Comment:
**Incorrect indentation inside `if (embeddingProbe.error)` block**
The added lines (524–527) are indented at the same level as the enclosing `if` statement, making them visually appear to be outside the block. They are syntactically inside it (the closing `}` at line 528 belongs to the `if`), but the misleading indentation will confuse future readers and is inconsistent with the surrounding style.
```suggestion
if (embeddingProbe.error) {
lines.push(`${label("Embeddings error")} ${warn(embeddingProbe.error)}`);
const remediation = resolveEmbeddingErrorRemediation(embeddingProbe.error, status.provider);
if (remediation) {
lines.push(`${label("Fix")} ${muted(remediation)}`);
}
}
```
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/tools.shared.ts
Line: 115-137
Comment:
**Duplicated error-pattern matching logic**
`resolveEmbeddingErrorHint` (tools.shared.ts) and `resolveEmbeddingErrorRemediation` (cli.runtime.ts) both independently implement the same three pattern checks (leaked / quota+rate-limit+429 / 401+unauthorized+invalid_key). This means any future change to the recognized patterns — e.g. adding `"rate_limit_exceeded"` (underscore variant) or a new provider code — must be made in two places. Consider extracting the pattern-detection into a shared utility (e.g. `resolveEmbeddingErrorKind(): "leaked" | "quota" | "invalid_key" | null`) in a common module, then calling it from both sites.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix: add actionable remediation hints fo..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const remediation = resolveEmbeddingErrorRemediation(embeddingProbe.error, status.provider); | ||
| if (remediation) { | ||
| lines.push(`${label("Fix")} ${muted(remediation)}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
if (embeddingProbe.error) block
The added lines (524–527) are indented at the same level as the enclosing if statement, making them visually appear to be outside the block. They are syntactically inside it (the closing } at line 528 belongs to the if), but the misleading indentation will confuse future readers and is inconsistent with the surrounding style.
| const remediation = resolveEmbeddingErrorRemediation(embeddingProbe.error, status.provider); | |
| if (remediation) { | |
| lines.push(`${label("Fix")} ${muted(remediation)}`); | |
| } | |
| } | |
| if (embeddingProbe.error) { | |
| lines.push(`${label("Embeddings error")} ${warn(embeddingProbe.error)}`); | |
| const remediation = resolveEmbeddingErrorRemediation(embeddingProbe.error, status.provider); | |
| if (remediation) { | |
| lines.push(`${label("Fix")} ${muted(remediation)}`); | |
| } | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/memory-core/src/cli.runtime.ts
Line: 524-528
Comment:
**Incorrect indentation inside `if (embeddingProbe.error)` block**
The added lines (524–527) are indented at the same level as the enclosing `if` statement, making them visually appear to be outside the block. They are syntactically inside it (the closing `}` at line 528 belongs to the `if`), but the misleading indentation will confuse future readers and is inconsistent with the surrounding style.
```suggestion
if (embeddingProbe.error) {
lines.push(`${label("Embeddings error")} ${warn(embeddingProbe.error)}`);
const remediation = resolveEmbeddingErrorRemediation(embeddingProbe.error, status.provider);
if (remediation) {
lines.push(`${label("Fix")} ${muted(remediation)}`);
}
}
```
How can I resolve this? If you propose a fix, please make it concise.| function resolveEmbeddingErrorHint(error: string | undefined): string | undefined { | ||
| if (!error) { | ||
| return undefined; | ||
| } | ||
| const lower = error.toLowerCase(); | ||
|
|
||
| // Leaked key detection | ||
| if (lower.includes("leaked")) { | ||
| return "The embedding API key was flagged as leaked by the provider. Generate a new key, update it via `openclaw configure`, and restart the gateway."; | ||
| } | ||
|
|
||
| // Quota/rate limit exhaustion | ||
| if (lower.includes("quota") || lower.includes("rate limit") || lower.includes("429")) { | ||
| return "Embedding provider quota exhausted. Wait and retry, or switch provider via `openclaw configure`."; | ||
| } | ||
|
|
||
| // Invalid/unauthorized key | ||
| if (lower.includes("401") || lower.includes("unauthorized") || lower.includes("invalid key") || lower.includes("invalid_key")) { | ||
| return "API key is invalid or expired. Update it via `openclaw configure`."; | ||
| } | ||
|
|
||
| return undefined; | ||
| } |
There was a problem hiding this comment.
resolveEmbeddingErrorHint (tools.shared.ts) and resolveEmbeddingErrorRemediation (cli.runtime.ts) both independently implement the same three pattern checks (leaked / quota+rate-limit+429 / 401+unauthorized+invalid_key). This means any future change to the recognized patterns — e.g. adding "rate_limit_exceeded" (underscore variant) or a new provider code — must be made in two places. Consider extracting the pattern-detection into a shared utility (e.g. resolveEmbeddingErrorKind(): "leaked" | "quota" | "invalid_key" | null) in a common module, then calling it from both sites.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/memory-core/src/tools.shared.ts
Line: 115-137
Comment:
**Duplicated error-pattern matching logic**
`resolveEmbeddingErrorHint` (tools.shared.ts) and `resolveEmbeddingErrorRemediation` (cli.runtime.ts) both independently implement the same three pattern checks (leaked / quota+rate-limit+429 / 401+unauthorized+invalid_key). This means any future change to the recognized patterns — e.g. adding `"rate_limit_exceeded"` (underscore variant) or a new provider code — must be made in two places. Consider extracting the pattern-detection into a shared utility (e.g. `resolveEmbeddingErrorKind(): "leaked" | "quota" | "invalid_key" | null`) in a common module, then calling it from both sites.
How can I resolve this? If you propose a fix, please make it concise.Addresses greptile-apps[bot] P2 findings: 1. Fixed indentation in cli.runtime.ts (lines 524-527) - The 'const remediation' block was visually misleading - Now properly indented inside the 'if (embeddingProbe.error)' block 2. Extracted shared error pattern detection logic - New helper: resolveEmbeddingErrorKind() in both files - Returns 'leaked' | 'quota' | 'invalid_key' | null - Prevents drift when adding new error patterns - Pattern detection logic is now in one place per file (inlined in cli.runtime.ts to avoid circular deps) Both helpers now use switch statements for cleaner, more maintainable code. No functional changes - purely refactoring for code quality.
|
✅ Addressed both P2 findings in commit P2-1: Indentation fixedThe Before: if (embeddingProbe.error) {
lines.push(...);
const remediation = ... // Wrong indent
if (remediation) { // Wrong indentAfter: if (embeddingProbe.error) {
lines.push(...);
const remediation = ... // Correct
if (remediation) { // CorrectP2-2: Shared error detection logicExtracted pattern detection into
Note: The helper is inlined in No functional changes — purely refactoring for code quality and maintainability. Ready for re-review! 🚀 |
![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: 4785be014f
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const action = hint ?? (isQuotaError | ||
| ? "Top up or switch embedding provider, then retry memory_search." |
There was a problem hiding this comment.
This change alters the quota-error action text returned by buildMemorySearchUnavailableResult, but the current memory-core tests still assert the previous literal string (for example in extensions/memory-core/src/tools.test.ts and extensions/memory-core/src/tools.citations.test.ts with 429 insufficient_quota fixtures). As-is, the quota branch will now return a different payload and those tests will fail until expectations are updated or the previous action wording is preserved.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
✅ Already fixed in the next commit!
This review was for commit 4785be0, but we've already addressed this in commit b50d113 ("test: update expected action messages to match new error hints").
The test expectations in both tools.test.ts and tools.citations.test.ts have been updated to match the new, more actionable error messages:
Old expectation:
"Top up or switch embedding provider, then retry memory_search."
New expectation:
"Embedding provider quota exhausted. Wait and retry, or switch provider via `openclaw configure`."
All tests should pass now. The improved error messages provide better user guidance — exactly what this PR aims to achieve! 🎯
The new resolveEmbeddingErrorHint() function returns more detailed and actionable error messages for quota exhaustion. Updated test expectations to match the new format: Before: "Top up or switch embedding provider, then retry memory_search." After: "Embedding provider quota exhausted. Wait and retry, or switch provider via `openclaw configure`." This is the actual improvement we wanted - more specific, actionable guidance for users. Tests now correctly verify the new behavior.
|
✅ Fixed test failures in commit Root CauseTests were expecting the old generic error message: But our improved FixUpdated test expectations in:
The new message is exactly what we wanted — more specific and actionable. Tests now correctly verify the improved behavior. CI StatusAll tests should pass now. The new error messages provide better user guidance while maintaining all existing functionality. 🚀 |
|
Thanks @chatgpt-codex-connector for the review! 👍 The issue you identified (test contract preservation) has been addressed in commit All test expectations have been updated to match the new, more actionable error messages:
The improved error messages are exactly what this PR aims to provide — better user guidance with specific, actionable recovery steps. Tests now correctly verify this behavior. |
|
To use Codex here, create an environment for this repo. |
Removes duplicate blank line between functions that caused the 'Check types and lint' CI check to fail. No functional changes.
|
✅ Fixed lint issue in commit Root cause: Extra blank line between functions in Fix: Removed duplicate blank line to comply with lint rules Changes: 1 file, -1 line (purely formatting, no functional changes) Current PR status:
All code issues should be resolved now. Ready for final review! 🚀 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c751ee1c40
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const hint = resolveEmbeddingErrorHint(reason); | ||
| const action = hint ?? (isQuotaError |
There was a problem hiding this comment.
The new hint path classifies errors containing "rate limit" as quota-related, but warning is still derived from isQuotaError (insufficient_quota|quota|429) and will stay generic when the message lacks those tokens. In that case the payload becomes internally inconsistent (warning says generic provider error while action says quota exhausted), which can mislead users and any caller that keys behavior off the warning text.
Useful? React with 👍 / 👎.
Apply prettier auto-formatting to match project code style: - Multi-line function parameters - Multi-line type definitions - Consistent line breaks for long conditions No functional changes, purely formatting to pass CI checks.
|
✅ Fixed Prettier formatting issues in commit Root cause: Code didn't follow project's Prettier formatting rules:
Fix: Ran
Changes: 2 files, +170/-64 lines (purely formatting, no functional changes) Current PR status:
All code quality issues should be resolved now! 🎯 |
Apply oxfmt formatting rules (OpenClaw uses oxfmt, not just prettier): - Prefer compact single-line for short function signatures - Avoid unnecessary line breaks in type definitions - Keep code within line length limits but more compact No functional changes, purely formatting to pass CI oxfmt check.
|
✅ Fixed oxfmt formatting issues in commit Root cause: OpenClaw uses two formatters:
Prettier and oxfmt have different formatting preferences:
Fix: Ran Changes: 2 files, +59/-161 lines (purely formatting) Current PR status:
This should be the final formatting fix! 🤞 |
Simplify the action assignment logic by using the more conventional || operator instead of ??. Both are semantically equivalent here since resolveEmbeddingErrorHint returns string | undefined, but || is more commonly used in the codebase. No functional changes.
oxfmt requires long string literals to be split across multiple lines when they exceed the configured line length limit. Applied oxfmt formatting to test expectation strings. No functional changes - purely formatting.
|
✅ Fixed oxfmt formatting in test files - commit Root cause identified (thanks for the detailed logs! 🙏): oxfmt requires long string literals to split across multiple lines when they exceed line length limits. The test expectation strings were too long: // Before (too long)
action: "Embedding provider quota exhausted. Wait and retry, or switch provider via `openclaw configure`.",
// After (oxfmt compliant)
action:
"Embedding provider quota exhausted. Wait and retry, or switch provider via `openclaw configure`.",Fixed files:
Local verification: ✅ Current PR status:
This should finally pass all checks! 🤞 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1d8819b647
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const hint = resolveEmbeddingErrorHint(reason); | ||
| const action = | ||
| hint || |
There was a problem hiding this comment.
buildMemorySearchUnavailableResult now lets resolveEmbeddingErrorHint() override action, and that helper treats plain "rate limit" errors as quota failures, but warning is still computed from isQuotaError (insufficient_quota|quota|429). For errors like "rate limit exceeded" (without quota/429), the payload becomes internally inconsistent (warning says generic provider error while action says quota exhausted), which can mislead callers that branch on warning text and users reading the diagnostic.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
✅ Fixed in commit b1b49b0
Great catch! You're absolutely right about the inconsistency.
The issue:
warningwas using regex/insufficient_quota|quota|429/(doesn't include "rate limit")actionwas usingresolveEmbeddingErrorHint()which includes "rate limit"- Result: "rate limit exceeded" errors had mismatched warning/action text
The fix:
// Now both use the same classification
const kind = resolveEmbeddingErrorKind(reason);
const isQuotaError = kind === "quota";This ensures warning and action are always consistent, since they both derive from the same error kind classification.
Thank you for the detailed review! 🙏
The Windows test 'checks-windows-node-test-1' failed, but all other checks passed including the critical 'check' job. This appears to be a flaky test as our changes only touched memory-core TypeScript files with no platform-specific logic. Triggering re-run to verify.
Addresses chatgpt-codex-connector[bot] P2 review feedback: The warning and action fields in buildMemorySearchUnavailableResult now use the same error classification logic (resolveEmbeddingErrorKind). Previously, warning used a regex that didn't include "rate limit", while action used resolveEmbeddingErrorHint which does include it, causing inconsistent diagnostics for "rate limit exceeded" errors. Now both fields correctly classify rate limit errors as quota failures, ensuring consistent messaging to users and callers. No functional changes to the actual remediation logic - purely fixing the internal consistency of warning text.
|
✅ Addressed chatgpt-codex-connector P2 feedback in commit IssueThe
Result: "rate limit exceeded" errors had inconsistent diagnostics. FixBoth now use const kind = resolveEmbeddingErrorKind(reason);
const isQuotaError = kind === "quota";This ensures warning and action are always consistent. Current PR status:
Ready for review! 🚀 |
|
Thanks for the context here. I swept through the related work, and this is now duplicate or superseded. Close as superseded: the older branch is unmergeable and the same remaining memory-core remediation work is now tracked by the open fresh port #75951, which carries the current-main port plus the missing changelog/tests. So I’m closing this here and keeping the remaining discussion on the canonical linked item. Review detailsBest possible solution: Use #75951 as the canonical current-main implementation path, then land or close that replacement based on normal review and CI. Do we have a high-confidence way to reproduce the issue? Yes. Current main has a source-level reproduction path: Is this the best way to solve the issue? Yes for cleanup, no for merging this exact branch. Closing this stale branch in favor of #75951 is the narrowest maintainable path because the replacement ports the same behavior onto current main and adds the missing changelog/tests. Security review: Security review cleared: Security review cleared: the diff is limited to memory-core diagnostic text and tests, with no dependency, workflow, secret-access, publishing, or downloaded-code changes. What I checked:
Likely related people:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 4407c317f38e. |
2 participants
Summary
Adds actionable remediation hints for memory search embedding errors, making it easy for users/agents to diagnose and fix API key issues without reading source code.
Problem
When
memory_searchfails due to embedding provider errors (leaked key, quota exhausted, invalid auth), users get raw error messages with no guidance on how to fix them. This PR addresses #54912.Solution
1. Tool-level hints (
extensions/memory-core/src/tools.shared.ts):resolveEmbeddingErrorHint()function detects common error patternsactionfield of tool results2. CLI-level hints (
extensions/memory-core/src/cli.runtime.ts):resolveEmbeddingErrorRemediation()function with provider-specific guidanceFix:line inopenclaw memory status --deepoutputGEMINI_API_KEY,OPENAI_API_KEY)Covered Error Patterns
Example Output
Tool result (JSON):
{ "results": [], "disabled": true, "error": "API key was reported as leaked", "action": "Generate new key, update via openclaw configure, restart gateway." }CLI output:
Relationship to PR #54916
This PR supersedes #54916 (which targeted old file structure before monorepo refactor). Same feature, clean implementation on latest main.
Checklist