Skip to content

fix(backup): guard os.chmod on Windows where it is a no-op - #764

Open
hashbender wants to merge 1 commit into
mainfrom
mirror/pr-56946
Open

fix(backup): guard os.chmod on Windows where it is a no-op#764
hashbender wants to merge 1 commit into
mainfrom
mirror/pr-56946

Conversation

@hashbender

Copy link
Copy Markdown
Owner

What does this PR do?

Fixes the backup import test failure on Windows where os.chmod(target, 0o600) is a silent no-op. On Windows, os.chmod with Unix permission bits succeeds without error but doesn't change anything — the file stays at default 0o666 permissions.

Extracts a _tighten_file_permissions() helper that skips the chmod on Windows (NTFS ACLs already restrict newly-created files to the owner / SYSTEM / Administrators), and makes the test assertion platform-aware.

Related Issue

Fixes NousResearch#56923

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • hermes_cli/backup.py: Added _tighten_file_permissions() helper that wraps os.chmod(path, 0o600) with a sys.platform == "win32" guard. Replaced both bare os.chmod calls (line 604 and 648) with the helper.
  • tests/hermes_cli/test_backup.py: Added import sys and wrapped the st_mode & 0o777 == 0o600 assertion in if sys.platform != "win32": so the test passes on both POSIX and Windows.

How to Test

  1. On macOS/Linux: pytest tests/hermes_cli/test_backup.py::TestMemoryProviderExternalPaths::test_import_restores_external_to_home_relative_location -xvs — should pass
  2. On Windows: same test should now pass (previously failed with assert (33206 & 511) == 384)
  3. Full backup test suite: pytest tests/hermes_cli/test_backup.py -q — all 145 tests should pass (verified on macOS)

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 26.4.1

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Screenshots / Logs

Before (Windows):

>       assert (restored.stat().st_mode & 0o777) == 0o600
E       AssertionError: assert (33206 & 511) == 384

After (macOS/Linux):

tests/hermes_cli/test_backup.py::TestMemoryProviderExternalPaths::test_import_restores_external_to_home_relative_location PASSED

Mirror-of: NousResearch#56946
NousResearch#56946

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Windows] backup import test fails: os.chmod(..., 0o600) is a no-op on Windows

1 participant