Skip to content

fix(update): fail safely on Git index locks - #63041

Open
supplefrog wants to merge 1 commit into
NousResearch:mainfrom
supplefrog:fix/63038-stale-update-lock
Open

fix(update): fail safely on Git index locks#63041
supplefrog wants to merge 1 commit into
NousResearch:mainfrom
supplefrog:fix/63038-stale-update-lock

Conversation

@supplefrog

@supplefrog supplefrog commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Makes hermes update fail safely when Git's index.lock exists. The updater no longer treats lock age as proof of ownership and never deletes the lock automatically. It exits before backup or Git mutation, prints the exact lock path, and gives a platform-specific recovery command for use only after the user confirms no Git operation is running.

The guard resolves both normal .git/ directories and linked-worktree .git pointer files.

Related Issue

Refs #3523. The narrower report #63038 was closed in favor of the existing updater issue.

Relationship to existing #3523 PRs

This PR adds the missing default-checkout preflight. It follows the stronger fail-closed lesson from prior update work: preserve every lock, abort before backup/Git mutation, and provide exact recovery guidance rather than guessing ownership from mtime.

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/update_cmd.py: resolve the real index-lock path for normal checkouts and linked worktrees; abort before backup/Git mutation without deleting the lock.
  • hermes_cli/main.py: re-export the moved helpers through the existing compatibility surface.
  • tests/hermes_cli/test_update_stale_index_lock.py: cover fresh and old locks, missing locks, linked worktrees, Windows recovery guidance, and the no-mutation call order.
  • Existing later-stage update-guard harnesses: stub the new earlier preflight so they continue testing their own guard boundaries.

How to Test

  1. python -m pytest -q tests/hermes_cli/test_update_orphan_backend_reap.py tests/hermes_cli/test_update_venv_health.py tests/hermes_cli/test_update_stale_index_lock.py — 27 passed.
  2. uv.exe tool run ruff check hermes_cli/main.py hermes_cli/update_cmd.py tests/hermes_cli/test_update_stale_index_lock.py tests/hermes_cli/test_update_venv_health.py tests/hermes_cli/test_update_orphan_backend_reap.py — passed.
  3. python scripts/check-windows-footguns.py --all — no Windows footguns in 964 files.

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: Windows 10

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 — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Screenshots / Logs

Not applicable; regression coverage exercises the CLI output and preserves the lock file.

@dplush

dplush commented Jul 12, 2026

Copy link
Copy Markdown

Review note: this currently treats lock age as ownership. A legitimate Git operation can hold index.lock for more than one hour (slow network/filesystem or a long-running operation), and this would unlink it before the updater starts.

The issue calls for confirming that no live Git process owns the repository. Could the guard establish that condition (with regression coverage), or otherwise fail early with a precise recovery hint rather than deleting based on mtime alone?

@alt-glitch alt-glitch added type/bug Something isn't working comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists labels Jul 12, 2026
@supplefrog
supplefrog force-pushed the fix/63038-stale-update-lock branch from 5ff77c8 to 021ab3b Compare July 13, 2026 14:59
@supplefrog supplefrog changed the title fix(update): clear orphaned Git index locks fix(update): fail safely on Git index locks Jul 13, 2026
@supplefrog

Copy link
Copy Markdown
Contributor Author

Agreed. I removed the mtime-based deletion entirely. The updated guard now preserves every index.lock, resolves both normal checkouts and linked worktrees, exits before backup or Git mutation, and prints the exact lock path plus a platform-specific recovery command for use only after confirming no Git operation is running. Regression coverage now includes fresh and two-hour-old locks, linked-worktree locks, and the no-backup/no-Git-mutation call order (6 passed).

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for revising this to preserve every lock rather than infer ownership from mtime. Current main's updater has no index.lock preflight: _cmd_update_impl starts the backup at hermes_cli/main.py:9581, then reaches Git configuration/cleanup/fetch paths at hermes_cli/main.py:9633, :9660, and :9689. The proposed guard is placed before the backup and those operations, handles both .git directories and worktree pointer files, and the tests cover fresh/old locks, linked worktrees, Windows guidance, and call ordering.

No correctness issue was identified in the reviewed diff. GitHub currently reports the PR mergeable against main.

Automated hermes-sweeper review.

@alt-glitch alt-glitch added the sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades label Jul 15, 2026
@teknium1 teknium1 added sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 15, 2026
@teknium1 teknium1 added the area/install-update Installer, updater, packaging, wheels, doctor label Jul 19, 2026
@supplefrog
supplefrog force-pushed the fix/63038-stale-update-lock branch 2 times, most recently from 48ad330 to 51a8b22 Compare August 15, 2026 02:40
@supplefrog
supplefrog force-pushed the fix/63038-stale-update-lock branch from 51a8b22 to 93214ec Compare August 16, 2026 06:32
@supplefrog

Copy link
Copy Markdown
Contributor Author

Rebased onto current main (411903b6f) and preserved the fail-closed index.lock preflight through the update-pipeline decomposition. Current verification: 27 passed; Windows-footgun scan passed for all 5 changed files; git diff --check passed. New head: 93214ec93.

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

Labels

area/install-update Installer, updater, packaging, wheels, doctor comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants