Skip to content

fix(skills): use bundle_content_hash in lock to prevent perpetual update_available - #41199

Open
liuhao1024 wants to merge 2 commits into
NousResearch:mainfrom
liuhao1024:fix-skills-update-content-hash
Open

fix(skills): use bundle_content_hash in lock to prevent perpetual update_available#41199
liuhao1024 wants to merge 2 commits into
NousResearch:mainfrom
liuhao1024:fix-skills-update-content-hash

Conversation

@liuhao1024

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes a bug where hermes skills update reinstalls files correctly but leaves a stale content_hash in .hub/lock.json, causing hermes skills check to perpetually report update_available even though the installed content is byte-for-byte identical to the latest bundle.

The root cause: install_from_quarantine() stored content_hash(install_dir) in the lock, but check_for_skill_updates() compares against bundle_content_hash(bundle). These two hash routines can diverge when disk-only files (e.g. .DS_Store) are present or when write_text/read_bytes round-trip encoding differs from the in-memory bundle content.

Fix: store bundle_content_hash(bundle) in the lock so the comparison is guaranteed to match after installation.

Related Issue

Fixes #41176

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • tools/skills_hub.py: Changed install_from_quarantine() to use bundle_content_hash(bundle) instead of content_hash(install_dir) when recording the lock entry and audit log. This ensures the stored hash is always consistent with what check_for_skill_updates() computes.
  • tests/tools/test_skills_hub.py: Added test_install_from_quarantine_records_bundle_hash_in_lock regression test that verifies the lock hash matches bundle_content_hash(bundle) after installation.

How to Test

  1. Install a hub skill whose lock content_hash is stale (predates current bundle hash)
  2. Run hermes skills check → skill shows update_available
  3. Run hermes skills update <name> → reports Updated 1 skill(s).
  4. Run hermes skills check again → skill now shows up_to_date (previously it would still show update_available)
  5. Run the regression test: pytest tests/tools/test_skills_hub.py::TestInstallPathSafety::test_install_from_quarantine_records_bundle_hash_in_lock -v

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

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

Code Intelligence

  • Analyzed: install_from_quarantine (callers: 3 — do_install, do_snapshot_import, web API; flows: 4)
  • Blast radius: LOW — single function change, all callers benefit from consistent hash
  • Related patterns: bundle_content_hash and content_hash are documented as needing to stay symmetric (line 772–775 in tools/skills_guard.py). This fix ensures we always use the bundle-side hash in the lock, eliminating the divergence path entirely.

…ate_available

install_from_quarantine() stored content_hash(install_dir) in the lock,
but check_for_skill_updates() compares against bundle_content_hash(bundle).
The two hash routines can diverge when disk-only files (e.g. .DS_Store)
are present or when write_text/read_bytes round-trip encoding differs from
the in-memory bundle content.

This caused hermes skills update to reinstall files correctly but leave a
stale content_hash in lock.json, so check_for_skill_updates() kept
reporting update_available forever (issue NousResearch#41176).

Fix: store bundle_content_hash(bundle) in the lock so the comparison is
guaranteed to match after installation.  Includes regression test.
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for targeting the lock/hash comparison boundary. The write-side normalization remains needed on current main: install_from_quarantine() stores content_hash(install_dir) at tools/skills_hub.py:3643, while checks compare it with bundle_content_hash(bundle) at tools/skills_hub.py:3753-3755.

Problems

  • The added test does not exercise the claimed divergent case. Its disk fixture exactly mirrors the bundle, and current main already proves that normal shape equal at tests/tools/test_skills_hub.py:1150-1169; the pre-change implementation would pass this test.

Suggested changes

  • Port the narrow production change to current tools/skills_hub.py:3635-3654, preserving current _skills_dir() and scan_provenance handling.
  • Add an end-to-end fixture that actually diverges under the disk and bundle hash paths, then assert a subsequent check_for_skill_updates() reports up_to_date.

Automated hermes-sweeper review.

@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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 tool/skills Skills system (list, view, manage) type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: hermes skills update doesn't refresh lock content_hash, causing permanent false 'update_available'

3 participants