fix(memory-core): silence expected operator.admin scope miss in dreaming cleanup#68020
fix(memory-core): silence expected operator.admin scope miss in dreaming cleanup#68020ImLukeF wants to merge 1 commit into
Conversation
… dreaming cleanup The dreaming cron's narrative cleanup calls subagent.deleteSession, which dispatches sessions.delete (ADMIN_SCOPE). From a background cron there is no inherited operator scope, so the plugin subagent falls through to a synthetic operator client that intentionally defaults to [WRITE_SCOPE] and does not mint admin (codified by 'rejects fallback session deletion without minting admin scope' in src/gateway/server-plugins.test.ts). Result: every dreaming cycle logged memory-core: narrative session cleanup failed for <phase> phase: missing scope: operator.admin The cleanup was already best-effort (try/catch), but the warn surfaced to users on every cycle and made healthy instances look broken. Treat the 'missing scope: operator.admin' case as an expected no-op and suppress the warn for just that specific error. Any other cleanup error (network, timeout, etc.) still surfaces as warn. Adds a test locking in the new behavior; existing test covering non-scope cleanup failures continues to pass unchanged.
openclaw-barnacle
Bot
added
extensions: memory-core
Greptile SummarySilences the expected Confidence Score: 5/5Safe to merge — change is a one-line log-suppression with a targeted test; no security invariants are weakened. Only finding is P2: No files require special attention. Prompt To Fix All With AIThis is a comment left during a code review.
Path: extensions/memory-core/src/dreaming-narrative.ts
Line: 945
Comment:
**`includes()` on formatted message can match cause-chain text**
`formatErrorMessage` appends nested `.cause` messages (separated by ` | `). If a future error wraps an unrelated primary error with `missing scope: operator.admin` as a cause, the warning would be silently dropped. Matching on the raw error or `startsWith` guards against this.
```suggestion
if (!(cleanupErr instanceof Error && cleanupErr.message === "missing scope: operator.admin")) {
```
This also makes the intent clearer: we are matching the primary error only, not anything in the full cause chain.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(memory-core): silence expected 'miss..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // delete with "missing scope: operator.admin" on every cycle, so treat | ||
| // it as an expected no-op rather than emitting noise. Genuine failures | ||
| // (any other error) are still surfaced as warn. | ||
| if (!errMessage.includes("missing scope: operator.admin")) { |
There was a problem hiding this comment.
includes() on formatted message can match cause-chain text
formatErrorMessage appends nested .cause messages (separated by |). If a future error wraps an unrelated primary error with missing scope: operator.admin as a cause, the warning would be silently dropped. Matching on the raw error or startsWith guards against this.
| if (!errMessage.includes("missing scope: operator.admin")) { | |
| if (!(cleanupErr instanceof Error && cleanupErr.message === "missing scope: operator.admin")) { |
This also makes the intent clearer: we are matching the primary error only, not anything in the full cause chain.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/memory-core/src/dreaming-narrative.ts
Line: 945
Comment:
**`includes()` on formatted message can match cause-chain text**
`formatErrorMessage` appends nested `.cause` messages (separated by ` | `). If a future error wraps an unrelated primary error with `missing scope: operator.admin` as a cause, the warning would be silently dropped. Matching on the raw error or `startsWith` guards against this.
```suggestion
if (!(cleanupErr instanceof Error && cleanupErr.message === "missing scope: operator.admin")) {
```
This also makes the intent clearer: we are matching the primary error only, not anything in the full cause chain.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Pull request overview
This PR reduces noisy logging in the memory-core “dreaming narrative” cleanup path by suppressing a known-expected authorization failure that occurs when the dreaming cron runs without inherited operator admin scope.
Changes:
- Suppress the cleanup
logger.warnonly whendeleteSessionfails with"missing scope: operator.admin". - Add a focused unit test ensuring the warning is not emitted for that expected background-cron fallback error.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| extensions/memory-core/src/dreaming-narrative.ts | Filters the specific expected "missing scope: operator.admin" cleanup error to avoid warning spam while preserving warnings for all other failures. |
| extensions/memory-core/src/dreaming-narrative.test.ts | Adds coverage asserting the cleanup warning is suppressed for the expected missing-admin-scope error case. |
|
Related work from PRtags group Title: Open PR duplicate: [Bug]: memory-core: narrative session cleanup fails with "missing scope: operator.admin"
|
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
Codex review: needs real behavior proof before merge. Reviewed June 15, 2026, 1:02 PM ET / 17:02 UTC. Summary PR surface: Source +12, Tests +25. Total +37 across 2 files. Reproducibility: no. I did not establish a current-main reproduction; the old release-era warning path is source-plausible, but current main now uses isolated cron plus owner-scoped plugin cleanup. Review metrics: none identified. 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:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Keep current main's isolated cron and owner-scoped plugin cleanup as the canonical fix, and only accept a rebased primary-error-only guard if current-main runtime proof still shows expected scope noise. Do we have a high-confidence way to reproduce the issue? No. I did not establish a current-main reproduction; the old release-era warning path is source-plausible, but current main now uses isolated cron plus owner-scoped plugin cleanup. Is this the best way to solve the issue? No. The log suppression was a plausible old-branch mitigation, but current main's isolated cron and plugin-owned cleanup are the cleaner owner-boundary solution; any remaining guard should be rebased, proof-backed, and primary-error-only. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 8ded75628437. Label changesLabel justifications:
Evidence reviewedPR surface: Source +12, Tests +25. Total +37 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
Bot
added
merge-risk: 🚨 other
|
Thanks @ImLukeF for the focused cleanup work here. I am closing this because the underlying memory-core dreaming cleanup path was fixed by the merged Clownfish replacement PR #84802 (#84802), which carried forward the credited contributor trail from the source work and landed the bounded session cleanup on May 21, 2026. This PR (#68020, #68020) covered the same expected |
Summary
Dreaming cron's narrative cleanup logs
memory-core: narrative session cleanup failed for <phase> phase: missing scope: operator.adminon every dreaming cycle, making healthy instances look broken.Root cause:
subagent.deleteSessiondispatchessessions.deletewhich requiresADMIN_SCOPE. In background-cron context there is no inherited operator scope, so the plugin subagent falls through to a synthetic operator client that intentionally defaults to[WRITE_SCOPE]and does not mint admin — this is codified by the existingrejects fallback session deletion without minting admin scopetest insrc/gateway/server-plugins.test.ts.The cleanup was already best-effort (wrapped in try/catch), so functionally nothing was broken — but the
warnsurfaced on every cycle.Fix
Suppress the
warnonly for the specific"missing scope: operator.admin"error (the expected background-cron path). Every other cleanup error — network, timeout, stale run, etc. — still surfaces as warn unchanged.This preserves the existing security invariant (background plugin calls don't auto-mint admin) rather than weakening scope enforcement just to quiet the log.
Why not grant admin to the synthetic client?
The existing test
rejects fallback session deletion without minting admin scopelocks in that behaviour as a deliberate security choice. Auto-minting admin in the fallback path would broaden privilege escalation surface for any plugin running in background context, which reviewers would correctly reject. Handling the known-expected error at the memory-core call site is the minimally invasive fix.Known caveat
Orphan narrative subagent sessions still accumulate because the delete is a no-op from the cron path. This PR does not address that — tracked separately. A follow-up could expose a privileged self-cleanup path (e.g.
sessions.delete.owned) scoped to sessions the caller created, but that's a larger design change.Test plan
does not warn when cleanup fails with missing operator.admin scope (background cron fallback path)— assertslogger.warnis not called with the cleanup message whendeleteSessionrejects withmissing scope: operator.adminwaits once more before cleanup after timeout and logs cleanup failures— unchanged, still passes (mock rejects with"still active", which is not the operator.admin case, so warn still fires)dreaming-narrative.test.tstests passserver-plugins.test.tstests pass (including the scope-enforcement test)Before / After
Before (every dreaming cycle, log level warn):
After: silent on the scope-mismatch case, still warns on genuine failures.