fix(skills): require --force to replace an untracked skill on install - #80903
fix(skills): require --force to replace an untracked skill on install#80903briandevans wants to merge 3 commits into
Conversation
`install_from_quarantine()` calls `shutil.rmtree(install_dir)` on whatever
sits at a skill's install path. Its own comment justifies that by deferring
the consent question upstream:
A directory that directly contains SKILL.md is an existing skill
installation and stays overwritable (hub-installed skills are
additionally guarded by the lock-file check in do_install()).
That parenthetical does not hold for a skill with no lock entry.
`do_install()`'s only pre-install collision check reads the hub lock file, so
`existing is None` for two first-class populations that live under `skills/`
without one: locally authored skills (`source_type="local"`) and bundled
skills the user has edited — the repo ships `skills list --modified`,
`skills diff` and `skills reset --restore` specifically for the latter. For
those, installing a same-named skill needs no `--force`, prints no warning,
and destroys the directory.
Consult the filesystem alongside the lock file, resolving the target with
`install_from_quarantine`'s own validators so the two agree on which
directory is at stake. When it already holds a skill (directly contains
`SKILL.md`) and the lock file has no entry for it, warn and require
`--force`, matching the consent gate a hub-installed skill already gets.
`do_install()` is the single production caller of
`install_from_quarantine()`, so this one guard covers every install surface:
`hermes skills install`, the `/skills install` slash command (which hardcodes
`skip_confirm=True`), the tui_gateway skills tool (`skip_confirm=True` with a
console that discards output), and `skills snapshot import`. `do_update()`
passes `force=True` and is unaffected.
The existing lock-file branch, the `--force` escape hatch, the category-bucket
guard, and the malformed-name errors are all left unchanged.
alt-glitch
added
type/bug
There was a problem hiding this comment.
Pull request overview
Adds an additional overwrite-consent gate for hermes skills install when the target directory already contains a skill on disk but has no Skills Hub lock entry, preventing silent deletion of locally-authored or user-edited skills.
Changes:
- Add
_untracked_skill_dir()to detect an existing on-disk skill directory (by presence ofSKILL.md) using the same path validators asinstall_from_quarantine(). - Extend
do_install()to refuse installs (unless--force) when the install target is an existing untracked skill directory, even if the hub lock file has no entry. - Add regression tests that exercise the real quarantine/install path and assert the on-disk skill content is preserved without
--force.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
hermes_cli/skills_hub.py |
Adds filesystem-based collision detection for untracked skills and gates overwrite behind --force. |
tests/hermes_cli/test_skills_hub.py |
Adds end-to-end regression coverage ensuring untracked skills aren’t silently replaced, and --force remains an escape hatch. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| shown = ( | ||
| f"{display_hermes_home()}/skills/" | ||
| f"{category + '/' if category else ''}{bundle.name}" | ||
| ) |
There was a problem hiding this comment.
Confirmed and fixed in faec6d69a93 (current head).
You're right that the two can diverge, and it's worth spelling out how far: _untracked_skill_dir() routes the category through _validate_install_parent_path() → _normalize_bundle_path(), which drops empty and . segments and rewrites \ → /, then _resolve_lock_install_path() .resolve()s the result. So --category "personal//./notes" produced a prompt naming skills/personal//./notes/note-taker while the directory install_from_quarantine() would have rmtree'd was skills/personal/notes/note-taker. Naming the deletion target is the entire purpose of this prompt, so a mismatch defeats it.
The message is now derived from the resolved untracked path (hermes_cli/skills_hub.py:685-692), keeping the profile-safe display_hermes_home() shorthand and falling back to the absolute path when the target lies outside HERMES_HOME (i.e. a SKILLS_DIR override, where there is no ~/ shorthand to apply).
Pinned by test_untracked_warning_names_the_resolved_deletion_target in tests/hermes_cli/test_skills_hub.py:369. It asserts the emitted path equals _untracked_skill_dir()'s return value — the actual rmtree target — and that the raw category string is absent from the output. Verified red before the change (the assertion diff showed the two paths side by side) and green after; the file's 7 tests pass.
One deliberate exclusion: the same raw-category construct appears in the two "Files will be at:" panels further down (the Official Skill and Disclaimer panels). Those describe an install destination rather than a deletion target, and they are not part of this PR's diff, so I've left them alone rather than widen an in-review change. Happy to send a follow-up for them if you'd like the normalization applied there too.
… warning The refusal message built its path by interpolating the raw `category` string, while the directory actually at risk is the one returned by `_untracked_skill_dir()`, which resolves through `_validate_install_parent_path()` / `_resolve_lock_install_path()`. Those normalize the category -- empty and "." segments are dropped and "\" is rewritten to "/" -- so the two could disagree: with `--category "personal//./notes"` the prompt named `skills/personal//./notes/note-taker` while `install_from_quarantine()` would have rmtree'd `skills/personal/notes/note-taker`. Naming the deletion target is the entire purpose of this consent prompt, so derive the message from the resolved path instead, keeping the `display_hermes_home()` shorthand and falling back to the absolute path when the target lies outside HERMES_HOME (a `SKILLS_DIR` override).
|
This was generated by AI during triage. Summary: Problems:
Solution: Checked against |
…rface
The guard added earlier in this PR refuses to replace an untracked skill by
returning early from do_install(). On the tui_gateway skills tool that refusal
was invisible: the handler hands do_install() a console whose print() is a
literal `pass`, and then returns {"installed": True, "name": query}
unconditionally. So the one surface this PR newly refuses on was the one
surface that reported the refusal as a success, with no way to override it.
Consult the guard's own helper (_untracked_skill_dir) before calling, and
report {"installed": False, "reason": "untracked_skill_exists", "path": ...}
with the directory that would have been replaced and an actionable hint.
Resolving that directory is delegated to the helper rather than rebuilt here,
so this surface and the CLI cannot name different deletion targets. `force` is
now forwarded to do_install(), giving this surface the same escape hatch the
CLI has; it is coerced with the project's shared is_truthy_value() so the JSON
string "false" cannot consent to an rmtree.
The success shape is unchanged, and do_install()'s `-> None` contract is not
touched: its ten other early returns (blocked scan, rate limit, unknown source
adapter, malformed name) misreport on this surface too, but they predate this
PR and belong to different code paths.
|
@spfcraze Confirmed on all three points, and fixed in 1. No-op console + unconditional success. Fixed at force = is_truthy_value(params.get("force", False))
if not force:
untracked = _untracked_skill_dir(query, "")
if untracked is not None:
return _ok(rid, {
"installed": False,
"name": query,
"reason": "untracked_skill_exists",
"path": str(untracked),
"message": (... "Retry with force: true to overwrite."),
})
do_install(query, force=force, skip_confirm=True, console=_Q())The success shape is byte-identical to before, so no consumer breaks. The directory is resolved by 2. The bare 3. And the checklist line you quoted was wrong, so I've corrected it rather than leaving it. It claimed the tool "gains the same refusal as the CLI"; the refusal behaviour did apply (no Regression tests, in
All three fail against the previous head One thing worth surfacing while this block is in view: #38688 (@coygeek) is a competing design on these same ~14 lines — it removes |
This is a sibling follow-up to #80848
do_update()→do_install(force=True)replacing a hub skill the user has edited. It comparescontent_hash(skill_dir)against the lockfile'scontent_hashand skips unless--force.do_check/do_update/skills_command/handle_skills_slash(@@1052,@@1063,@@1082,@@1745,@@1929) — thedo_installcollision block is untouched.--force. Disjoint hunks (@@179,@@636); the two changes compose.What does this PR do?
install_from_quarantine()callsshutil.rmtree(install_dir)on whatever sits at a skill's install path. Its own comment justifies that by deferring the consent question upstream —tools/skills_hub.py, above thermtree:That parenthetical does not hold for a skill with no lock entry.
do_install()'s only pre-install collision check is:HubLockFileonly tracks hub installs, soexisting is Nonefor two first-class populations that live underskills/without a lock row:source_type = "local"/trust = "local".skills list --modified,skills diffandskills reset --restorespecifically for this population.For either, installing a same-named skill takes no
--force, prints no warning, and deletes the directory. This PR makes that comment true by consulting the filesystem alongside the lock file.This is the sibling of the data-loss report in #75983 ("silently deletes an entire existing category directory … unconditional rmtree, data loss"). The fix for that one —
75e85ef(refuse to overwrite category bucket during skill install), widened a day later by881ac52(hybrid skill-dir nesting and file collisions) — guards the category-bucket branch and explicitly leaves theSKILL.md-bearing branch overwritable, on the strength of the lock-file claim quoted above. This closes that remaining branch.Coverage: one guard covers every install surface
do_install()is the single production caller ofinstall_from_quarantine():So the one guard covers all of these, and no other site shares the root cause:
hermes skills installskills_command,hermes_cli/skills_hub.py/skills installhandle_skills_slashskip_confirm = Trueis hardcodedskills.manage,tui_gateway/methods_tools.pyskip_confirm=Trueand a console whoseprintdiscards output. Fixed by the follow-up commit belowhermes skills snapshot importdo_snapshot_importforcethrough; defaults toFalsehermes skills updatedo_updateforce=True— unaffected by this change; that path is #80848'sFollow-up commit
0398a15— make the refusal observable onskills.manageThanks @spfcraze — this was a real hole and it was one this PR opened. The table row above understated it, so it is corrected there too.
The problem. The refusal behaviour reached the tui_gateway surface (no
rmtree), but the report did not. As filed, the handler was:do_install()is-> Noneand refuses with a barereturn, and_Q.printis a literalpass. So on the one surface this PR newly refuses on, a refusal was reported asinstalled: True, theUse --force to overwrite.hint went nowhere, and there was noforceargument to override it with. That is strictly worse than the pre-PR state, where the same unconditionalinstalled: Trueat least happened to be true.The fix. Consult the guard's own helper before calling, and forward
force:installed: falsewithreason: "untracked_skill_exists",path(the directory that would have been replaced), and amessagecarrying the actionable hint. The success shape is byte-identical to today's{"installed": True, "name": query}, so no consumer breaks._untracked_skill_dir()— the same helper the CLI guard uses — rather than rebuilt here, so the two surfaces cannot name different deletion targets. This is the same reasoning as the Copilot thread on the CLI message, applied to the second surface.forceis forwarded todo_install(), giving this surface the CLI's escape hatch. It is coerced through the project's sharedis_truthy_value()(asmethods_session.py/methods_prompt.pydo) rather thanbool(), becausebool("false")isTrueand this gate decides whether a user's own skill directory isrmtree'd.No client change is needed for
/skills install.ui-tui/src/app/slash/commands/ops.ts:612already branches on the flag:so an honest
installed: falserenders asinstall failedtoday, with noui-tui/edit. Separately,ui-tui/src/components/skillsHub.tsx:83discards the payload (.then(() => onClose())) — a pre-existing gap in the overlay that predates this PR and is not touched here; #38688 already proposes changes to that file (see Positioning).Deliberately out of scope:
do_install()'s-> Nonecontract is unchanged. It has 11 barereturns; the other ten (unknown source adapter, fetch failure / rate limit, missing-name on a URL install, blocked scan verdict, cancelled confirm, invalid install path) also misreport asinstalled: Trueon this surface. But those predate this PR, sit on different code paths, and fixing them means changing the return type of a function with five call sites — a general outcome-reporting refactor bundled into a consent-gate fix. Happy to file it as a follow-up, or to fold it in here if you would rather have it in one go.Related Issue
No auto-close keyword — this PR does not fully resolve an open issue on its own.
Related: #75983 (CLOSED, the category-bucket case whose fix introduced the comment quoted above) and #80848 (the update-caller half of the same hazard).
Type of Change
Changes Made
hermes_cli/skills_hub.py— new_untracked_skill_dir(name, category)helper. It resolves the install directory usinginstall_from_quarantine()'s own validators (_validate_skill_name,_validate_install_parent_path,_resolve_lock_install_path) so both functions agree on which directory is at stake, and returns it only when that directory directly containsSKILL.md.hermes_cli/skills_hub.py— indo_install(), after the existing lock-file block, refuse whennot existing and not forceand the helper reports an untracked skill. Message follows the surrounding console idiom and the path is rendered viadisplay_hermes_home(), matching the confirm panel.tests/hermes_cli/test_skills_hub.py— three regression tests.tui_gateway/methods_tools.py(follow-up0398a15) — theskills.manageinstallaction pre-checks with_untracked_skill_dir(), reportsinstalled: false+reason+path+messageon refusal, and forwardsforce(coerced withis_truthy_value) todo_install(). +29/-2.tests/tui_gateway/test_protocol.py(follow-up0398a15) — three regression tests on that handler. +91.Deliberately unchanged, to keep the diff to one idea:
already installedpath keeps its exact message and behavior.--forceremains a full escape hatch.ValueErrors ininstall_from_quarantine()are untouched;_untracked_skill_dirreturnsNoneon those so the existing, more actionable downstream error is still what the user sees.tools/skills_hub.pyis not edited. The point is to make its existing comment true, not to move the guard.How to Test
SKILL.mdnow contains the upstream body. The hand-written file is gone.Warning: 'note-taker' already exists at <hermes-home>/skills/note-taker and is not tracked by the skills hub (a local or user-edited skill)./Installing would replace it. Use --force to overwrite.— and the file is untouched.--forcestill overwrites.Regression test, verified in both directions
The tests drive the real
quarantine_bundle/install_from_quarantine/HubLockFileinsidetmp_path(only the remote source and the scanner are faked), so the destructivermtreegenuinely runs and the red is real data loss rather than a mock assertion.RED — production hunk reverted (
git stash push -- hermes_cli/skills_hub.py), tests untouched:It fails on the data-survival assertion, not on a message string.
GREEN — fix restored:
The other two tests hold in both directions by design — they are non-regression assertions, pinning that
--forcestill overwrites and that the lock-file branch keeps its existing message.Follow-up commit
0398a15, also verified in both directionsThree tests in
tests/tui_gateway/test_protocol.py, driven throughserver.handle_request()so they exercise the real registered handler. They seed a real untracked skill directory undertmp_pathand stub onlydo_install, so "was the refusal reported?" and "wasdo_installreached at all?" are both real assertions.RED — follow-up production hunk reverted (
git checkout HEAD~1 -- tui_gateway/methods_tools.py), tests untouched:The first failure is the reported bug exactly: the surface says
installed: Truefor an install it refused.GREEN — fix restored:
43 passed(tests/tui_gateway/test_protocol.py).Suites run for the follow-up commit (green on
0398a15):Suites run for the first two commits (green on
faec6d6):Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests pass — ran the targeted suites listed under "How to Test" instead of the full tree; leaving this unchecked rather than claiming a run I didn't doDocumentation & Housekeeping
I've updated relevant documentation (README,
docs/, docstrings) — or N/A — N/A: no user-facing flag or config changed;--forcealready documented. Behavior is covered by the new docstring.I've updated
cli-config.yaml.exampleif I added/changed config keys — or N/A — N/AI've updated
CONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/A — N/AI've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A — pathlib only, no separators or platform branches; path resolution is delegated to the existing
_resolve_lock_install_path, so it inherits whatever junction/symlink handling that helper has (including any hardening from fix(skills): resolve install path against resolved skills dir (Windows junction) #76885 / fix(skills): resolve the skills root before computing the lock install path #64954).I've updated tool descriptions/schemas if I changed tool behavior — or N/A — The
skills.manageinstallaction changes in0398a15: it accepts a new optionalforceparam and, on refusal, returnsinstalled: falsewithreason/path/messageinstead ofinstalled: true. The success shape is byte-identical.skills.managehas no declared JSON schema — its params are read from theparamsdict — so there is no schema file to update; the two typed TS consumers areSkillsInstallResponse(ui-tui/src/app/slash/commands/ops.ts:41, whose fields are all optional and which already branches onr.installed) and the inline type atui-tui/src/components/skillsHub.tsx:83. Neither needs a change to keep compiling or behaving.(This line previously read "N/A: no tool schema change; the tui_gateway skills tool gains the same refusal as the CLI." That overstated: the refusal behaviour reached that surface, but the report and the override did not — which is exactly what the table above records as consent "none", so the body contradicted itself. Corrected here and in that table.)
Related / Positioning
Dedup performed before opening, recorded so it can be checked rather than taken on trust:
gh pr list --state open --limit 300 --json number,author,files, snapshot spanning PRs fix(search): translate \d/\D for the grep fallback engine #80454–fix(web): consult pluginTabs before the hardcoded root title in resolvePageTitle #80897): the only other open PR touchinghermes_cli/skills_hub.pyortests/hermes_cli/test_skills_hub.pyin that window is fix(skills): skip locally-edited hub skills on update unless --force (Paperclip port) #80848.gh search prs --state open):do_install,install_from_quarantine,skills_hub,HubLockFile,_resolve_lock_install_path,untracked skill,local skill overwrite,skill install rmtree,user-edited skill. Every_resolve_lock_install_path/install_from_quarantinehit is about resolving the path correctly under symlinks, junctions or CRLF (fix(skills): install_from_quarantine fails when HERMES_HOME is a symlink #49885, fix(skills): handle symlinked skills install paths #53493, fix(skills): resolve the skills root before computing the lock install path #64954, fix: resolve _skills_dir() for symlinked HERMES_HOME install paths (#64953) #65050, fix(skills): resolve install path against resolved skills dir (Windows junction) #76885, fix(skills): resolve paths before computing relative install path #53409, fix(skills): resolve symlinks before relative_to in skill install #53541, fix(skills): restore bundle/installed content-hash symmetry on Windows #78082, fix(skills): normalize CRLF to LF in content_hash for Windows parity #58827, fix(skills): use bundle_content_hash in lock to prevent perpetual update_available #41199), not about consent to overwrite. TheHubLockFilehits are all atomic-write PRs (fix(skills_hub): use atomic writes for lock.json and taps.json #16440, fix(skills-hub): use atomic writes for lock.json, taps.json, and index cache #29699, fix: guard json.loads() and use atomic writes for persistent state #29019, fix: close PIL Image FDs, atomic writes, replace deprecated utcnow #30139, feat(skills): inventory and audit effective skill roots #64244).existing is Nonecase:except ValueError→except (ValueError, PermissionError)on the quarantine/install calls, plus a read-only gate at the top ofdo_install.skill_lockconcurrency lock; orthogonal mechanism.if not force:toif not (force or reinstall_existing):inside theif existing:branch so update flows can reinstall without also forcing past a blocked scan. That only has an effect whenexistingis truthy, so it leaves the no-lock-entry case exactly as it is today. The two changes are complementary; this PR leaves that line untouched, so they should not conflict textually either.git log --since=30d -- hermes_cli/skills_hub.pyshows 5 commits, none indo_install.Re-run for the follow-up commit's file,
tui_gateway/methods_tools.py(snapshot spanning PRs #80579–#81135):fix(tui-gateway): preserve skill install review), and I would rather flag it than have you find it. It removes thedo_installcall from this handler outright and returns{"installed": False, "status": "review_required"}, i.e. it takes install off this surface rather than making it report honestly, and it carries matchingui-tui/changes (ops.ts,skillsHub.tsx). It is not a duplicate of this change, and as written it does not apply to the current tree: it targetstui_gateway/server.py@15761, and the mechanical splitf67ca22("split @method handlers into methods_* modules") moved that handler intotui_gateway/methods_tools.py. Last updated 2026-07-21;mergeStateStatus: UNKNOWN. If you prefer that direction, say so and I will close this half — the two are alternatives on the same 14 lines, not complements.tui_gateway/methods_tools.pyin that window is feat: route skills to configured provider and model #80692 (@pedro-dalben, skill provider/model routing), whose hunks are at@545/@554— disjoint from theinstallaction._untracked_skill_dirreturns this PR only.methods_toolsreturns fix(security): route tui_gateway shell.exec through the sanitized env builder #78036 (shell.execenv builder,@1886/@1893— disjoint) and my own fix(tui_gateway): tolerate a deleted working directory across session, completion and exec paths (supersedes #40153) #76131 (@391/@1408/@1886— disjoint; the install block sits between its two nearest hunks).skills.managereturns fix(tui): ignore invalid skills browse pagination #51946, fix(skills): add skills.read_only runtime write guard (#64926) #64963, feat: expose skills RPCs #19168, fix(security): close skill-scanning and command-approval gaps for non… #57990 — none on the install action.@842), fix(tui_gateway): tolerate a deleted working directory across session, completion and exec paths (supersedes #40153) #76131 (above) and fix(tui_gateway): reconcile every build-relevant override with the deferred agent build #75385 (@635). All hunk-disjoint from@1736.Happy to rebase behind #80848, fold this into it, or adjust the wording of the warning if you'd prefer it phrased differently.