Conversation
_locate_skill refused every name with more than one candidate. That is right for one skill reachable through two tiers (silent shadowing) but wrong for duplication inside a single search dir: the runtime root is a symlink view of the library, so a top-level symlink and a nested category copy of the same skill collided and made the bare name unusable — bundle members were then reported as missing and skipped. Candidates are now ranked within one root: a real SKILL.md wins over a legacy <name>.md, then the shallower path. Cross-tier ambiguity keeps refusing. Ownership is decided lexically so a symlinked entry is not reclassified into another root. Trust warnings accept the resolved target of every root entry as well as the lexical path, so a root that exposes skills through symlinks no longer warns on every load and the injection-pattern signal stays readable. Tests: 183 passed (tests/tools/test_skills_tool.py + tests/agent/test_skill_*.py). The two new trust tests fail against the previous code; the outside-file test passes on both and guards the invariant.
`_load_bundle_file` documents "None (logged) on any error so a broken bundle can't break discovery", but only caught `OSError` and `YAMLError`. `UnicodeDecodeError` is a `ValueError`, so one manifest with an invalid byte propagated out of `scan_bundles` and took down discovery of EVERY bundle (TUI `/bundles`, slash-command bundles, cron prompts built from bundles). Adds a `UnicodeError` branch (log + skip) and a regression test asserting a latin-1 manifest is skipped while valid bundles still load. Verified with the canonical runner (scripts/run_tests.sh): tests/agent/test_skill_bundles.py - 19 passed (18 + 1 regression test) skills set (9 files) - 184 passed
teknium1
added a commit
that referenced
this pull request
Sep 17, 2026
…shape Slim redo of the cherry-picked #112180 hunks without changing behaviour: `_owning_search_dir` / `_rank_same_root_candidate` lose their unreachable fallbacks (the owning root is chosen by lexical containment, so `relative_to(root)` cannot fail), the trust check accepts the lexical path inline instead of through a nested helper, and the WHY-only comments replace the install-specific narration. Tests trimmed to two invariants per fix: same-root nested copy resolves to the shallower path, equal-rank tie still refuses; symlinked entry is trusted by the root that exposes it, a genuinely outside file still warns. Dropped the legacy flat `<name>.md` test (the rank still covers it; re-probed live).
Related: #113126 merged the maintainer salvage of these skill and bundle-discovery fixes. |
This was referenced Sep 21, 2026
beardthelion
added a commit
to beardthelion/hermes-agent
that referenced
this pull request
Sep 21, 2026
read_text(encoding="utf-8") raises UnicodeDecodeError - a ValueError,
not an OSError - so readers guarded only by except OSError crash instead
of degrading:
- _inject_context_from: one corrupt .md in a source job's output dir
crashed _build_job_prompt (which runs outside run_job's try), so the
downstream job failed on every fire while the file remained. The read
now skips the file like a silent/blank archive, so an older usable
archive still gets used instead of masking the whole source.
- read_active_org_id: a non-UTF-8 .active_org marker escaped its
fail-safe contract ("None = no org skills load") into
_build_skills_manifest during system-prompt assembly - every turn.
Now returns None as documented.
The sibling arm in skill_bundles._load_bundle_file is already covered
by open PR NousResearch#112180, which adds the same UnicodeError branch there.
This branch has not been deployed
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?
Two fixes in the skill/bundle layer. Both were reproduced before being changed, and each one has a regression test that fails on
main.1. Same-root duplicate names resolve instead of refusing (
tools/skills_tool.py)_locate_skill()refused every name with more than one candidate — correct when the candidates come from two different tiers (silent shadowing), wrong when both live under the same root, which is the normal layout on installs whose skills root is a symlink view of another directory:Both candidates are the same skill in the same root, yet the bare name was refused, so
skill_view('<name>')failed and bundle members declared by bare name were printed underSkills missing (skipped)while sitting on disk.The approach: rank candidates within one root (real
SKILL.mdbeats legacy<name>.md, then shallower path), keep refusing cross-tier ambiguity, and decide ownership lexically so a symlinked entry is not reclassified into the root it points at. The trust check additionally accepts the resolved target of every configured root entry, so a symlinked root no longer warns on every load.2. A non-UTF-8 manifest no longer takes down bundle discovery (
agent/skill_bundles.py)_load_bundle_file()documents "None(logged) on any error so a broken bundle can't break discovery", but only caughtOSErrorandyaml.YAMLError.UnicodeDecodeErroris aValueError, not anOSError, so a single manifest containing an invalid byte propagated out ofscan_bundles()and broke every bundle surface for that install — the TUI/bundleslisting, slash-command bundle loading, and cron prompts built from bundles.Reproduced before the fix:
Now that file is logged and skipped while valid bundles still load, which is what the docstring already promised.
Related Issue
Fixes #112179
Type of Change
Changes Made
tools/skills_tool.py—_owning_search_dir()(which configured root owns a candidate, decided lexically) and_rank_same_root_candidate()(tie-break inside one root);_collect_skill_candidates()/_locate_skill()use them so same-root duplication resolves while cross-tier ambiguity still refuses.tools/skills_tool.py—_log_security_warnings()accepts a symlinked entry when the root that exposed it vouches for the resolved target.agent/skill_bundles.py—_load_bundle_file()gains aUnicodeErrorbranch (log +None) so an unreadable-encoding manifest cannot abort discovery of the other bundles.tests/tools/test_skills_tool.py—TestSameRootDuplicationResolves(resolution) andTestTrustWarningSymlinkAware(warn/do-not-warn). Two of the new tests fail againstmain;test_genuinely_outside_file_still_warnspasses on both and guards the invariant.tests/agent/test_skill_bundles.py—test_skips_invalid_utf8_without_breaking_discovery(fails onmainwith theUnicodeDecodeErrorabove, passes here).How to Test
main: create a skills root containing both a top-level symlink and a nested copy of one skill, thenskill_view('<name>')→Skill name collision for '<name>': 2 candidates. On this branch the name resolves to the realSKILL.md.pytest tests/tools/test_skills_tool.py -q -k "Collision or SameRoot"→ the cross-tier refusal test still passes.pytest tests/tools/test_skills_tool.py -q -k TrustWarningSymlinkAware→ 2 passed here, 1 failed onmain.pytest tests/agent/test_skill_bundles.py -q -k utf8→ fails onmain(UnicodeDecodeError), passes here.Verification
Run with the repo's canonical runner (
scripts/run_tests.sh, per-file isolation), not a barepytest:The full
tests/sweep is still running locally on a 4-core/4 GB host; I am not ticking the full-suite box until it reports, and the PR stays a draft until then.Checklist
Code
Documentation
Testing
General