feat(plugins): references_dir + plugin_reference_tool for agent-visible skill references - #27
Merged
Merged
Conversation
plugin_skill() and PluginSkill now accept an optional references_dir -- a companion directory of reference files sibling to SKILL.md -- validated the same way SKILL.md is (required unless the skill is optional, in which case a missing directory is dropped with a warning instead of raising). register_plugin() forwards references_dir to the host's register_skill only when the host's live signature actually accepts it (probed via inspect.signature, honoring both an explicit references_dir parameter and a **kwargs catch-all), so older hosts are never called with an argument they don't understand. This does not yet make any directory agent-visible: as of this writing, no released Hermes Agent host reads or serves plugin-skill companion files (confirmed by reading hermes-agent's _serve_plugin_skill, which hardcodes linked_files=None and never receives a directory argument). A warning is logged naming the skill so that gap stays visible instead of silently doing nothing. This is groundwork so plugins that already declare references_dir start working with no further kit-side change once a host adds support -- the actual host-side fix is out of scope for this repo and tracked separately.
The inspect.signature() capability probe alone is unsound: a bare Mock(spec=...) test double or a decorator applied without functools.wraps both present a (*args, **kwargs) shape that the probe reads as "host accepts references_dir" even when the real host doesn't -- and the actual ctx.register_skill(**kwargs) call had no guard, so a wrong guess crashed registration entirely. register_plugin now retries once without references_dir on a TypeError naming it; Python raises that error at argument binding, before the callee's body runs, so retrying is safe even against a host with side effects. Also fixes a real correctness bug: plugin_skill() was resolving an optional skill's missing references_dir to None before register_plugin ever saw it, so the "dropped with a warning" behavior documented in the README never actually logged anything for the common case (a references_dir that doesn't exist yet at declare time). plugin_skill() now only validates references_dir when the skill is required, mirroring exactly how SKILL.md's own optional handling already works, so register_plugin's existing re-check is the single place that warns and drops it. Extracts _signature_accepts_kwarg as a shared, named probe (previously inlined once for this feature and duplicated in concept from the existing _call_session_db_evolving pattern), updates skills/hermes-plugins/references/plugin-kit.md and README.md per this repo's own doc-sync rule, and adds 6 more tests covering the probe's except-branch, an explicit-named-parameter host, the retry-on-rejection safety net, and an unrelated-TypeError passthrough.
…rences plugin_reference_tool(skill, *, toolset, name=None, description=None) builds an ordinary @tool function that lists or reads files under a skill's references_dir, sidestepping register_skill/skill_view entirely -- the agent can reach the directory's contents today, on any host, via a normal tool call, without waiting on hermes-agent to add companion-file support. Review surfaced two real bugs before either landed silently: - references_dir accepted any directory with no requirement that it live near the skill; pointing it at /etc (or a mounted secrets volume) turned this into an arbitrary-file-read tool using only the documented public API. plugin_skill now requires references_dir to resolve as a descendant of the skill's own directory, and plugin_reference_tool re-checks the same constraint defensively in case PluginSkill is constructed directly, bypassing that factory. - The default tool-name derivation only swapped hyphens for underscores, so any skill name plugin_skill legally accepts but containing an uppercase letter or a leading digit (e.g. "Sample-Skill", "2fa-setup") produced an invalid tool name and crashed with ValueError. Fixed with a proper normalize-and-guard helper. Also rejects non-string file_path with a clean error instead of leaking a raw TypeError, and adds test coverage for the containment fix, the naming fix, absolute-path/non-string/empty-directory/ directory-as-file_path edge cases, and registering the tool through register_plugin like any other capability.
offendingcommit
marked this pull request as ready for review
August 18, 2026 20:29
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.
Plugin skills declared via
plugin_skill()can only ever expose their bareSKILL.md— there's no way to ship a companionreferences/directory alongside it, so a plugin that wants to give the agent a deeper lookup doc has nowhere to put it through this SDK's own registration path. This adds that declaration surface, and a working way to actually reach it today rather than waiting on a host change.What's here
references_dironplugin_skill()/PluginSkill/register_plugin()— forward-compatible groundwork. The actual host-side capability to serve companion files lives inhermes-agent, not here, and hasn't shipped in any released host. This is the kit half of that contract: plugins that declarereferences_dirtoday start working automatically the moment a host adds support, no second kit-side change required.plugin_reference_tool(skill, *, toolset)— makes the directory agent-visible today, without waiting on that host change. It builds an ordinary@toolfunction (the same mechanism every other plugin capability already uses) that lists or reads files underreferences_dirdirectly, sidesteppingregister_skill/skill_viewentirely.Design decisions
skills/hermes-pluginsauthoring reference, docstrings) states plainly that no released host readsreferences_dirviaregister_skillyet, rather than implying a capability that doesn't exist.plugin_reference_toolexists precisely because that gap has no timeline.Mock(spec=...)test double or a decorator applied withoutfunctools.wrapsboth present a(*args, **kwargs)shape that reads as "the host accepts this" even when it doesn't — confirmed by reproducing both live.register_plugin()retries once withoutreferences_dirif the host rejects it with aTypeErrornaming it; Python raises that error at argument binding, before the callee's body executes, so retrying is safe even against a host with side effects.references_dirmust actually be scoped to the skill. Review reproduced, end-to-end, that an unscopedreferences_dir(e.g. pointing it at/etc) turnsplugin_reference_toolinto an arbitrary-file-read tool using only the documented public API. Fixed by requiringreferences_dirto resolve as a descendant of the skill's own directory — enforced inplugin_skill()at declaration time, and re-checked defensively inplugin_reference_tool()in casePluginSkillis constructed directly, bypassing that factory.plugin_skill()was resolving an optional skill's missingreferences_dirtoNonebeforeregister_plugin()ever ran its own check, so the "dropped with a warning" behavior the README promised never actually logged anything for the ordinary case. Fixed by havingplugin_skill()only validate existence for required skills, exactly mirroring howSKILL.mditself is already handled.plugin_skill()itself accepts. The first cut only swapped hyphens for underscores; any skill name with an uppercase letter or leading digit (both legal perplugin_skill()'s own name pattern) produced an invalid tool name and crashed. Fixed with a proper normalize-and-guard helper, reproduced and locked in with a test.Test plan
just test— 181 unit tests pass: 13 forreferences_dirplumbing (declaration-time/registration-time validation, the capability probe and its retry-on-rejection safety net, an explicit-named-parameter host, a**kwargs-only host, uninspectable signatures, unrelatedTypeErrorpassthrough) and 15 forplugin_reference_tool(listing, reading, custom name/description, the containment fix from both enforcement points, the tool-name fix for uppercase/leading-digit skill names, absolute-path/relative-traversal/symlink rejection, non-stringfile_path, empty directories, and a directory passed asfile_path).just test-contractagainst the realhermes-agentcheckout could not be run in this environment — fails onmaintoo, before this change, on an unrelated pre-existing import gap. Confirmed via direct source reading (not just the failing import) that the real host'sregister_skill(self, name, path, description="")has noreferences_dirparameter and no**kwargs, so this change's fallback path — register without it, log a warning — is exactly what fires against production today.uv run python -m compileall -q hermes_plugin_kit tests— clean.Known Residuals
Reviewed and accepted, not applied, each for a stated reason:
plugin_reference_toolreads are not TOCTOU-safe against areferences_dirwritable by an untrusted process at runtime (the containment check and the eventual read are separate syscalls, not a held file descriptor). Fine for the intended case — a static directory shipped with the plugin — insufficient if that assumption doesn't hold for a given deployment; documented in the README and the docstring rather than solved with fd-pinning, which is disproportionate for a directory authored by the plugin's own build pipeline.register_plugin()is not transactional. A failure partway through registers some surfaces and not others — true of every registration surface in this function already, not specific to this change.tests/test_hermes_contract.py(just test-contract) was not extended with a case exercisingreferences_diragainst the real host signature — the localhermes-agentcheckout can't currently be imported in this environment for unrelated reasons (see Test plan).Post-Deploy Monitoring & Validation
No additional operational monitoring required for the
references_dirplumbing — it ships no runtime behavior change for any consumer (no plugin in this ecosystem currently passes it).plugin_reference_toolis opt-in per plugin (a plugin must explicitly call it and register the result), so it also has no default-on behavior to monitor; once a plugin adopts it, watch that plugin's own tool-call logs for the new<skill>_read_referencetool name.