fix(skills): make hermes skills reset honest when local copy differs from bundled#29863
Open
briandevans wants to merge 1 commit into
Open
fix(skills): make hermes skills reset honest when local copy differs from bundled#29863briandevans wants to merge 1 commit into
hermes skills reset honest when local copy differs from bundled#29863briandevans wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR improves the UX and correctness of hermes skills reset by clarifying when a plain reset can re-baseline bundled-skill tracking vs. when a user’s modified copy will remain preserved (and skipped) unless --restore is used.
Changes:
- Update collision hint messaging to recommend
hermes skills reset --restore <name>instead of looping with plainreset. - Refine
reset_bundled_skill(..., restore=False)to report whether the skill was actually re-baselined or remains preserved/skipped. - Add/adjust tests and CLI help text to reflect the corrected semantics.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tools/skills_sync.py | Adjusts reset messaging logic and collision hint to point users to --restore when needed. |
| tests/tools/test_skills_sync.py | Updates hint assertion and adds a regression test for the “modified copy remains preserved” reset behavior. |
| hermes_cli/skills_hub.py | Updates /skills reset usage guidance to match new behavior. |
| hermes_cli/main.py | Updates argparse help/description to accurately describe reset vs. restore semantics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+415
to
+430
| manifest_after = _read_manifest() | ||
| if name in manifest_after: | ||
| action = "manifest_cleared" | ||
| message = ( | ||
| f"Cleared manifest entry for '{name}'. Future `hermes update` runs " | ||
| f"will re-baseline against your current copy and accept upstream changes." | ||
| ) | ||
| else: | ||
| action = "manifest_cleared_local_preserved" | ||
| message = ( | ||
| f"Cleared manifest entry for '{name}', but your local copy differs " | ||
| f"from the bundled version, so it remains preserved and will not " | ||
| f"receive upstream updates. Use " | ||
| f"`hermes skills reset --restore {name}` to revert to the shipped " | ||
| f"version." | ||
| ) |
Comment on lines
+423
to
+430
| action = "manifest_cleared_local_preserved" | ||
| message = ( | ||
| f"Cleared manifest entry for '{name}', but your local copy differs " | ||
| f"from the bundled version, so it remains preserved and will not " | ||
| f"receive upstream updates. Use " | ||
| f"`hermes skills reset --restore {name}` to revert to the shipped " | ||
| f"version." | ||
| ) |
| f"will re-baseline against your current copy and accept upstream changes." | ||
| ) | ||
| else: | ||
| action = "manifest_cleared_local_preserved" |
9aa7293 to
2380b93
Compare
0973c62 to
24affbb
Compare
…s from bundled
`reset_bundled_skill(restore=False)` always returned the same upbeat message
("Future `hermes update` runs will re-baseline against your current copy and
accept upstream changes."), but for user-modified skills the next sync hits
the new-skill-collision branch, skips the skill, and never writes a manifest
entry — so future updates keep skipping it and printing "yours was kept. Run
`hermes skills reset {name}`", which is exactly the loop the user just took.
Fix:
- `reset_bundled_skill` now reads the manifest after the re-sync and only
reports `manifest_cleared` when the entry was actually re-baselined. When
the local copy differs from bundled, it returns a new
`manifest_cleared_local_preserved` action with a message that says the
local copy is preserved and points at `--restore` to overwrite it.
- `sync_skills` collision hint now suggests `hermes skills reset --restore`
instead of plain `reset`, since plain `reset` falls into this same branch
again for any divergent copy.
- CLI `hermes skills reset --help` and TUI `/skills reset` usage block were
describing the same misleading "stops marking it as user-modified"
framing; both are corrected to mirror the runtime behavior.
Refs NousResearch#29856.
24affbb to
30aafbd
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Fixes the misleading success message from
hermes skills reset <name>(no--restore) when the user's local copy of a bundled skill genuinely differs from the bundled version.Today, plain
resetalways returns:But the implementation only re-baselines when
dir_hash(dest) == bundled_hash. For genuinely modified skills,sync_skillsfalls into the new-skill-collision branch (tools/skills_sync.py:204), skips the skill, and refuses to poison the manifest. The manifest stays empty, the next sync prints the same "yours was kept. Runhermes skills reset {name}" message (tools/skills_sync.py:222) — and the user runs the exact same command that just told them updates would resume. That's the loop the issue describes.Mirrors the runtime resolver
reset_bundled_skill -> sync_skills: the success message now reflects which branchsync_skillsactually took, and every user-facing surface that recommendsreset(CLI--help, TUI/skills resetusage line) now points at--restorefor modified copies.Related Issue
Fixes #29856
Type of Change
Changes Made
tools/skills_sync.py—reset_bundled_skill(restore=False)now re-reads the manifest after the resync and returns a newmanifest_cleared_local_preservedaction with an honest message when the local copy differs from bundled. Thesync_skillscollision hint now suggestshermes skills reset --restore <name>instead of plainreset.hermes_cli/main.py—hermes skills reset --helptext updated to mirror the runtime: re-baseline path for byte-identical copies,--restorepath for modified copies.hermes_cli/skills_hub.py— TUI/skills resetusage block updated identically.tests/tools/test_skills_sync.py— new testtest_reset_on_genuinely_modified_skill_reports_preservedcovers the bug repro from the issue (modified copy → must report preserved-with-restore). Existingtest_collision_prints_reset_hintupdated to assert the--restoresuggestion.How to Test
main:Regression guard
Without
tools/skills_sync.pychanges, bothtest_reset_on_genuinely_modified_skill_reports_preservedand the updatedtest_collision_prints_reset_hintfail with the expected old messages, confirming each test is actually checking the fixed behavior:Checklist
Code
fix(scope):,feat(scope):, etc.)Documentation & Housekeeping
docs/, docstrings) — N/A; the runtime help text inhermes_cli/main.pyandhermes_cli/skills_hub.pyIS the user-facing doc, and both are updated.cli-config.yaml.exampleif I added/changed config keys — N/A (no config changes)CONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — N/A