From 610e2e02fb10360fdd1610d25a7fd084dd8d7e76 Mon Sep 17 00:00:00 2001 From: Guilherme Artiles Date: Sun, 2 Aug 2026 09:04:32 -0300 Subject: [PATCH] fix(curator): tell the background reviewer to read before it writes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_background_review_read_before_write_guard` refuses a background-review `skill_manage` write when the target file was not loaded via `skill_view` in the same review turn (patch, edit, write_file over an existing file, remove_file). `CURATOR_REVIEW_PROMPT` never says so. It lists `skill_view` only under "read the current landscape", so the reviewer goes straight to the write and every mutation is refused. The failure is silent from the outside: the curator run completes, writes nothing, and reads like a pass that simply found nothing to consolidate. On our deployment that was 32 of 32 attempted writes rejected over 48h before anyone read the logs. This adds the missing instruction to the toolset block, plus a test that fails if a future guarded action is added to `skill_manager_tool` without being named in the prompt — the guard and the prompt have to drift together or not at all. Co-Authored-By: Claude Opus 5 --- agent/curator.py | 7 +++++++ tests/agent/test_curator.py | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+) diff --git a/agent/curator.py b/agent/curator.py index 9668e76a46..c08d249612 100644 --- a/agent/curator.py +++ b/agent/curator.py @@ -539,6 +539,13 @@ CURATOR_REVIEW_PROMPT = ( "merges.\n\n" "Your toolset:\n" " - skills_list, skill_view — read the current landscape\n" + " READ BEFORE WRITE — enforced, not advisory. Before skill_manage " + "action=patch, action=edit, action=write_file on a file that already " + "exists, or action=remove_file, call skill_view on that SAME target in " + "this review turn — skill_view(name) for SKILL.md, " + "skill_view(name, file_path=...) for a supporting file — and build the " + "write from the content it just returned. A write without that read is " + "REFUSED and nothing is saved.\n" " - skill_manage action=patch — add sections to the umbrella\n" " - skill_manage action=create — create a new umbrella SKILL.md\n" " - skill_manage action=write_file — add a references/, templates/, " diff --git a/tests/agent/test_curator.py b/tests/agent/test_curator.py index f501c55b5f..bf448b21b6 100644 --- a/tests/agent/test_curator.py +++ b/tests/agent/test_curator.py @@ -525,6 +525,42 @@ def test_curator_does_not_instruct_model_to_pin(): +def test_curator_prompt_covers_every_read_before_write_guarded_action(): + """The review prompt must name every action the read-before-write guard + protects. + + ``_background_review_read_before_write_guard`` refuses a write when the + target was not loaded via ``skill_view`` in the same review turn. The + curator's forked reviewer only knows to do that if the prompt says so — + a guard the prompt never mentions is a silently jammed write channel, + not a safety net. This test fails if a new guarded action is added to + ``skill_manager_tool`` without teaching the prompt about it. + """ + import inspect + import re + + from agent.curator import CURATOR_REVIEW_PROMPT + from tools import skill_manager_tool + + known_actions = { + "create", "edit", "patch", "write_file", "remove_file", "delete", + } + source = inspect.getsource(skill_manager_tool) + guarded = set() + for call in source.split("_background_review_read_before_write_guard(")[1:]: + guarded |= known_actions.intersection(re.findall(r'"([^"]+)"', call[:200])) + + assert guarded, "no guarded actions found — did the guard get renamed?" + + prompt = CURATOR_REVIEW_PROMPT + assert "skill_view" in prompt + missing = [a for a in sorted(guarded) if f"action={a}" not in prompt] + assert not missing, ( + "read-before-write guards these actions but the curator prompt never " + f"tells the reviewer to read first for: {missing}" + ) + + def test_cli_pin_refuses_bundled_skill(curator_env, capsys): from hermes_cli import curator as cli skills_dir = curator_env["home"] / "skills"