fix: background review can now read skills before patching — denial storm ended, cache parity intact (#61521, #39996)

The self-improvement review fork advertises the parent's full tool schema
(deliberate — tools[] must stay byte-identical for prompt-cache parity)
but denied everything except memory/skill tools at dispatch. Models
naturally reach for read_file to inspect a SKILL.md before patching, got
denied, then attempted a blind skill_manage patch which the
read-before-write guard correctly refused. One deployment logged ~142
denials + ~204 refusals over 2 days: the self-improvement loop ran
continuously but almost never landed a skill patch.

Fix is dispatch-side ONLY — zero request-body change, cache untouched:

- Whitelist read_file + search_files on the review fork (reads are
  side-effect-free). Write tools (write_file/patch/terminal) stay denied:
  autonomous maintenance must go through skill_manage's validation.
- read_file now registers full reads with the review fork's
  read-before-write guard (same as skill_view), so the natural
  read_file -> skill_manage(patch) sequence lands. Partial reads
  (offset>1 / truncated) don't count. No-op outside review forks.
- Self-correcting deny message: names skill_view/skill_manage/memory as
  substitutes so one denial redirects the model instead of a storm
  (the actionable half of #61521's proposal 2).

Rejects #39997's alternative (narrow the advertised schema on local
endpoints): local backends have KV/prefix caches too, and re-prefilling
a large snapshot is most expensive exactly there.

Live A/B (real dispatch path, isolated HERMES_HOME): on main,
read_file DENIED -> patch REFUSED (read-before-write); on this branch,
read_file OK -> patch LANDED. tools[] identical in both.

agent/background_review.py

@@ -1546,11 +1546,31 @@ def _run_review_in_thread(
                     quiet_mode=True,
                 )
             }
+            # Read-only file tools are whitelisted too (#61521, #39996): the
+            # model naturally reaches for read_file/search_files to inspect a
+            # skill before patching it. Denying them caused a per-review
+            # denial storm (~142 denials + ~204 read-before-write refusals
+            # over 2 days on one deployment) that starved the self-improvement
+            # loop — the model never loaded SKILL.md the way the
+            # read-before-write guard requires, so almost no patch landed.
+            # This is a DISPATCH-side change only: the advertised ``tools[]``
+            # stays byte-identical to the parent's, so prompt-cache parity is
+            # untouched. read_file registers the read with the
+            # read-before-write guard (tools/file_tools.py), so a
+            # read_file → skill_manage(patch) sequence now succeeds. Write
+            # tools (write_file/patch/terminal) stay denied — autonomous
+            # maintenance must go through skill_manage's validation, and the
+            # deny message below names that substitute so one denial
+            # redirects the model instead of a storm.
+            review_whitelist |= {"read_file", "search_files"}
             set_thread_tool_whitelist(
                 review_whitelist,
                 deny_msg_fmt=(
                     "Background review denied non-whitelisted tool: "
-                    "{tool_name}. Only memory/skill tools are allowed."
+                    "{tool_name}. Allowed here: skill_view/skills_list/"
+                    "read_file/search_files to read, "
+                    "skill_manage(action='patch'|...) to change skills, and "
+                    "memory for notes. Do not retry {tool_name}."
                 ),
             )
             try:

tests/run_agent/test_background_review_toolset_restriction.py

@@ -127,12 +127,79 @@ def test_background_review_installs_thread_local_whitelist():
     assert "skill_manage" in whitelist
     assert "skill_view" in whitelist
     assert "skills_list" in whitelist
-    # dangerous tools must NOT be in the whitelist
+    # read-only file tools are allowed too (#61521): the model reaches for
+    # read_file to inspect a skill before patching; denying it caused a
+    # per-review denial storm that starved the self-improvement loop.
+    assert "read_file" in whitelist
+    assert "search_files" in whitelist
+    # write/dangerous tools must NOT be in the whitelist
+    assert "write_file" not in whitelist
+    assert "patch" not in whitelist
     assert "terminal" not in whitelist
     assert "send_message" not in whitelist
     assert "delegate_task" not in whitelist
     assert "web_search" not in whitelist
     assert "execute_code" not in whitelist
+    # The deny message must name the correct substitutes so a single denial
+    # redirects the model instead of a 142-denial storm (#61521).
+    deny = captured.get("deny_msg_fmt") or ""
+    assert "skill_manage" in deny
+    assert "skill_view" in deny
+
+
+def test_read_file_registers_background_review_read_mark(tmp_path):
+    """read_file inside a review fork must satisfy the read-before-write guard.
+
+    The whitelist now allows read_file; without this mark, the model would
+    read SKILL.md via read_file and still get "content has not been loaded
+    in this review turn" on the follow-up skill_manage patch (#61521).
+    """
+    from tools.file_tools import read_file_tool
+    from tools.skill_manager_tool import (
+        _background_review_has_read,
+        _reset_background_review_read_marks,
+    )
+    from tools.skill_provenance import (
+        BACKGROUND_REVIEW,
+        reset_current_write_origin,
+        set_current_write_origin,
+    )
+
+    target = tmp_path / "SKILL.md"
+    target.write_text("---\nname: t\n---\nbody\n")
+
+    token = set_current_write_origin(BACKGROUND_REVIEW)
+    try:
+        _reset_background_review_read_marks()
+        assert not _background_review_has_read(target)
+        out = read_file_tool(str(target), task_id="bg-review-test")
+        assert "body" in out
+        assert _background_review_has_read(target), (
+            "full read_file inside a review fork must register with the "
+            "read-before-write guard"
+        )
+
+        # A partial read must NOT satisfy the guard.
+        _reset_background_review_read_marks()
+        read_file_tool(str(target), offset=2, task_id="bg-review-test2")
+        assert not _background_review_has_read(target)
+    finally:
+        reset_current_write_origin(token)
+
+
+def test_read_file_outside_review_does_not_mark(tmp_path):
+    """Foreground reads must not populate the review-fork read set."""
+    from tools.file_tools import read_file_tool
+    from tools.skill_manager_tool import (
+        _background_review_has_read,
+        _reset_background_review_read_marks,
+    )
+
+    target = tmp_path / "SKILL.md"
+    target.write_text("content\n")
+    _reset_background_review_read_marks()
+    read_file_tool(str(target), task_id="fg-test")
+    assert not _background_review_has_read(target)
 
 
 

tools/file_tools.py

@@ -1963,12 +1963,30 @@ def read_file_tool(path: str, offset: int = 1, limit: int = 2000, task_id: str =
         # truncated (large file with more content than limit covered).
         # Outside the _read_tracker_lock so the registry's own locking
         # isn't nested under ours.
+        _partial = (offset > 1) or bool(result_dict.get("truncated"))
         try:
-            _partial = (offset > 1) or bool(result_dict.get("truncated"))
             file_state.record_read(task_id, resolved_str, partial=_partial)
         except Exception:
             logger.debug("file_state.record_read failed", exc_info=True)
 
+        # Background-review read-before-write guard integration (#61521):
+        # when the self-improvement review fork reads a skill file with
+        # read_file (now whitelisted dispatch-side), register the read the
+        # same way skill_view does, so a follow-up
+        # skill_manage(action='patch') on the loaded file is accepted.
+        # A partial read doesn't count — the guard requires the CURRENT
+        # full content to have been seen. No-op outside review forks
+        # (mark_background_review_skill_read gates on is_background_review).
+        if not _partial:
+            try:
+                from tools.skill_manager_tool import mark_background_review_skill_read
+
+                mark_background_review_skill_read(Path(resolved_str))
+            except Exception:
+                logger.debug(
+                    "background-review read-mark failed", exc_info=True
+                )
+
         if count >= 4:
             # Hard block: stop returning content to break the loop
             return tool_error(