Skip to content

refactor(skills): dedupe file-listing + share user-modified predicate (follow-up to #48286) - #48293

Merged
kshitijk4poor merged 1 commit into
NousResearch:mainfrom
kshitijk4poor:chore/skills-diff-cleanup
Jun 18, 2026
Merged

refactor(skills): dedupe file-listing + share user-modified predicate (follow-up to #48286)#48293
kshitijk4poor merged 1 commit into
NousResearch:mainfrom
kshitijk4poor:chore/skills-diff-cleanup

Conversation

@kshitijk4poor

Copy link
Copy Markdown
Collaborator

Summary

Cleanup follow-up to #48286 (hermes skills list-modified / diff), which merged before these refactors landed. All changes are behavior-preserving — a /simplify-code pass over the salvage diff surfaced four small wins:

Change Detail
Reuse _skill_file_list() diff_bundled_skill() reimplemented the rglobis_filerelative_to().as_posix() file-set enumeration inline (twice — once per side). The module already has _skill_file_list() doing exactly that; now set(_skill_file_list(dest)) / set(_skill_file_list(bundled_src)).
Share the user-modified predicate Extracted _is_tracked_user_modification(origin_hash, user_hash) and use it in both the sync loop and list_user_modified_bundled_skills() so the "kept user edit" rule (has origin hash AND hash differs) can't drift between the two sites — directly serves AGENTS.md "extend, don't duplicate".
Read binary files once _read_text_for_diff_read_for_diff returns (bytes, text); the binary-compare branch now reuses the bytes it already read instead of re-reading both files from disk.
Trim the return contract Dropped the unused user_present key from diff_bundled_skill()'s return dict (no consumer or test ever read it).
De-brittle the invariant test test_update_modified_notice dropped its >= 2 sites count-floor (which would fail a welcome consolidation of the two print paths into a shared helper) while keeping the per-site "count notice ⇒ discovery hint" invariant — still mutation-tested.

Verification

  • tests/tools/test_skills_sync.py + test_skills_list_modified_diff.py + tests/hermes_cli/test_update_modified_notice.py — 66 passed.
  • Mutation-tested the notice invariant: removing the hint from either update path fails the test; restoring it passes.
  • The shared predicate keeps the sync loop byte-identical (its empty-origin case is already guarded above the call site, so the predicate's bool(origin_hash) check is redundant-but-harmless there).
  • ruff clean.

Follow-up to #48286.

… trim diff contract

Cleanup pass on the salvage (behavior-preserving):

- diff_bundled_skill now uses the existing _skill_file_list() helper
  instead of reimplementing the rglob/is_file/relative_to file-set
  enumeration inline (twice).
- Extract _is_tracked_user_modification(origin_hash, user_hash) and use
  it in BOTH the sync loop and list_user_modified_bundled_skills() so the
  'kept user edit' rule can't drift between the two sites.
- _read_text_for_diff -> _read_for_diff returns (bytes, text); the binary
  branch now compares the bytes it already read instead of re-reading
  both files from disk.
- Drop the unused 'user_present' key from diff_bundled_skill's return
  contract (no consumer or test ever read it).
- test_update_modified_notice: drop the brittle '>= 2 sites' count-floor
  so consolidating the two print paths into a shared helper stays a
  welcome refactor; keep the per-site 'count notice => discovery hint'
  invariant (still mutation-tested).
@alt-glitch alt-glitch added type/refactor Code restructuring, no behavior change comp/tools Tool registry, model_tools, toolsets tool/skills Skills system (list, view, manage) comp/cli CLI entry point, hermes_cli/, setup wizard P3 Low — cosmetic, nice to have labels Jun 18, 2026
@kshitijk4poor
kshitijk4poor merged commit 860cf51 into NousResearch:main Jun 18, 2026
34 checks passed
waefrebeorn pushed a commit to waefrebeorn/slermes that referenced this pull request Jul 2, 2026
…-diff-cleanup

refactor(skills): dedupe file-listing + share user-modified predicate (follow-up to NousResearch#48286)
habarmc1223-sudo pushed a commit to habarmc1223-sudo/hermes-agent-fluxmem that referenced this pull request Jul 8, 2026
…-diff-cleanup

refactor(skills): dedupe file-listing + share user-modified predicate (follow-up to NousResearch#48286)
santhreal pushed a commit to santhreal/hermes-agent that referenced this pull request Jul 13, 2026
…-diff-cleanup

refactor(skills): dedupe file-listing + share user-modified predicate (follow-up to NousResearch#48286)
Gravezzz pushed a commit to Gravezzz/hermes-agent that referenced this pull request Jul 21, 2026
…-diff-cleanup

refactor(skills): dedupe file-listing + share user-modified predicate (follow-up to NousResearch#48286)
@kshitijk4poor
kshitijk4poor deleted the chore/skills-diff-cleanup branch August 5, 2026 07:07
leewenjie pushed a commit to leewenjie/hermes-agent that referenced this pull request Aug 7, 2026
…-diff-cleanup

refactor(skills): dedupe file-listing + share user-modified predicate (follow-up to NousResearch#48286)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cli CLI entry point, hermes_cli/, setup wizard comp/tools Tool registry, model_tools, toolsets P3 Low — cosmetic, nice to have tool/skills Skills system (list, view, manage) type/refactor Code restructuring, no behavior change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants