From 1ee30352ca60f79d984bba62ba1ffd2716c36924 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Sat, 29 Aug 2026 18:25:54 -0700 Subject: [PATCH] =?UTF-8?q?fix:=20background=20review=20can=20now=20read?= =?UTF-8?q?=20skills=20before=20patching=20=E2=80=94=20denial=20storm=20en?= =?UTF-8?q?ded,=20cache=20parity=20intact=20(#61521,=20#39996)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 | 22 +++++- ...t_background_review_toolset_restriction.py | 69 ++++++++++++++++++- tools/file_tools.py | 20 +++++- 3 files changed, 108 insertions(+), 3 deletions(-) diff --git a/agent/background_review.py b/agent/background_review.py index 6ff57f6f16..72f8fe3a29 100644 --- a/agent/background_review.py +++ b/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: diff --git a/tests/run_agent/test_background_review_toolset_restriction.py b/tests/run_agent/test_background_review_toolset_restriction.py index ab39d87eda..bc45bdfba2 100644 --- a/tests/run_agent/test_background_review_toolset_restriction.py +++ b/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) diff --git a/tools/file_tools.py b/tools/file_tools.py index eef53d0895..ddcb060301 100644 --- a/tools/file_tools.py +++ b/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(