Skip to content

fix(memory-wiki): skip prune when configured path is transiently unavailable (#97523)#97527

Closed
liuhao1024 wants to merge 1 commit into
openclaw:mainfrom
liuhao1024:fix/memory-wiki-transient-path-prune-guard
Closed

fix(memory-wiki): skip prune when configured path is transiently unavailable (#97523)#97527
liuhao1024 wants to merge 1 commit into
openclaw:mainfrom
liuhao1024:fix/memory-wiki-transient-path-prune-guard

Conversation

@liuhao1024

@liuhao1024 liuhao1024 commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

In memory-wiki vaultMode: "unsafe-local", a transiently unavailable configured source path (undocked external drive, NAS rebooting, cloud-synced folder not mounted yet) causes the next wiki sync to delete every imported page for that path AND destroy the user-authored human-notes block inside each page. The loss is permanent: on the next successful sync the page is recreated empty, with no prior content to restore the notes from.

Why This Change Was Made

collectUnsafeLocalArtifacts treats a missing path as "zero artifacts collected" (indistinguishable from "all sources deleted"). The sync function then calls pruneImportedSourceEntries unconditionally, which hard-deletes every state entry whose syncKey is not in activeKeys — wiping all imported pages.

The fix adds a guard: when zero artifacts are collected AND at least one configured path's parent directory is inaccessible, skip the prune entirely. This preserves imported pages and user-authored human-notes blocks during transient outages. When the parent directory IS accessible (user intentionally removed source files), pruning proceeds normally.

User Impact

Users with unsafe-local vault mode on removable/network/cloud-synced storage no longer lose imported pages and hand-written notes when the storage is temporarily unavailable.

Evidence

  • Behavior addressed: memory-wiki unsafe-local sync prunes imported pages when a configured path is transiently unavailable
  • Environment tested: macOS, Node 22, OpenClaw 2026.5.28
  • Steps run after the patch: Ran pnpm test:extension memory-wiki which exercises syncMemoryWikiUnsafeLocalSources with both transient unavailability (new test) and intentional file deletion (existing test)
  • Evidence after fix:
 Test Files  31 passed (31)
      Tests  187 passed (187)
   Start at  00:17:37
   Duration  17.89s
  • Observed result after fix: All 187 tests pass including the new transient-unavailability test and the existing prune-on-delete test. The guard correctly distinguishes between "parent directory inaccessible" (skip prune) and "file removed from accessible directory" (proceed with prune).
  • What was not tested: Live NAS/removable-drive unavailability scenario (requires physical hardware). The test simulates this by removing both the source directory and its parent mount point.

Real behavior proof

  • Behavior addressed: memory-wiki unsafe-local sync prunes imported pages when a configured path is transiently unavailable
  • Environment tested: macOS, Node 22, OpenClaw 2026.5.28
  • Steps run after the patch: Ran pnpm test:extension memory-wiki which exercises syncMemoryWikiUnsafeLocalSources with both transient unavailability (new test) and intentional file deletion (existing test)
  • Evidence after fix:
 Test Files  31 passed (31)
      Tests  187 passed (187)
   Start at  00:17:37
   Duration  17.89s

  • Observed result after fix: All 187 tests pass including the new transient-unavailability test and the existing prune-on-delete test. The guard correctly distinguishes between "parent directory inaccessible" (skip prune) and "file removed from accessible directory" (proceed with prune).

  • What was not tested: Live NAS/removable-drive unavailability scenario (requires physical hardware). The test simulates this by removing both the source directory and its parent mount point.

  • AI-assisted (Hermes Agent)

@clawsweeper

clawsweeper Bot commented Jun 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 28, 2026, 12:31 PM ET / 16:31 UTC.

Summary
The branch adds an unsafe-local prune guard that skips pruning when no artifacts are collected and some configured path parent is inaccessible, plus a single-path regression test.

PR surface: Source +22, Tests +45. Total +67 across 2 files.

Reproducibility: yes. source-backed. The linked issue includes a current-main production-function repro, and current main still drops missing configured paths from activeKeys before the shared prune deletes inactive entries.

Review metrics: 1 noteworthy metric.

  • Mixed-path coverage: 1 single-path outage test added, 0 mixed-path outage tests. unsafeLocal.paths is an array, and the prune decision needs coverage for one unavailable path alongside another configured path.

Stored data model
Persistent data-model change detected: serialized state: extensions/memory-wiki/src/unsafe-local.test.ts, unknown-data-model-change: extensions/memory-wiki/src/unsafe-local.test.ts, vector/embedding metadata: extensions/memory-wiki/src/unsafe-local.test.ts, vector/embedding metadata: extensions/memory-wiki/src/unsafe-local.ts. Confirm migration or upgrade compatibility proof before merge.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #97523
Summary: This PR is the candidate fix for the canonical unsafe-local data-loss issue, but the current branch does not fully fix the mixed configured-path root cause.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🧂 unranked krab
Result: blocked until real behavior proof from a real setup is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P1] Make pruning path-scoped and add a mixed-path regression for one unavailable configured path plus another active or intentionally emptied path.
  • [P1] Add redacted real behavior proof after the code fix; terminal output or logs from a real unsafe-local import flow are sufficient.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body provides extension test output only; it does not include redacted live terminal output, logs, or another real unsafe-local import workflow proof after the fix. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

  • [P1] The guard is all-or-nothing: with two configured paths, an unavailable path can still be pruned when another path produces artifacts, and an intentionally emptied available path can be retained when a different path is inaccessible and total artifacts are zero.
  • [P1] The PR body proof is extension-test output only, so this external PR still needs redacted real behavior proof from the unsafe-local import workflow before merge.

Maintainer options:

  1. Scope pruning per configured source path (recommended)
    Before merge, make the prune decision path-aware and add a mixed-path regression that proves unavailable scopes are preserved while available scopes still prune genuine deletions.
  2. Pause or replace this partial guard
    If the source-sync state cannot support path-scoped pruning cleanly on this branch, keep the canonical issue open and replace this PR with a narrower implementation.

Next step before merge

  • [P1] Author or maintainer revision is needed for path-scoped pruning, and the external contributor still needs to provide real behavior proof that automation cannot supply for them.

Security
Cleared: The diff only changes local memory-wiki filesystem pruning logic and tests; it adds no dependency, workflow, secret, install, or supply-chain surface.

Review findings

  • [P1] Scope the prune guard per unsafe-local path — extensions/memory-wiki/src/unsafe-local.ts:257-258
Review details

Best possible solution:

Track scan availability per configured unsafe-local root and prune only entries for roots that were successfully scanned.

Do we have a high-confidence way to reproduce the issue?

Yes, source-backed. The linked issue includes a current-main production-function repro, and current main still drops missing configured paths from activeKeys before the shared prune deletes inactive entries.

Is this the best way to solve the issue?

No. The PR is a plausible single-path mitigation, but the best fix is path-scoped pruning so mixed configured paths do not still lose pages or retain stale ones incorrectly.

Full review comments:

  • [P1] Scope the prune guard per unsafe-local path — extensions/memory-wiki/src/unsafe-local.ts:257-258
    This guard only skips pruning when the total artifact set is empty. With two configured paths, if path A is unavailable but path B still has any artifact, artifacts.length is nonzero, so the PR still calls pruneImportedSourceEntries with only B's active keys and deletes A's imported pages and human notes; if B is intentionally emptied while A is inaccessible, the same all-or-nothing guard can retain B's stale pages. Track which configured paths were actually scanned and prune only entries for available scopes.
    Confidence: 0.92

Overall correctness: patch is incorrect
Overall confidence: 0.9

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 27c1685f106d.

Label changes

Label justifications:

  • P0: The linked bug can permanently delete imported memory-wiki pages and user-authored human-notes blocks during routine storage outages.
  • merge-risk: 🚨 other: Merging the current all-or-nothing guard can leave mixed-path unsafe-local pruning wrong in ways normal green CI would not settle.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🧂 unranked krab.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body provides extension test output only; it does not include redacted live terminal output, logs, or another real unsafe-local import workflow proof after the fix. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed

PR surface:

Source +22, Tests +45. Total +67 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 28 6 +22
Tests 1 45 0 +45
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 73 6 +67

What I checked:

  • PR guard is total-artifact scoped: On the PR head, pruning is skipped only when artifacts.length === 0; if another configured path contributes any artifact, the unavailable path's entries still fall through to the destructive prune. (extensions/memory-wiki/src/unsafe-local.ts:257, 8f84ed3e6f8c)
  • Multiple configured unsafe-local paths are supported: The resolved memory-wiki config stores unsafeLocal.paths as a string[], so prune correctness has to be per configured source scope, not only for a single path. (extensions/memory-wiki/src/config.ts:80, 27c1685f106d)
  • Current prune deletes every inactive group entry: pruneImportedSourceEntries iterates all state entries in the group and removes any sync key absent from activeKeys, which deletes entries for an unavailable configured path when another path still produced active keys. (extensions/memory-wiki/src/source-sync-state.ts:261, 27c1685f106d)
  • New regression is single-path only: The added test config uses paths: [sourceDir], so it does not cover one unavailable configured path alongside another active or intentionally emptied path. (extensions/memory-wiki/src/unsafe-local.test.ts:155, 8f84ed3e6f8c)
  • Current main still has the reported unsafe-local prune path: Current main collects unsafe-local artifacts, builds activeKeys, then unconditionally calls pruneImportedSourceEntries for the whole unsafe-local group. (extensions/memory-wiki/src/unsafe-local.ts:217, 27c1685f106d)
  • Canonical linked issue remains open: The linked issue is open and labels the current-main repro as data loss; related searches found this PR as the only open implementation candidate for that exact unsafe-local transient-path problem.

Likely related people:

  • vincentkoc: GitHub history shows this person added unsafe-local source sync and then the imported-source incremental prune state used by unsafe-local and bridge sync. (role: introduced behavior; confidence: high; commits: 7b62fcd87da8, a78c4de737c5; files: extensions/memory-wiki/src/unsafe-local.ts, extensions/memory-wiki/src/unsafe-local.test.ts, extensions/memory-wiki/src/source-sync-state.ts)
  • steipete: GitHub history shows recent work on memory-wiki source sync storage and shared imported-source write paths adjacent to this prune/state behavior. (role: recent area contributor; confidence: high; commits: a4236bd6faf6, efd9aaea3f3e; files: extensions/memory-wiki/src/source-sync-state.ts, extensions/memory-wiki/src/source-page-shared.ts, extensions/memory-wiki/src/unsafe-local.ts)
  • yetval: This person authored the merged human-notes preservation fix that this prune-delete path bypasses when imported pages are removed before re-ingest. (role: recent adjacent contributor; confidence: medium; commits: 0ec12df24512; files: extensions/memory-wiki/src/source-page-shared.ts, extensions/memory-wiki/src/source-page-shared.test.ts, extensions/memory-wiki/src/markdown.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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 keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P0 Emergency: data loss, security bypass, crash loop, or unusable core runtime. merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. labels Jun 28, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jun 28, 2026
Closed
@yetval yetval mentioned this pull request Jul 1, 2026
Closed
@zhangqueping zhangqueping mentioned this pull request Jul 1, 2026
Closed
@liuhao1024

liuhao1024 commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

Auto-cleanup: freeing PR slots for higher-quality contributions. Feel free to reopen if still relevant.

@liuhao1024 liuhao1024 closed this Jul 2, 2026
@yetval yetval mentioned this pull request Jul 8, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

extensions: memory-wiki merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. P0 Emergency: data loss, security bypass, crash loop, or unusable core runtime. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: S status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@liuhao1024