test: skip chmod-dependent backup rotation tests on Windows#32286
test: skip chmod-dependent backup rotation tests on Windows#32286
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb2ff87f66
ℹ️ 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".
| it.skipIf(IS_WINDOWS)( | ||
| "maintainConfigBackups composes rotate/copy/harden/prune flow", |
There was a problem hiding this comment.
Keep Windows coverage for maintainConfigBackups flow
Skipping the entire maintainConfigBackups composes rotate/copy/harden/prune flow test on Windows removes validation of rotate/copy/orphan-prune behavior on that platform, even though only the mode check is chmod-dependent. This means a Windows-only regression in the backup maintenance sequence could now pass CI unnoticed; the skip should be narrowed to just the permission assertion instead of the whole test.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR correctly guards two file-permission-asserting tests with
Confidence Score: 5/5
Last reviewed commit: bb2ff87 |
| it.skipIf(IS_WINDOWS)( | ||
| "maintainConfigBackups composes rotate/copy/harden/prune flow", | ||
| async () => { | ||
| await withTempHome(async () => { | ||
| const stateDir = process.env.OPENCLAW_STATE_DIR?.trim(); | ||
| if (!stateDir) { | ||
| throw new Error("Expected OPENCLAW_STATE_DIR to be set by withTempHome"); | ||
| } | ||
| const configPath = path.join(stateDir, "openclaw.json"); | ||
| await fs.writeFile(configPath, JSON.stringify({ token: "secret" }), { mode: 0o600 }); | ||
| await fs.writeFile(`${configPath}.bak`, "previous", { mode: 0o644 }); | ||
| await fs.writeFile(`${configPath}.bak.orphan`, "old"); | ||
|
|
||
| await maintainConfigBackups(configPath, fs); | ||
|
|
||
| // A new primary backup is created from the current config. | ||
| await expect(fs.readFile(`${configPath}.bak`, "utf-8")).resolves.toBe( | ||
| JSON.stringify({ token: "secret" }), | ||
| ); | ||
| // Prior primary backup gets rotated into ring slot 1. | ||
| await expect(fs.readFile(`${configPath}.bak.1`, "utf-8")).resolves.toBe("previous"); | ||
| // Mode hardening still applies. | ||
| const primaryBackupStat = await fs.stat(`${configPath}.bak`); | ||
| expect(primaryBackupStat.mode & 0o777).toBe(0o600); | ||
| // Out-of-ring orphan gets pruned. | ||
| await expect(fs.stat(`${configPath}.bak.orphan`)).rejects.toThrow(); | ||
| }); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Entire test skipped on Windows, including non-permission assertions
The maintainConfigBackups test is skipped wholesale on Windows, but only the final permission assertion (mode & 0o777 === 0o600) is Windows-incompatible. The other three assertions — file content, rotation into ring slot 1, and orphan pruning — are all filesystem-content checks that work fine on Windows.
Skipping the full test means the compose flow (rotate → copy → harden → prune) gets zero coverage on Windows. Consider splitting the test into a platform-agnostic block for the content/rotation/pruning checks and a platform-guarded block for the permission check, or pulling the permission assertion into its own it.skipIf(IS_WINDOWS) test alongside the existing hardenBackupPermissions test.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/config/config.backup-rotation.test.ts
Line: 121-149
Comment:
**Entire test skipped on Windows, including non-permission assertions**
The `maintainConfigBackups` test is skipped wholesale on Windows, but only the final permission assertion (`mode & 0o777 === 0o600`) is Windows-incompatible. The other three assertions — file content, rotation into ring slot 1, and orphan pruning — are all filesystem-content checks that work fine on Windows.
Skipping the full test means the compose flow (`rotate → copy → harden → prune`) gets zero coverage on Windows. Consider splitting the test into a platform-agnostic block for the content/rotation/pruning checks and a platform-guarded block for the permission check, or pulling the permission assertion into its own `it.skipIf(IS_WINDOWS)` test alongside the existing `hardenBackupPermissions` test.
How can I resolve this? If you propose a fix, please make it concise.bb2ff87 to
d1aafcb
Compare
chmod is a no-op on Windows — file permissions always report 0o666 regardless of what was set, so asserting 0o600 will never pass.
d1aafcb to
54810b2
Compare
mizoz
left a comment
There was a problem hiding this comment.
LGTM ✅ - This is the correct fix. chmod is a no-op on Windows, so skipping these permission-asserting tests is the right approach. The IS_WINDOWS guard matches the pattern used elsewhere in the codebase.
chmod is a no-op on Windows — file permissions always report 0o666 regardless of what was set, so asserting 0o600 will never pass.
Adds
IS_WINDOWSguard withit.skipIfon the two permission-asserting tests (hardenBackupPermissionsandmaintainConfigBackups), matching the pattern used for other platform-specific test skips.All 4 tests still pass on macOS/Linux; the 2 chmod tests skip on Windows.