Skip to content

test(cli): gate only the POSIX mode-bit assertion on Windows - #59433

Open
LceAn wants to merge 1 commit into
NousResearch:mainfrom
LceAn:patch1
Open

test(cli): gate only the POSIX mode-bit assertion on Windows#59433
LceAn wants to merge 1 commit into
NousResearch:mainfrom
LceAn:patch1

Conversation

@LceAn

@LceAn LceAn commented Jul 6, 2026

Copy link
Copy Markdown

What & why

Native Windows does not expose POSIX permission bits from os.chmod(..., 0o600), so the credential-shaped restore test failed on st_mode even though restoration was correct.

Change

Keep the test enabled on every platform and gate only the POSIX-specific 0o600 assertion. Windows still verifies the restored home-relative path, file contents, and absence of _external leakage under HERMES_HOME.

Validation

  • The focused external-restore test passes on native Windows.
  • ruff and git diff --check pass.

This follows the reviewer-requested assertion-level scope and the repository precedent in #60888.

@alt-glitch alt-glitch added type/test Test coverage or test infrastructure comp/cli CLI entry point, hermes_cli/, setup wizard platform/windows Native Windows-specific behavior or breakage sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows area/auth Authentication, OAuth, credential pools P3 Low — cosmetic, nice to have labels Jul 6, 2026

@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 the focused Windows portability fix. The premise remains valid on current main: tests/hermes_cli/test_backup.py:2471 unconditionally checks 0o600, while hermes_cli/backup.py:599-604 writes the external file and applies os.chmod.

Problems

  • The added method-level marker skips more than the POSIX-specific assertion. It also removes Windows coverage for the portable restore assertions at tests/hermes_cli/test_backup.py:2467-2473: file existence, restored content, and preventing _external from appearing under HERMES_HOME.

Suggested changes

  • Keep the test running and guard only the mode-bit assertion at tests/hermes_cli/test_backup.py:2471 on Windows. This follows the linked #60888 discussion's stronger pattern of retaining meaningful Windows behavior coverage while excluding unsupported POSIX mode checks.

Automated hermes-sweeper review.

Comment thread tests/hermes_cli/test_backup.py Outdated
@@ -2443,6 +2443,10 @@ def test_backup_skips_external_paths_outside_home(self, tmp_path, monkeypatch):
(outside / "leak.json").unlink()
outside.rmdir()

@pytest.mark.skipif(

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.

Skipping the entire method also drops Windows coverage of the portable restoration assertions below: file existence, contents, and no _external leak into HERMES_HOME. Keep the test enabled and condition only the POSIX-mode assertion.

@teknium1 teknium1 added the sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users label Jul 15, 2026
@LceAn

LceAn commented Jul 18, 2026

Copy link
Copy Markdown
Author

Updated per review. The test now stays enabled on Windows and only the 0o600 assertion is gated with if os.name != "nt"; existence, content, and no-_external-leak coverage still runs there.

Validation: scripts/run_tests.sh tests/hermes_cli/test_backup.py -q -k test_import_restores_external_to_home_relative_location → 1 passed.

@LceAn LceAn changed the title test(cli): skip 0o600 assertion on Windows (os.chmod is a no-op) test(cli): gate only the POSIX 0o600 assertion on Windows Jul 20, 2026
@LceAn

LceAn commented Jul 20, 2026

Copy link
Copy Markdown
Author

Maintenance update: rebuilt the same reviewed assertion-level fix on current main because this large test file changed upstream, then normalized it to LF and refreshed the title/body. Windows still runs the portable restore assertions; only the POSIX mode-bit check is gated. The diff is now +3/-2.

Keep the restore, content, and no-leak assertions active on Windows while gating only the POSIX mode-bit check.
@LceAn LceAn changed the title test(cli): gate only the POSIX 0o600 assertion on Windows test(cli): gate only the POSIX mode-bit assertion on Windows Jul 20, 2026
@LceAn

LceAn commented Jul 20, 2026

Copy link
Copy Markdown
Author

Second-pass maintenance: retained the reviewer-approved assertion-level gate after revalidation. Windows continues to cover restore path, content, and _external isolation; only POSIX mode bits are excluded.

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-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows type/test Test coverage or test infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants