Skip to content

fix(cli): delete profile directories with read-only file recovery - #43377

Open
zhao0112 wants to merge 1 commit into
NousResearch:mainfrom
zhao0112:chore/fix-profile-delete-readonly
Open

fix(cli): delete profile directories with read-only file recovery#43377
zhao0112 wants to merge 1 commit into
NousResearch:mainfrom
zhao0112:chore/fix-profile-delete-readonly

Conversation

@zhao0112

Copy link
Copy Markdown
Contributor

Summary

  • Make hermes profile delete robust against read-only/immutable files by using a shutil.rmtree onerror handler.
  • Added _make_writable in hermes_cli/profiles.py to clear filesystem-level write restrictions (chmod + best-effort chflags) and retry deletion.
  • Added regression test proving profile deletion succeeds even when a delete fails first with a PermissionError.

Testing

  • python3 -m py_compile hermes_cli/profiles.py tests/hermes_cli/test_profiles.py
  • python3 -m pytest -o addopts='' -q tests/hermes_cli/test_profiles.py

Closes #43339

@alt-glitch alt-glitch added type/bug Something isn't working comp/cli CLI entry point, hermes_cli/, setup wizard P3 Low — cosmetic, nice to have duplicate This issue or pull request already exists labels Jun 10, 2026
@alt-glitch

alt-glitch commented Jun 10, 2026

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related to open #43340 and merged #57333. Current main now has the broader profile-delete recovery path (profile-bound backend cleanup plus retrying removal), so this stale branch is not a duplicate. Please rebase or salvage any remaining immutable-file handling.

@OutThisLife

Copy link
Copy Markdown
Collaborator

Heads up on overlap with main: the core of this PR — retrying rmtree after chmod-ing read-only files — is already in delete_profile's _make_writable handler on main (it adds S_IWUSR to the path and its parent dir, then retries). So the read-only recovery this targets is covered.

The one genuinely novel bit here is the chflags step (clearing BSD/macOS uchg-style immutable flags), which main does not do. That's a niche-but-real addition, so I'm leaving this open rather than closing it as superseded — if you want to keep it, please rebase onto main and trim it down to just the chflags delta layered on top of the existing _make_writable (dropping the duplicate chmod/retry that's already there). That'd make it a tight, reviewable add.

For context: the headline #47368 symptom (profile reappearing) is fixed by #49435 (recreation guard, merging), and the 3.11 onexc crash is already on main — so this PR's remaining value is specifically the immutable-flag case.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 14, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for addressing a real deletion-recovery edge case. Current main already covers the read-only chmod/retry portion, but the immutable-flag delta remains useful.

Problems

  • Current main's delete_profile uses _rmtree_with_retry at hermes_cli/profiles.py:1433-1460 and a dual-signature handler at hermes_cli/profiles.py:1555-1592. The submitted direct onerror call would not preserve the current retry loop or Python 3.12 onexc path.
  • The added test simulates os.unlink failure but never supplies or asserts os.chflags, so it does not cover the proposed immutable-flag behavior.

Suggested changes

  • Retain current main's deletion flow and layer only the best-effort chflags clear into its existing handler before chmod.
  • Add a mocked os.chflags assertion alongside the forced deletion failure.

Automated hermes-sweeper review.

@alt-glitch alt-glitch added needs-decision Awaiting maintainer decision before any implementation and removed sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades duplicate This issue or pull request already exists labels Jul 17, 2026
@teknium1 teknium1 added the area/profiles Multi-profile isolation, HERMES_HOME scoping label Jul 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/profiles Multi-profile isolation, HERMES_HOME scoping comp/cli CLI entry point, hermes_cli/, setup wizard needs-decision Awaiting maintainer decision before any implementation P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Desktop] Profile deletion fails when .env has macOS immutable flag

4 participants