[codex] Fix background review skill read-before-write

agent/background_review.py

@@ -9,8 +9,9 @@
 
 The fork inherits the parent's live runtime (provider, model, base_url,
 credentials, cached system prompt) so it hits the same prefix cache and
-uses the same auth.  It runs with a tool whitelist limited to memory and
-skill management tools; everything else is denied at runtime.
+uses the same auth. It runs with a runtime whitelist of exact tool names:
+``memory`` when enabled, plus ``skills_list``, ``skill_view``, and
+``skill_manage``. Everything else is denied at runtime.
 
 See the ``hermes-agent-dev`` skill (``references/self-improvement-loop.md``)
 for invariants and PR review criteria.
@@ -163,6 +164,14 @@ def _digest_history(messages_snapshot: List[Dict], tail: int = 24) -> List[Dict]
     return [digest] + keep
 
 
+def _format_background_review_tool_contract(tool_names: set) -> str:
+    """Render the exact runtime whitelist for the review agent."""
+    preferred_order = ("memory", "skills_list", "skill_view", "skill_manage")
+    ordered = [name for name in preferred_order if name in tool_names]
+    ordered.extend(sorted(set(tool_names) - set(ordered)))
+    return ", ".join(f"`{name}`" for name in ordered)
+
+
 # Review-prompt strings — used by ``spawn_background_review_thread`` to build
 # the user-message that the forked review agent receives.  AIAgent exposes
 # them as class attributes (``_MEMORY_REVIEW_PROMPT`` etc.) for back-compat;
@@ -232,6 +241,11 @@ def _digest_history(messages_snapshot: List[Dict], tail: int = 24) -> List[Dict]
     "the skill can invoke directly (verification scripts, fixture "
     "generators, deterministic probes, anything the agent should run "
     "rather than hand-type each time).\n"
+    "     BEFORE adding any support file, you MUST call "
+    "skill_view(name='<umbrella>') and read the full current SKILL.md "
+    "body. Name + description from skills_list is not enough. If the "
+    "full SKILL.md shows the reference does not fit, duplicates "
+    "existing guidance, or belongs elsewhere, do not add it.\n"
     "     Add support files via skill_manage action=write_file with "
     "file_path starting 'references/', 'templates/', or 'scripts/'. "
     "The umbrella's SKILL.md should gain a one-line pointer to any "
@@ -341,7 +355,11 @@ def _digest_history(messages_snapshot: List[Dict], tail: int = 24) -> List[Dict]
     "notes) written concise and task-focused; `templates/<name>.<ext>` "
     "for starter files meant to be copied and modified; "
     "`scripts/<name>.<ext>` for statically re-runnable actions "
-    "(verification, fixture generators, probes). Add a one-line "
+    "(verification, fixture generators, probes). BEFORE adding any "
+    "support file, call skill_view(name='<umbrella>') and read the "
+    "full current SKILL.md body; name + description from skills_list "
+    "is not enough. If the full SKILL.md shows it does not fit or "
+    "would duplicate existing guidance, do not add it. Add a one-line "
     "pointer in SKILL.md so future agents find them.\n"
     "  4. CREATE A NEW CLASS-LEVEL UMBRELLA when nothing exists. "
     "Name at the class level — NOT a PR number, error string, "
@@ -862,11 +880,12 @@ def _bg_review_auto_deny(command, description, **kwargs):
                     quiet_mode=True,
                 )
             }
+            allowed_tools = _format_background_review_tool_contract(review_whitelist)
             set_thread_tool_whitelist(
                 review_whitelist,
                 deny_msg_fmt=(
                     "Background review denied non-whitelisted tool: "
-                    "{tool_name}. Only memory/skill tools are allowed."
+                    f"{{tool_name}}. Only these tools are allowed: {allowed_tools}."
                 ),
             )
             try:
@@ -887,9 +906,10 @@ def _bg_review_auto_deny(command, description, **kwargs):
                 review_agent.run_conversation(
                     user_message=(
                         prompt
-                        + "\n\nYou can only call memory and skill "
-                        "management tools. Other tools will be denied "
-                        "at runtime — do not attempt them."
+                        + "\n\nBackground review can only call these "
+                        f"tools: {allowed_tools}. Use `skill_view` to "
+                        "read skills before `skill_manage` writes. "
+                        "Other tools will be denied at runtime."
                     ),
                     conversation_history=_review_history,
                 )

tests/run_agent/test_background_review_toolset_restriction.py

@@ -133,8 +133,54 @@ def _no_init(self, *args, **kwargs):
     assert "delegate_task" not in whitelist
     assert "web_search" not in whitelist
     assert "execute_code" not in whitelist
+    deny_msg = captured["deny_msg_fmt"]
+    assert "Only these tools are allowed" in deny_msg
+    assert "`memory`" in deny_msg
+    assert "`skills_list`" in deny_msg
+    assert "`skill_view`" in deny_msg
+    assert "`skill_manage`" in deny_msg
+    assert "management tools" not in deny_msg
 
 
+def test_background_review_agent_prompt_names_exact_allowed_tools():
+    """The review fork prompt must enumerate exact tools, not vague categories."""
+    import run_agent
+
+    captured = {}
+
+    def _no_init(self, *args, **kwargs):
+        return None
+
+    def _capture_run_conversation(self, *args, **kwargs):
+        captured["user_message"] = kwargs.get("user_message", "")
+        self._session_messages = []
+        return {"final_response": "Nothing to save."}
+
+    def _noop(self, *args, **kwargs):
+        return None
+
+    agent = _make_agent_stub(run_agent.AIAgent)
+
+    with patch.object(run_agent.AIAgent, "__init__", _no_init), \
+         patch.object(run_agent.AIAgent, "run_conversation", _capture_run_conversation), \
+         patch.object(run_agent.AIAgent, "shutdown_memory_provider", _noop), \
+         patch.object(run_agent.AIAgent, "close", _noop), \
+         patch("threading.Thread", _SyncThread):
+        agent._spawn_background_review(
+            messages_snapshot=[],
+            review_memory=True,
+            review_skills=True,
+        )
+
+    prompt = captured["user_message"]
+    assert "Background review can only call these tools" in prompt
+    assert "`memory`" in prompt
+    assert "`skills_list`" in prompt
+    assert "`skill_view`" in prompt
+    assert "`skill_manage`" in prompt
+    assert "Use `skill_view` to read skills before `skill_manage` writes" in prompt
+    assert "management tools" not in prompt
+
 
 
 

tests/run_agent/test_review_prompt_class_first.py

@@ -51,6 +51,17 @@ def test_skill_review_prompt_treats_user_corrections_as_skill_signal():
     )
 
 
+def test_skill_review_prompt_requires_full_skill_read_before_support_file():
+    """Support files must require reading the umbrella SKILL.md first (#58475)."""
+    prompt = AIAgent._SKILL_REVIEW_PROMPT
+    lower = prompt.lower()
+    assert "before adding any support file" in lower
+    assert "skill_view(name='<umbrella>')" in prompt
+    assert "full current SKILL.md" in prompt
+    assert "name + description" in lower
+    assert "not enough" in lower
+
+
 
 
 
@@ -76,6 +87,17 @@ def test_combined_review_prompt_has_memory_section():
     assert "memory tool" in prompt
 
 
+def test_combined_review_prompt_requires_full_skill_read_before_support_file():
+    """Combined review must carry the same support-file read-first contract."""
+    prompt = AIAgent._COMBINED_REVIEW_PROMPT
+    lower = prompt.lower()
+    assert "before adding any support file" in lower
+    assert "skill_view(name='<umbrella>')" in prompt
+    assert "full current SKILL.md" in prompt
+    assert "name + description" in lower
+    assert "not enough" in lower
+
+
 
 
 

tests/tools/test_skill_manager_tool.py

@@ -863,6 +863,11 @@ def test_background_review_support_file_overwrite_requires_that_file_read(self,
             ))
             assert blocked["success"] is False
             assert blocked.get("_read_before_write_required") is True
+            assert "You must load the current references/workflow.md content" in blocked["error"]
+            assert (
+                "skill_view(name='reviewed', file_path='references/workflow.md')"
+                in blocked["error"]
+            )
 
             assert json.loads(skill_view("reviewed", "references/workflow.md"))["success"] is True
             allowed = json.loads(skill_manage(
@@ -874,3 +879,47 @@ def test_background_review_support_file_overwrite_requires_that_file_read(self,
             assert allowed["success"] is True, allowed
 
         _reset_background_review_read_marks()
+
+    @pytest.mark.parametrize(
+        "file_path",
+        [
+            "references/new-learning.md",
+            "templates/starter.txt",
+            "scripts/check.py",
+        ],
+    )
+    def test_background_review_new_support_file_requires_skill_md_read_first(
+        self, tmp_path, monkeypatch, file_path
+    ):
+        from tools.skills_tool import skill_view
+        from tools.skill_manager_tool import _reset_background_review_read_marks
+
+        _reset_background_review_read_marks()
+        with _curator_pass(tmp_path, monkeypatch=monkeypatch):
+            _create_curator_skill("reviewed", _skill_content("reviewed"))
+            target = tmp_path / ".hermes" / "skills" / "reviewed" / file_path
+
+            blocked = json.loads(skill_manage(
+                action="write_file",
+                name="reviewed",
+                file_path=file_path,
+                file_content="new support detail\n",
+            ))
+            assert blocked["success"] is False
+            assert blocked.get("_read_before_write_required") is True
+            assert "You must load the current SKILL.md content" in blocked["error"]
+            assert "skill_view(name='reviewed')" in blocked["error"]
+            assert file_path in blocked["error"]
+            assert not target.exists()
+
+            assert json.loads(skill_view("reviewed"))["success"] is True
+            allowed = json.loads(skill_manage(
+                action="write_file",
+                name="reviewed",
+                file_path=file_path,
+                file_content="new support detail\n",
+            ))
+            assert allowed["success"] is True, allowed
+            assert target.read_text(encoding="utf-8") == "new support detail\n"
+
+        _reset_background_review_read_marks()

tools/skill_manager_tool.py

@@ -62,9 +62,10 @@ def mark_background_review_skill_read(path: Path) -> None:
 
     The autonomous review fork is allowed to evolve skills, but it must not
     patch or rewrite content it has only inferred from the transcript.  The
-    skill_view tool calls this after returning file content to the model; write
+    skill_view tool calls this after returning file content to the model. Write
     paths below require the corresponding target path to be present when the
-    current origin is ``background_review``.
+    current origin is ``background_review``; creating a new support file under
+    an existing skill requires the umbrella SKILL.md to have been read first.
     """
     try:
         from tools.skill_provenance import is_background_review
@@ -426,6 +427,7 @@ def _background_review_read_before_write_guard(
     target: Path,
     action: str,
     file_label: str,
+    intent: Optional[str] = None,
 ) -> Optional[Dict[str, Any]]:
     """Require review forks to load the exact target before mutating it."""
     try:
@@ -438,14 +440,25 @@ def _background_review_read_before_write_guard(
     if _background_review_has_read(target):
         return None
 
+    view_call = (
+        f"skill_view(name={name!r})"
+        if file_label == "SKILL.md"
+        else f"skill_view(name={name!r}, file_path={file_label!r})"
+    )
+    action_phrase = intent or {
+        "edit": "replace it",
+        "patch": "patch it",
+        "write_file": "write it",
+        "remove_file": "remove it",
+    }.get(action, f"{action} it")
+
     return {
         "success": False,
         "error": (
-            f"Refusing background curator {action} for skill '{name}': "
-            f"the current {file_label} content has not been loaded in this "
-            "review turn. Call skill_view(name) for SKILL.md, or "
-            "skill_view(name, file_path=...) for a supporting file, then "
-            "retry the write using the content just returned."
+            f"You must load the current {file_label} content before you can "
+            f"{action_phrase}. Call {view_call} first, then retry the "
+            f"{action}. Read-before-write ensures you see the current content "
+            "before modifying it."
         ),
         "_read_before_write_required": True,
     }
@@ -1308,6 +1321,17 @@ def _write_file(name: str, file_path: str, file_content: str) -> Dict[str, Any]:
         )
         if read_guard:
             return read_guard
+    else:
+        skill_md = existing["path"] / "SKILL.md"
+        read_guard = _background_review_read_before_write_guard(
+            name,
+            skill_md,
+            "write_file",
+            "SKILL.md",
+            intent=f"add {file_path!r} under this skill",
+        )
+        if read_guard:
+            return read_guard
     target.parent.mkdir(parents=True, exist_ok=True)
     # Back up for rollback
     original_content = target.read_text(encoding="utf-8") if target.exists() else None