hermes-admin/hermes-agent
1
0
Fork 0
Code Issues Pull Requests Actions 28 Packages Projects Releases Wiki Activity

fix(curator): remove terminal from the consolidation fork (issue #96962)

Browse Source 
The curator LLM fork was steered by its own prompt to re-home skill
support files with terminal `mkdir -p ... && mv ...`. A terminal move
writes the same bytes with NO ledger entry, so the archive that follows
snapshots an already-stripped package (files: 1) and `hermes curator
rollback` restores a hollow skill — SKILL.md back, references/ gone.

Remove the capability rather than guard it: the fork's enabled_toolsets
drops "terminal", so terminal and process disappear together and there
is no shell to parse, no process stdin to feed, no remote-backend
divergence — a heuristic command guard over a Turing-complete input
space can guarantee none of that. Every mutation the pass needs has a
ledgered skill_manage action (write_file / remove_file / delete), and
the prompt now steers exactly those. Reading works through skill_view.

Tests pin both halves: the call-site kwarg (["skills"] only), the
resolved surface (no execution/write tools), and the prompt steering
(no mkdir -p / mv shapes).
This commit is contained in:
zengzheqing
2026-08-30 08:57:40 -07:00
committed by Teknium
parent 98e2f110cf
commit 37ae0e1d39
2 changed files with 91 additions and 43 deletions
Download Patch File Download Diff File Expand all files Collapse all files

38
agent/curator.py
View File

@@ -411,9 +411,6 @@ CURATOR_DRY_RUN_BANNER = (
"\n"
" • DO NOT call skill_manage with action=patch, create, delete, "
"write_file, or remove_file.\n"
" • DO NOT call terminal to mv skill directories into .archive/.\n"
" • DO NOT call terminal to mv, cp, rm, or rewrite any file under "
"~/.hermes/skills/.\n"
" • skills_list and skill_view are FINE — read as much as you need.\n"
"\n"
"Your output IS the deliverable. Produce the exact same "
@@ -508,9 +505,14 @@ CURATOR_REVIEW_PROMPT = (
"copied and modified\n"
" • `scripts/<name>.<ext>` for statically re-runnable actions "
"(verification scripts, fixture generators, probes)\n"
" Then archive the old sibling. Use `terminal` with `mkdir -p "
"~/.hermes/skills/<umbrella>/references/ && mv ... <umbrella>/"
"references/<topic>.md` (or templates/ / scripts/).\n\n"
" Then archive the old sibling. Re-home the content through the "
"LEDGERED tool surface: `skill_manage action=write_file` on the umbrella "
"to place the file (subdirectories are created for you), then "
"`skill_manage action=remove_file` on the source to drop the original, "
"then `skill_manage action=delete` on the source. Never a terminal move "
"— a shell mv/cp writes the same bytes with no ledger entry, so the "
"archive that follows snapshots an already-stripped package and "
"`hermes curator rollback` restores a hollow skill (issue #96962).\n\n"
"Package integrity — not optional:\n"
"Before demoting or archiving a skill, inspect it as a COMPLETE "
"directory package, not just SKILL.md. A skill root may include "
@@ -556,9 +558,10 @@ CURATOR_REVIEW_PROMPT = (
"skill, or `absorbed_into=\"\"` when you're truly pruning with no "
"forwarding target. This drives cron-job skill-reference migration — "
"guessing from your YAML summary after the fact is fragile.\n"
" - terminal — move LOCAL candidate content into "
"a support subfile when package integrity requires it; never mv, cp, rm, "
"patch, or rewrite bundled, hub-installed, or external-dir skills\n\n"
" You have NO terminal access in this pass — every filesystem mutation "
"goes through skill_manage above so it is ledgered and rollback-able "
"(issue #96962). Reading files works through skill_view (including "
"skill_view(name, file_path=...) for support files).\n\n"
"'keep' is a legitimate decision ONLY when the skill is already a "
"class-level umbrella and none of the proposed merges would improve "
"discoverability. 'This is narrow but distinct from its siblings' "
@@ -1543,7 +1546,9 @@ def run_curator_review(
If *dry_run* is True, the automatic stale/archive transitions are SKIPPED
and the LLM review pass is instructed to produce a report only — no
skill_manage mutations, no terminal archive moves. The REPORT.md still
skill_manage mutations. (The fork has no terminal access at all — see the
``enabled_toolsets=["skills"]`` kwarg in ``_run_llm_review``.) The
REPORT.md still
gets written and ``state.last_report_path`` still records it so users
can read what the curator WOULD have done. A dry-run also honors
*consolidate*: when consolidation is off, the preview only reports the
@@ -1945,7 +1950,18 @@ def _run_llm_review(prompt: str) -> Dict[str, Any]:
credential_pool=_credential_pool,
request_overrides=_request_overrides,
**_agent_kwargs,
enabled_toolsets=["skills", "terminal"],
enabled_toolsets=["skills"],
# ``terminal`` was deliberately removed from this fork (issue
# #96962): a terminal ``mv``/``cp``/``rm`` under the skills tree
# writes the same bytes with NO ledger entry, so the archive that
# followed snapshotted an already-stripped package and ``hermes
# curator rollback`` restored a hollow skill. Every mutation this
# fork needs has a ledgered skill_manage action (write_file /
# remove_file / delete), and reading works through skill_view.
# Removing the toolset closes the hole by construction — no
# command-parsing heuristic to evade, no process stdin to feed,
# no remote-backend divergence — which no terminal-write guard
# over a Turing-complete input space can guarantee.
# Umbrella-building over a large skill collection is worth a
# high iteration ceiling — the pass typically takes 50-100
# API calls against hundreds of candidate skills. The

96
tests/agent/test_curator.py
View File

@@ -851,17 +851,19 @@ def test_review_fork_uses_runtime_model_and_output_cap(curator_env, monkeypatch)
def test_review_fork_restricts_toolsets_to_skills_and_terminal(curator_env, monkeypatch):
"""The curator LLM fork must advertise only the skills + terminal toolsets.
def test_review_fork_restricts_toolsets_to_skills_only(curator_env, monkeypatch):
"""The curator LLM fork must advertise only the skills toolset.
Without ``enabled_toolsets=["skills", "terminal"]`` on the AIAgent(...) call
in ``_run_llm_review``, ``enabled_toolsets`` defaults to None and init_agent
grants the fork the full default catalog (~30 tools) plus the context_engine
(lcm_*) tools, billing ~7K wasted schema tokens on every one of the fork's
50-100 API calls per consolidation pass. The prompt (curator.py:509-523)
confines the model to four tools in natural language, but only this kwarg
filters the advertised request schema. Capturing the constructor kwarg is
the sole assertion that distinguishes fixed from unfixed code.
``terminal`` was removed from this fork for issue #96962: a terminal
mv/cp/rm under the skills tree bypasses the skill ledger entirely, so the
archive that followed snapshotted an already-stripped package and
``hermes curator rollback`` restored a hollow skill. Removing the toolset
(rather than guarding terminal commands) closes every shell bypass by
construction. Without ``enabled_toolsets=["skills"]`` on the AIAgent(...)
call in ``_run_llm_review``, ``enabled_toolsets`` defaults to None and
init_agent grants the fork the full default catalog (~30 tools) plus the
context_engine (lcm_*) tools. Capturing the constructor kwarg is the sole
assertion that distinguishes fixed from unfixed code.
"""
curator = curator_env["curator"]
@@ -892,37 +894,67 @@ def test_review_fork_restricts_toolsets_to_skills_and_terminal(curator_env, monk
# error is None proves the fork was actually constructed (capture ran).
assert meta.get("error") is None, meta.get("error")
assert captured.get("enabled_toolsets") == ["skills", "terminal"], (
"curator review fork did not pass enabled_toolsets=['skills', "
"'terminal'] to AIAgent; the full default tool catalog (plus lcm_* "
"context_engine tools) would be advertised; got "
f"{captured.get('enabled_toolsets')!r}"
assert captured.get("enabled_toolsets") == ["skills"], (
"curator review fork did not pass enabled_toolsets=['skills'] to "
"AIAgent; terminal must stay out (issue #96962) and the full default "
"tool catalog (plus lcm_* context_engine tools) must not be "
f"advertised; got {captured.get('enabled_toolsets')!r}"
)
def test_review_fork_toolset_surface_is_skills_plus_terminal():
"""Documentary check on the static surface the fork's kwarg resolves to.
def test_review_fork_toolset_surface_excludes_execution_tools():
"""The fork's toolset kwarg must resolve to a surface with no shell access.
Registry-independent (include_registry=False) so a plugin-registered tool
tagged into these toolsets cannot flake the membership checks. This
documents the intended surface (the four prompt-named tools present, dead
default and lcm_* schema absent) but does not itself guard the call-site
kwarg. No exact-set pin: intentional additions to either toolset must not
fail this test.
``terminal`` and ``process`` must stay out of the curator fork's resolved
surface (issue #96962): a shell mv/cp/rm under the skills tree bypasses
the skill ledger entirely, the archive that follows snapshots an
already-stripped package, and ``hermes curator rollback`` restores a
hollow skill. The call-site kwarg is pinned to ``["skills"]`` by the test
above; this test pins the RESOLUTION, so an ``includes: ["terminal"]``
added to the skills toolset definition — or a new execution tool merged
into it — fails here even though the kwarg never changed.
Registry-independent (``include_registry=False``): the boundary is the
static toolset definition. A plugin registering a tool into "skills" is
the user's own install decision, not the attack class this guard defends
against.
"""
from toolsets import resolve_toolset
surface = set(resolve_toolset("skills", include_registry=False)) | set(
resolve_toolset("terminal", include_registry=False)
)
surface = set(resolve_toolset("skills", include_registry=False))
# The four prompt-named tools are all present.
# The ledgered write surface is present in full.
assert "skills_list" in surface
assert "skill_view" in surface
assert "skill_manage" in surface
assert "terminal" in surface
# Representative dropped default + context_engine tools are absent.
assert "read_file" not in surface
assert "web_search" not in surface
assert "lcm_grep" not in surface
# The incident class stays out: no command execution, no background
# process steering (stdin is a second unguarded write sink), and no
# generic filesystem-write tool.
for tool in ("terminal", "process", "write_file", "patch",
"execute_code", "computer_use", "browser_exec"):
assert tool not in surface, (
f"execution/write tool {tool!r} leaked into the curator fork's "
"surface via the skills toolset — un-ledgered skill mutations "
f"become possible again (issue #96962); surface={sorted(surface)}"
)
def test_review_prompt_does_not_steer_terminal_writes():
"""The consolidation prompt must not steer the fork into shell mutations.
The #96962 incident was steered by a prompt line telling the fork to
``mkdir -p ~/.hermes/skills/<umbrella>/references/ && mv ...`` its
support files. Removing terminal from the toolset takes away the
capability; removing the steering stops the fork burning tool calls on
attempts that can only be refused. Both halves are load-bearing.
"""
from agent.curator import CURATOR_DRY_RUN_BANNER, CURATOR_REVIEW_PROMPT
for text in (CURATOR_REVIEW_PROMPT, CURATOR_DRY_RUN_BANNER):
assert "mkdir -p" not in text, (
"the curator prompt steers the fork toward a shell write of the "
"skills tree (issue #96962); re-home content via skill_manage "
"write_file/remove_file instead"
)
assert "&& mv" not in text