diff --git a/agent/curator.py b/agent/curator.py index c08d249612..11da351c51 100644 --- a/agent/curator.py +++ b/agent/curator.py @@ -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/.` for statically re-runnable actions " "(verification scripts, fixture generators, probes)\n" - " Then archive the old sibling. Use `terminal` with `mkdir -p " - "~/.hermes/skills//references/ && mv ... /" - "references/.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 diff --git a/tests/agent/test_curator.py b/tests/agent/test_curator.py index 4b8a46c9bb..eac61e01c5 100644 --- a/tests/agent/test_curator.py +++ b/tests/agent/test_curator.py @@ -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//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