fix: exec tool gateway crash (#68376) and memory-core dreaming bloat (#68379)#68445
fix: exec tool gateway crash (#68376) and memory-core dreaming bloat (#68379)#68445mushuiyu886 wants to merge 2 commits into
Conversation
…g bloat (openclaw#68379) Addresses two production issues with improvements from code review feedback: openclaw#68376 — exec tool gateway crash: - Use instanceof Promise check instead of optional chaining to safely handle void callbacks that may return non-Promise truthy values - Update onUpdate type to void | Promise<void> - Add explanatory comment in catch body openclaw#68379 — memory-core dreaming bloat: - Use day-based session key instead of ms timestamp to reuse narrative sessions within the same day - Use fs.stat for corpus size cap check before reading the full file - Add 2MB cap on per-day session corpus files - Build new entries object in writeStore instead of mutating caller's store, with 4096 entry LRU eviction cap - Narrow scope-mismatch check to "missing scope" only (not broad "permission" substring)
…rrative sessionKey uses a day-bucket for session reuse within the same calendar day, while idempotencyKey includes the raw nowMs timestamp to stay unique per run. Previously both used the day-bucket, causing the gateway to dedupe fresh narrative sweeps as stale cached results. Also updates the test to assert the split key formats.
openclaw-barnacle
Bot
added
extensions: memory-core
Greptile SummaryThis PR fixes two distinct production issues: a gateway crash from unhandled promise rejections in the exec tool's Confidence Score: 5/5Safe to merge — all findings are P2 style concerns, and both bug fixes are targeted and correct. The exec-tool crash fix is minimal and correctly handles the Promise rejection race. The four dreaming/memory bloat fixes are well-reasoned and match the stated goals. The only concern is the errMsg.includes("missing scope") string match for log-level branching, which is a style/robustness issue but does not affect correctness or user-facing behavior. No P0 or P1 issues found. No files require special attention beyond the minor string-match concern in extensions/memory-core/src/dreaming-narrative.ts. Prompt To Fix All With AIThis is a comment left during a code review.
Path: extensions/memory-core/src/dreaming-narrative.ts
Line: 963
Comment:
**Brittle error classification via message substring match**
`errMsg.includes("missing scope")` ties the log-level branch to the current English error message wording. If the upstream error format changes (different phrasing, added prefix, localization), this silently falls through to the `warn` branch — the symptom that motivated the change would reappear without any clear signal.
Consider matching on a structured error code or type instead. If the upstream surface exposes an error code (e.g. `code === "MISSING_SCOPE"` or a typed error subclass), prefer that. As a fallback, anchoring to a more specific substring like `"missing scope: operator"` is slightly more robust, but a code-based check would be more durable.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(memory-core): split idempotencyKey f..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // logged at info level instead of warning — they are expected when the | ||
| // plugin lacks admin scope and are not actionable by the user (#68379). | ||
| const errMsg = formatErrorMessage(cleanupErr); | ||
| if (errMsg.includes("missing scope")) { |
There was a problem hiding this comment.
errMsg.includes("missing scope") ties the log-level branch to the current English error message wording. If the upstream error format changes (different phrasing, added prefix, localization), this silently falls through to the warn branch — the symptom that motivated the change would reappear without any clear signal.
Consider matching on a structured error code or type instead. If the upstream surface exposes an error code (e.g. code === "MISSING_SCOPE" or a typed error subclass), prefer that. As a fallback, anchoring to a more specific substring like "missing scope: operator" is slightly more robust, but a code-based check would be more durable.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/memory-core/src/dreaming-narrative.ts
Line: 963
Comment:
**Brittle error classification via message substring match**
`errMsg.includes("missing scope")` ties the log-level branch to the current English error message wording. If the upstream error format changes (different phrasing, added prefix, localization), this silently falls through to the `warn` branch — the symptom that motivated the change would reappear without any clear signal.
Consider matching on a structured error code or type instead. If the upstream surface exposes an error code (e.g. `code === "MISSING_SCOPE"` or a typed error subclass), prefer that. As a fallback, anchoring to a more specific substring like `"missing scope: operator"` is slightly more robust, but a code-based check would be more durable.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
The codebase does not yet expose a structured error code for scope errors — the UI layer (ui/src/ui/controllers/scope-errors.ts:16) uses the
exact same err.message.includes("missing scope: operator.read") pattern, with a comment explicitly stating "RPC scope failures do not yet
expose a dedicated structured detail code." This is a problem that requires adding structured error codes at the gateway protocol layer to
resolve properly, and is not suitable for introducing a new error type in this PR alone.
|
Closing this as implemented after Codex automated review. Current main already covers the two linked bug surfaces through shipped exec restart-state hardening and memory-core dreaming safeguards; this unmerged multi-issue branch is now obsolete. Best possible solution: Close this PR as obsolete because current main already covers the linked production failures. Keep the shipped restart queue and dreaming safeguards; handle any residual onUpdate/corpus/store-cap hardening as a separate focused PR against current main with tests. 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 7d9dc8cf24fc; fix evidence: release v2026.4.23, commit a9797214338b. |
1 participant
Summary
onUpdatecallback. TheemitUpdate()call could produce a rejected Promise from an asynconUpdate, but the synchronous call site never caught it, triggering the global unhandled-rejection handler (process.exit(1)).sessionKeyto prevent unbounded session growth from per-heartbeat unique keysidempotencyKey(uses rawnowMs) so the gateway does not cache/dedupe fresh narrative sweeps within the same day-bucket sessionTest plan
pnpm checkpasses (lint, tsgo, import cycles)extensions/memory-core/src/dreaming-narrative.test.ts— all 42 tests pass