fix: atomic file writes on Windows-mounted Docker volumes#53965
fix: atomic file writes on Windows-mounted Docker volumes#53965jhawpetoss6-collab wants to merge 1 commit intoopenclaw:mainfrom
Conversation
Greptile SummaryThis PR intends to fix
Confidence Score: 1/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/shared/fs-atomic-fix.ts
Line: 1-19
Comment:
**Fix is orphaned — crash still occurs**
`writeTextFileAtomicSync` is defined here but never imported or called anywhere in the codebase. The function that actually triggers the crash is `writeTextFileAtomic` in `src/secrets/shared.ts` (line 62), which still contains the unprotected `fs.chmodSync` call:
```typescript
// src/secrets/shared.ts – still unpatched
fs.chmodSync(tempPath, mode);
```
All callers (e.g. `src/secrets/apply.ts` line 829, `restoreFileSnapshot` at line 737) go through that existing function. Until `src/secrets/shared.ts` is updated, issue #53947 will continue to reproduce on every write.
The fix should be applied directly to `writeTextFileAtomic` in `src/secrets/shared.ts`:
```typescript
export function writeTextFileAtomic(pathname: string, value: string, mode = 0o600): void {
ensureDirForFile(pathname);
const tempPath = `${pathname}.tmp-${process.pid}-${Date.now()}`;
fs.writeFileSync(tempPath, value, "utf8");
try {
fs.chmodSync(tempPath, mode);
} catch {
// EPERM expected on NTFS/CIFS mounts (Windows Docker Desktop)
}
fs.renameSync(tempPath, pathname);
}
```
`writeJsonFileSecure` (line 48 of `src/secrets/shared.ts`) has the same unguarded `chmodSync` and should also be patched.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/shared/fs-atomic-fix.ts
Line: 8-9
Comment:
**Missing `ensureDirForFile` before write**
The existing `writeTextFileAtomic` in `src/secrets/shared.ts` calls `ensureDirForFile(pathname)` before creating the temp file (line 59), ensuring the parent directory exists. This new implementation skips that step and will throw `ENOENT` if the destination directory doesn't already exist — a regression compared to the function it intends to replace.
```suggestion
const tempPath = `${pathname}.tmp-${process.pid}-${Date.now()}`;
fs.mkdirSync(require("path").dirname(pathname), { recursive: true, mode: 0o700 });
fs.writeFileSync(tempPath, value, "utf8");
```
Or import and call the shared `ensureDirForFile` helper if this file is kept.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/shared/fs-atomic-fix.ts
Line: 1
Comment:
**Node built-in import convention**
The rest of the codebase uses the `node:` protocol prefix for built-in modules (e.g. `import fs from "node:fs"` in `src/secrets/shared.ts`, `src/secrets/apply.ts`, etc.). The bare `"fs"` import here is inconsistent.
```suggestion
import fs from "node:fs";
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix: handle EPERM on chmod during atomic..." | Re-trigger Greptile |
| import fs from "fs"; | ||
|
|
||
| /** | ||
| * Handles EPERM issues during chmod on Windows-mounted Docker volumes. | ||
| * Addresses #53947. | ||
| */ | ||
| export function writeTextFileAtomicSync(pathname: string, value: string, mode: number = 0o600) { | ||
| const tempPath = `${pathname}.tmp-${process.pid}-${Date.now()}`; | ||
| fs.writeFileSync(tempPath, value, "utf8"); | ||
|
|
||
| try { | ||
| // This fails on Windows-mounted volumes in Docker | ||
| fs.chmodSync(tempPath, mode); | ||
| } catch (e) { | ||
| // Silently continue if chmod fails (EPERM expected on NTFS/CIFS mounts) | ||
| } | ||
|
|
||
| fs.renameSync(tempPath, pathname); | ||
| } |
There was a problem hiding this comment.
writeTextFileAtomicSync is defined here but never imported or called anywhere in the codebase. The function that actually triggers the crash is writeTextFileAtomic in src/secrets/shared.ts (line 62), which still contains the unprotected fs.chmodSync call:
// src/secrets/shared.ts – still unpatched
fs.chmodSync(tempPath, mode);All callers (e.g. src/secrets/apply.ts line 829, restoreFileSnapshot at line 737) go through that existing function. Until src/secrets/shared.ts is updated, issue #53947 will continue to reproduce on every write.
The fix should be applied directly to writeTextFileAtomic in src/secrets/shared.ts:
export function writeTextFileAtomic(pathname: string, value: string, mode = 0o600): void {
ensureDirForFile(pathname);
const tempPath = `${pathname}.tmp-${process.pid}-${Date.now()}`;
fs.writeFileSync(tempPath, value, "utf8");
try {
fs.chmodSync(tempPath, mode);
} catch {
// EPERM expected on NTFS/CIFS mounts (Windows Docker Desktop)
}
fs.renameSync(tempPath, pathname);
}writeJsonFileSecure (line 48 of src/secrets/shared.ts) has the same unguarded chmodSync and should also be patched.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/shared/fs-atomic-fix.ts
Line: 1-19
Comment:
**Fix is orphaned — crash still occurs**
`writeTextFileAtomicSync` is defined here but never imported or called anywhere in the codebase. The function that actually triggers the crash is `writeTextFileAtomic` in `src/secrets/shared.ts` (line 62), which still contains the unprotected `fs.chmodSync` call:
```typescript
// src/secrets/shared.ts – still unpatched
fs.chmodSync(tempPath, mode);
```
All callers (e.g. `src/secrets/apply.ts` line 829, `restoreFileSnapshot` at line 737) go through that existing function. Until `src/secrets/shared.ts` is updated, issue #53947 will continue to reproduce on every write.
The fix should be applied directly to `writeTextFileAtomic` in `src/secrets/shared.ts`:
```typescript
export function writeTextFileAtomic(pathname: string, value: string, mode = 0o600): void {
ensureDirForFile(pathname);
const tempPath = `${pathname}.tmp-${process.pid}-${Date.now()}`;
fs.writeFileSync(tempPath, value, "utf8");
try {
fs.chmodSync(tempPath, mode);
} catch {
// EPERM expected on NTFS/CIFS mounts (Windows Docker Desktop)
}
fs.renameSync(tempPath, pathname);
}
```
`writeJsonFileSecure` (line 48 of `src/secrets/shared.ts`) has the same unguarded `chmodSync` and should also be patched.
How can I resolve this? If you propose a fix, please make it concise.| const tempPath = `${pathname}.tmp-${process.pid}-${Date.now()}`; | ||
| fs.writeFileSync(tempPath, value, "utf8"); |
There was a problem hiding this comment.
ensureDirForFile before write
The existing writeTextFileAtomic in src/secrets/shared.ts calls ensureDirForFile(pathname) before creating the temp file (line 59), ensuring the parent directory exists. This new implementation skips that step and will throw ENOENT if the destination directory doesn't already exist — a regression compared to the function it intends to replace.
| const tempPath = `${pathname}.tmp-${process.pid}-${Date.now()}`; | |
| fs.writeFileSync(tempPath, value, "utf8"); | |
| const tempPath = `${pathname}.tmp-${process.pid}-${Date.now()}`; | |
| fs.mkdirSync(require("path").dirname(pathname), { recursive: true, mode: 0o700 }); | |
| fs.writeFileSync(tempPath, value, "utf8"); |
Or import and call the shared ensureDirForFile helper if this file is kept.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/shared/fs-atomic-fix.ts
Line: 8-9
Comment:
**Missing `ensureDirForFile` before write**
The existing `writeTextFileAtomic` in `src/secrets/shared.ts` calls `ensureDirForFile(pathname)` before creating the temp file (line 59), ensuring the parent directory exists. This new implementation skips that step and will throw `ENOENT` if the destination directory doesn't already exist — a regression compared to the function it intends to replace.
```suggestion
const tempPath = `${pathname}.tmp-${process.pid}-${Date.now()}`;
fs.mkdirSync(require("path").dirname(pathname), { recursive: true, mode: 0o700 });
fs.writeFileSync(tempPath, value, "utf8");
```
Or import and call the shared `ensureDirForFile` helper if this file is kept.
How can I resolve this? If you propose a fix, please make it concise.| @@ -0,0 +1,19 @@ | |||
| import fs from "fs"; | |||
There was a problem hiding this comment.
The rest of the codebase uses the node: protocol prefix for built-in modules (e.g. import fs from "node:fs" in src/secrets/shared.ts, src/secrets/apply.ts, etc.). The bare "fs" import here is inconsistent.
| import fs from "fs"; | |
| import fs from "node:fs"; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/shared/fs-atomic-fix.ts
Line: 1
Comment:
**Node built-in import convention**
The rest of the codebase uses the `node:` protocol prefix for built-in modules (e.g. `import fs from "node:fs"` in `src/secrets/shared.ts`, `src/secrets/apply.ts`, etc.). The bare `"fs"` import here is inconsistent.
```suggestion
import fs from "node:fs";
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc3f5681f7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| * Handles EPERM issues during chmod on Windows-mounted Docker volumes. | ||
| * Addresses #53947. | ||
| */ | ||
| export function writeTextFileAtomicSync(pathname: string, value: string, mode: number = 0o600) { |
There was a problem hiding this comment.
Wire this helper into active sync write paths
This commit adds writeTextFileAtomicSync, but it is never imported or called anywhere (repo-wide search for writeTextFileAtomicSync|fs-atomic-fix only matches this new file), so runtime behavior is unchanged. The existing synchronous writers that still do direct fs.chmodSync (for example src/secrets/shared.ts:62 and src/infra/json-file.ts:22) can still throw EPERM on Windows-mounted Docker volumes, so the crash this commit claims to fix remains.
Useful? React with 👍 / 👎.
|
This pull request has been automatically marked as stale due to inactivity. |
|
Codex review: needs changes before merge. Summary Reproducibility: yes. for the PR defect: source inspection shows the new helper has no callers, while Next step before merge Security Review findings
Review detailsBest possible solution: Replace the orphan helper with a focused change on the active secrets persistence writer, preserving parent-directory creation and credential-file permissions while adding targeted regression coverage and a changelog entry. Do we have a high-confidence way to reproduce the issue? Yes for the PR defect: source inspection shows the new helper has no callers, while Is this the best way to solve the issue? No. Adding a disconnected helper is not the narrow maintainable fix; the existing writer or a shared helper actually used by it needs the guarded behavior while preserving directory creation and permission semantics. Full review comments:
Overall correctness: patch is incorrect Security concerns:
Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 7c0f5463a564. |
|
Codex review: found issues before merge. What this changes: The PR adds Maintainer follow-up before merge: This is an open contributor PR with known blocking defects, and the correct repair touches credential/secrets persistence semantics owned by the security-sensitive path; maintainers should request changes or choose a replacement rather than queue an automated repair from this review. Review findings:
Review detailsBest possible solution: Revise this PR or replace it with a focused change on the active secrets persistence path: preserve existing parent-directory creation, handle the Windows-mounted Docker chmod/rename failure where the current writer is actually called, add targeted regression coverage, and keep credential-file permission failures visible enough for operators and secops review. Full review comments:
Overall correctness: patch is incorrect Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against e46dccb35374. |
This PR fixes a crash when OpenClaw attempts to write config or auth files while running in Docker on a Windows host (#53947).
Changes:
chmodSyncin a try/catch to handleEPERMon NTFS/CIFS mounts./claim #53947