Skip to content

refactor(memory-core): use replaceFileAtomic for dreaming file writes#96734

Closed
SunnyShu0925 wants to merge 1 commit into
openclaw:mainfrom
SunnyShu0925:fix/dreaming-markdown-atomic-write
Closed

refactor(memory-core): use replaceFileAtomic for dreaming file writes#96734
SunnyShu0925 wants to merge 1 commit into
openclaw:mainfrom
SunnyShu0925:fix/dreaming-markdown-atomic-write

Conversation

@SunnyShu0925

@SunnyShu0925 SunnyShu0925 commented Jun 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What Problem This Solves

dreaming-markdown.ts uses fs.writeFile for three dreaming write paths (inline daily, separate report, deep report). fs.writeFile is non-atomic on Linux—a process crash mid-write can leave the target file truncated or empty. The sibling module dreaming-dreams-file.ts already avoids this by using replaceFileAtomic from openclaw/plugin-sdk/security-runtime.

This PR aligns the two modules so all dreaming file writes in memory-core use the same safe atomic write pattern.

Why This Change Was Made

replaceFileAtomic writes to a temp file then renames atomically, so a crash during write never corrupts the target file. The same tool and pattern is already used in dreaming-dreams-file.ts—this is a consistency refactor that also brings the crash-safety benefit to the remaining write paths.

No new dependencies or behavioral changes. Temp cleanup is handled by replaceFileAtomic internally.

User Impact

None visible. Dreaming output is identical. The only difference is that a process crash during a dreaming phase write will no longer risk truncating daily memory or dreaming report files.

Evidence

  • 9 existing tests pass unchanged
  • The same replaceFileAtomic import and usage pattern already exists in extensions/memory-core/src/dreaming-dreams-file.ts:84-91
  • Single file change: +23 -3 lines in extensions/memory-core/src/dreaming-markdown.ts

@SunnyShu0925
refactor(memory-core): use replaceFileAtomic for dreaming file writes
2fc40b3 
Align with the sibling dreaming-dreams-file.ts which already uses
replaceFileAtomic from openclaw/plugin-sdk/security-runtime.

Replaces three fs.writeFile calls (inline daily, separate report, deep
report) with replaceFileAtomic for consistent file I/O patterns within
the dreaming module.

Test: existing 9 tests pass unchanged, no behavioral change.
@clawsweeper

clawsweeper Bot commented Jun 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 25, 2026, 6:08 AM ET / 10:08 UTC.

Summary
The PR replaces three memory-core dreaming markdown fs.writeFile paths with replaceFileAtomic from the plugin SDK.

PR surface: Source +20. Total +20 across 1 file.

Reproducibility: yes. for the PR defect by source inspection: the PR calls replaceFileAtomic, and the @openclaw/fs-safe v0.3.0 source shows default 0700 directory and 0600 file chmod behavior. I did not run a live Dreaming scenario in this read-only review.

Review metrics: 1 noteworthy metric.

  • Permission-affecting write replacements: 3 write paths changed. These paths write documented human-readable memory outputs, so maintainers need to confirm permission compatibility before merge.

Stored data model
Persistent data-model change detected: serialized state: extensions/memory-core/src/dreaming-markdown.ts, vector/embedding metadata: extensions/memory-core/src/dreaming-markdown.ts. Confirm migration or upgrade compatibility proof before merge.

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

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

Rank-up moves:

  • Preserve current file and parent directory permissions, or document and test a maintainer-approved private permission contract.
  • [P1] Add redacted real OpenClaw Dreaming/file-write proof in the PR body so ClawSweeper can re-review.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body only cites existing tests and does not show redacted terminal output, logs, a recording, or linked artifact from a real Dreaming/file-write run; private paths, endpoints, keys, and phone numbers should be redacted before posting proof, and updating the PR body should trigger re-review. 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] Merging as-is can chmod memory/ or memory/dreaming/... directories to 0700 and create new markdown outputs as 0600, which can break group-shared workspaces or artifact readers that rely on current umask-based permissions.
  • [P1] The PR body only cites existing tests and does not show a redacted real OpenClaw Dreaming or file-write run after the patch.

Maintainer options:

  1. Preserve Artifact Permissions (recommended)
    Keep the atomic replacement but pass options or add a small wrapper so existing parent directory and markdown output permissions remain compatible with current fs.writeFile behavior.
  2. Accept Private Memory Artifacts
    Maintainers may intentionally accept 0700 directories and 0600 generated reports, but that should be recorded as a user-visible compatibility decision with focused tests before merge.
  3. Defer To Broader Hardening
    If maintainers want to handle permissions together with the broader memory deletion hardening work, pause this PR and continue review in fix(memory-core): harden dreaming daily-file writes and drain dangling recall refs #94537.

Next step before merge

  • [P1] Manual follow-up is needed because contributor real-behavior proof and the permission compatibility decision cannot be supplied by automation.

Security
Needs attention: No new dependency or execution surface was added, but the diff changes local filesystem permission behavior for memory artifacts and needs explicit handling.

Review findings

  • [P1] Preserve memory artifact permissions — extensions/memory-core/src/dreaming-markdown.ts:92-98
Review details

Best possible solution:

Keep atomic replacement, but preserve existing file and directory permission behavior unless maintainers explicitly approve and test a private artifact contract, then require redacted real run proof.

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

Yes for the PR defect by source inspection: the PR calls replaceFileAtomic, and the @openclaw/fs-safe v0.3.0 source shows default 0700 directory and 0600 file chmod behavior. I did not run a live Dreaming scenario in this read-only review.

Is this the best way to solve the issue?

No. replaceFileAtomic is the right primitive, but this is not the best merge shape until it preserves current artifact permissions or explicitly documents and tests a maintainer-approved permission change.

Full review comments:

  • [P1] Preserve memory artifact permissions — extensions/memory-core/src/dreaming-markdown.ts:92-98
    These replaceFileAtomic calls rely on helper defaults that chmod the destination directory to 0700 and create new files as 0600. The touched paths are documented human-readable memory artifacts, while current fs.writeFile/fs.mkdir behavior follows the process umask, so an upgrade can break group-readable workspaces or artifact readers. Preserve the current parent/file modes or make the private-mode change explicit with tests and maintainer approval.
    Confidence: 0.86

Overall correctness: patch is incorrect
Overall confidence: 0.86

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 4ecb45bf7729.

Label changes

Label justifications:

  • P2: This is a normal-priority memory-core hardening PR with a concrete compatibility blocker and limited runtime blast radius.
  • merge-risk: 🚨 compatibility: The diff can change permissions on existing memory artifact directories and new markdown reports during upgrade.
  • 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 only cites existing tests and does not show redacted terminal output, logs, a recording, or linked artifact from a real Dreaming/file-write run; private paths, endpoints, keys, and phone numbers should be redacted before posting proof, and updating the PR body should trigger re-review. 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 +20. Total +20 across 1 file.

View PR surface stats
Area Files Added Removed Net
Source 1 23 3 +20
Tests 0 0 0 0
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 1 23 3 +20

Security concerns:

  • [medium] Implicit private-mode chmod on memory artifacts — extensions/memory-core/src/dreaming-markdown.ts:92
    The atomic helper defaults chmod parent directories and destination files; applying those defaults to human-readable memory outputs is security-sensitive and compatibility-sensitive unless it is an intentional hardened-permission contract.
    Confidence: 0.86

What I checked:

  • PR diff changes three dreaming markdown writes: The PR replaces the inline daily write, light/REM separate report write, and deep separate report write with replaceFileAtomic calls, including explicit mode: 0o600 and no explicit dirMode. (extensions/memory-core/src/dreaming-markdown.ts:92, 2fc40b37edaa)
  • Current main uses default write and mkdir permissions: Current main creates the parent directories with default fs.mkdir behavior and writes the daily/report markdown outputs with fs.writeFile, so file and directory modes follow the process umask rather than a hard-coded private mode. (extensions/memory-core/src/dreaming-markdown.ts:91, 4ecb45bf7729)
  • Sibling DREAMS.md path preserves existing mode: The adjacent DREAMS.md helper already uses replaceFileAtomic, but it sets preserveExistingMode: true, which supports the primitive choice while showing permission preservation matters for this area. (extensions/memory-core/src/dreaming-dreams-file.ts:84, 4ecb45bf7729)
  • Dependency contract changes modes by default: @openclaw/fs-safe v0.3.0 defaults dirMode to 0o700, defaults file mode to 0o600, calls mkdir, chmods the parent directory, writes the temp file with that mode, and chmods the destination after rename. (@openclaw/fs-safe/src/replace-file.ts:383, cb91a5e42b30)
  • Dreaming outputs are documented as human-readable artifacts: The docs classify DREAMS.md and optional phase report files under memory/dreaming/<phase>/YYYY-MM-DD.md as human-readable output, not hidden private machine state. Public docs: docs/concepts/dreaming.md. (docs/concepts/dreaming.md:21, 4ecb45bf7729)
  • Overlapping hardening PR is not a safe superseder: The related broader hardening PR remains open with its own proof and session-state blockers, so it is overlapping context rather than a merged or clean canonical replacement for this PR. (0214b37c748f)

Likely related people:

  • vincentkoc: Live commit history shows recent changes to dreaming-markdown.ts, including deep sleep summary work and earlier dreaming trail behavior. (role: recent dreaming-markdown contributor; confidence: high; commits: 329fa44d23f4, 5716d83336fd, a9dbaa112465; files: extensions/memory-core/src/dreaming-markdown.ts)
  • jalehman: Live commit history shows very recent memory-core dreaming pipeline work in adjacent caller paths that feed this write helper. (role: recent memory dreaming area contributor; confidence: medium; commits: da50a450d271, c24d266b2d09; files: extensions/memory-core/src/dreaming-phases.ts)
  • steipete: History shows adjacent filesystem-safety extraction and memory-core state work relevant to the permission semantics of replaceFileAtomic. (role: adjacent fs-safe and memory-core contributor; confidence: medium; commits: 538605ff44d2, 89c90210fb90, 9c08d8cd3514; files: src/infra/replace-file.ts, extensions/memory-core/src/dreaming-markdown.ts, extensions/memory-core/src/dreaming-phases.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. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. labels Jun 25, 2026
@SunnyShu0925

SunnyShu0925 commented Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

Closing this PR as it is a subset of #94537 which has a more complete implementation (correct preserveExistingMode on all write paths, additional tests, and real behavior proof). Consolidating efforts on #94537.

@clawsweeper clawsweeper Bot mentioned this pull request Jun 25, 2026
Closed
23 tasks
@SunnyShu0925
SunnyShu0925 deleted the fix/dreaming-markdown-atomic-write branch July 2, 2026 03:35
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-core Extension: memory-core merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. P2 Normal backlog priority with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: XS 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

@SunnyShu0925