Skip to content

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

Open
liuhao1024 wants to merge 1 commit into
NousResearch:mainfrom
liuhao1024:liuhao/cron-bugfix-56923-windows-chmod-noop
Open

fix(backup): guard os.chmod on Windows where it is a no-op#56946
liuhao1024 wants to merge 1 commit into
NousResearch:mainfrom
liuhao1024:liuhao/cron-bugfix-56923-windows-chmod-noop

Conversation

@liuhao1024

Copy link
Copy Markdown
Contributor

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 #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

os.chmod(target, 0o600) silently succeeds on Windows without changing
file permissions, causing test_import_restores_external_to_home_relative_
location to fail with (33206 & 511) == 384.

Extract _tighten_file_permissions() helper that skips chmod on win32
(NTFS ACLs already restrict to owner/SYSTEM/Administrators).  Update
test assertion to be platform-aware.

Fixes NousResearch#56923
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/cli CLI entry point, hermes_cli/, setup wizard platform/windows Native Windows-specific behavior or breakage area/auth Authentication, OAuth, credential pools sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows labels Jul 2, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Competing with #56949 (@AlexFucuson9) for the same fix (#56923). This PR skips the chmod on Windows (relying on the inherited NTFS ACL that already restricts new files to owner/SYSTEM/Administrators) and makes the test platform-aware; #56949 instead actively sets a tight Windows DACL via ctypes. Both are valid — leaving open for a maintainer to pick minimal (#56946) vs. belt-and-suspenders (#56949).

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for isolating the Windows-specific mode-bit assertion; the current external restore test does assert POSIX 0600 after os.chmod at hermes_cli/backup.py:603-606, so the reported native-Windows failure remains relevant.

Problems

  • The helper's except OSError: pass changes the normal import path's established failure handling. Current hermes_cli/backup.py:647-651 lets a failed chmod reach the enclosing handler, which records an import error instead of incrementing restored; the proposed helper suppresses that failure for POSIX too.

Suggested changes

  • Keep the Windows skip, but preserve caller-specific OSError behavior: the external path may remain best-effort, while the normal secret-file path should retain its outer error reporting/accounting semantics.

Automated hermes-sweeper review.

Comment thread hermes_cli/backup.py
return
try:
os.chmod(path, 0o600)
except OSError:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This catch changes the normal import path's current behavior: run_import presently lets a chmod failure at the secret-file branch reach its enclosing handler, append an error, and avoid incrementing restored. Keep the Windows skip, but preserve that caller-specific failure handling rather than swallowing POSIX chmod errors here.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/auth Authentication, OAuth, credential pools comp/cli CLI entry point, hermes_cli/, setup wizard P3 Low — cosmetic, nice to have platform/windows Native Windows-specific behavior or breakage sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/bug Something isn't working

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

3 participants