From 1b05a1e914361bd8d94a0d657dcfcdfcf5f78a81 Mon Sep 17 00:00:00 2001 From: hermes-agent Date: Sat, 11 Jul 2026 20:27:34 +0700 Subject: [PATCH 1/2] fix(agent): document read-before-write handshake in background-review prompt (#62397) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The background self-improvement review fork was instructed to PATCH loaded skills, but the prompt never told it about the read-before-write handshake required by tools/skill_manager_tool.py's _background_review_read_before_write_guard: Guard: refuses the write unless skill_view(target) was called in the current review turn. Prompt: told the fork to patch but never told it to skill_view first. Net effect: the fork tries to patch, gets refused with _read_before_write_required, and the correction is silently dropped. Real users hit this on kanban-orchestrator, home-assistant, and other skills (errors.log pattern). Because the fork is background, the failure is silent to the user — the advertised "improves skills during use" loop degrades quietly. Fix: prepend a READ-BEFORE-WRITE HANDSHAKE section to _SKILL_REVIEW_PROMPT that explains the guard's failure mode and the exact retry contract: - skill_view(name) for SKILL.md edits - skill_view(name, file_path='references/.md') for support files (the guard tracks reads per-path, not per-skill — easy to miss) - On _read_before_write_required, call skill_view and reissue the write with the content just returned (the guard compares against the in-memory version, so re-deriving from memory re-triggers the refusal) - First-time creations (no existing file) are fine without a read Why fix the prompt and not the guard? The guard is doing the right thing (it prevents accidental overwrites when the fork never saw the file). The prompt is what tells the LLM when the guard will refuse — fixing the prompt removes the silent-drop failure mode without weakening the guard. Both layers need each other; the prompt is where the contract gets documented for the LLM. 7 new tests in tests/run_agent/test_background_review_prompt_read_before_write.py pin every clause of the new contract so a future prompt edit can't silently drop the skill_view handshake again. The two belt-and-suspenders tests assert the existing prompt sections (preference order, protected skills) didn't regress. Verified locally: 7/7 new + 14/14 existing (test_background_review.py, test_background_review_cache_parity.py) = 21/21 passed. Closes #62397 --- agent/background_review.py | 23 ++++ ...kground_review_prompt_read_before_write.py | 121 ++++++++++++++++++ 2 files changed, 144 insertions(+) create mode 100644 tests/run_agent/test_background_review_prompt_read_before_write.py diff --git a/agent/background_review.py b/agent/background_review.py index bf78f679236d..dbb3df73500f 100644 --- a/agent/background_review.py +++ b/agent/background_review.py @@ -183,6 +183,29 @@ def _digest_history(messages_snapshot: List[Dict], tail: int = 24) -> List[Dict] "ACTIVE — most sessions produce at least one skill update, even if " "small. A pass that does nothing is a missed learning opportunity, " "not a neutral outcome.\n\n" + "READ-BEFORE-WRITE HANDSHAKE (load the target before patching — #62397):\n" + " Every background-review write goes through a guard that refuses the\n" + " mutation when the exact target file wasn't loaded via skill_view in\n" + " this review turn. The guard's failure mode is a hard refusal — the\n" + " write is silently dropped and the learning is lost. To avoid that:\n" + " • Before any skill_manage(action='patch' | 'edit' | 'write_file' |\n" + " 'delete' | 'remove_file'), call skill_view FIRST in this turn.\n" + " • For SKILL.md: skill_view(name) returns the current body.\n" + " • For a support file (under references/, templates/, scripts/):\n" + " skill_view(name, file_path='references/.md') — the file_path\n" + " form is required because the guard tracks reads per-path, not\n" + " per-skill.\n" + " • If the guard returns {\"_read_before_write_required\": true}, call\n" + " skill_view for the target, then REISSUE the write using the\n" + " content just returned by skill_view (do not re-derive it from\n" + " memory — the guard compares against the in-memory version).\n" + " • This applies even when you're CREATING a new support file: the\n" + " guard refuses the write whenever the file already exists, so a\n" + " single re-edit must be preceded by a skill_view of the existing\n" + " content. First-time creations (no existing file) are fine\n" + " without a read.\n" + " Skipping this handshake is the #1 cause of \"the background review\n" + " ran but nothing got patched\" — don't skip it.\n\n" "Target shape of the library: CLASS-LEVEL skills, each with a rich " "SKILL.md and a `references/` directory for session-specific detail. " "Not a long flat list of narrow one-session-one-skill entries. This " diff --git a/tests/run_agent/test_background_review_prompt_read_before_write.py b/tests/run_agent/test_background_review_prompt_read_before_write.py new file mode 100644 index 000000000000..b24b0e293368 --- /dev/null +++ b/tests/run_agent/test_background_review_prompt_read_before_write.py @@ -0,0 +1,121 @@ +"""Regression tests for #62397: background review fork must call skill_view +before skill_manage, otherwise the read-before-write guard refuses the +patch and the learning is silently dropped. + +Root cause: a mismatch between two shipped components. +1. The guard (tools/skill_manager_tool.py) refuses background-curator + writes when the exact target wasn't loaded via skill_view in this + review turn. +2. The prompt (_SKILL_REVIEW_PROMPT in agent/background_review.py) + told the fork to PATCH loaded skills but never told it about the + skill_view handshake required for support files (templates/, + scripts/, references/) or for skills that weren't loaded via + /skill-name but were located via skills_list. + +Fix: explicit instruction in the prompt so the fork +(a) knows to call skill_view before any write_file / patch / edit, +(b) knows the retry contract when _read_before_write_required comes + back, and +(c) understands support-file writes need skill_view(name, file_path=...). + +Tests below assert the prompt carries each contract clause so a future +prompt edit can't silently drop them again. +""" + +from __future__ import annotations + +import pytest + + +@pytest.fixture +def skill_review_prompt() -> str: + from agent.background_review import _SKILL_REVIEW_PROMPT + + return _SKILL_REVIEW_PROMPT + + +class TestSkillReviewPromptReadBeforeWriteContract: + """The background-review fork must be told, in plain language, that: + + 1. ANY write to a skill file (SKILL.md or a support file under + references/ / templates/ / scripts/) requires a prior skill_view + call in THIS review turn. + 2. If the guard returns _read_before_write_required, the fork must + call skill_view(name) (or skill_view(name, file_path=...) for a + support file) and retry the write with the returned content. + 3. This applies even when the skill is being CREATED — the guard + fires for patch/edit/write_file/delete/remove_file. + """ + + def test_prompt_mentions_skill_view_before_write(self, skill_review_prompt): + # The fork must be told to load the target via skill_view BEFORE + # any patch / write_file / edit action. + assert "skill_view" in skill_review_prompt, ( + "Background review prompt must reference skill_view so the " + "fork loads targets before patching" + ) + + def test_prompt_explicitly_requires_skill_view_before_patch(self, skill_review_prompt): + # The contract must be stated explicitly (not buried in the + # middle of an unrelated paragraph where the LLM can miss it). + # We check for any of the common phrasings. + lower = skill_review_prompt.lower() + explicit_phrases = [ + "before any patch", + "before patching", + "call skill_view", + "skill_view(name)", + "before any write", + ] + assert any(p in lower for p in explicit_phrases), ( + "Prompt must explicitly require skill_view before patch/write. " + f"Searched: {explicit_phrases}" + ) + + def test_prompt_explains_read_before_write_retry_contract(self, skill_review_prompt): + """If the guard returns _read_before_write_required, the fork must + know to call skill_view and retry. Without this clause the fork + silently drops the learning.""" + assert "_read_before_write_required" in skill_review_prompt, ( + "Prompt must surface the exact guard signal name so the fork " + "knows what to do when it sees it" + ) + + def test_prompt_documents_support_file_read_pattern(self, skill_review_prompt): + """support_file writes need skill_view(name, file_path=...) — the + fork can't just call skill_view(name) for templates/scripts/ + references/* writes. Without this clause every support-file write + fails the guard.""" + assert "file_path" in skill_review_prompt, ( + "Prompt must show the skill_view(name, file_path=...) form " + "for support-file writes (references/, templates/, scripts/)" + ) + + def test_prompt_says_use_returned_content_on_retry(self, skill_review_prompt): + """On retry, the fork must reuse the content skill_view just + returned (not re-derive it from memory) so the guard sees a + consistent in-memory version.""" + lower = skill_review_prompt.lower() + reuse_phrases = [ + "content just returned", + "returned content", + "content returned by skill_view", + "returned by skill_view", + ] + assert any(p in lower for p in reuse_phrases), ( + "Prompt must tell the fork to reuse content returned by " + "skill_view on retry. Searched: " + str(reuse_phrases) + ) + + +class TestSkillReviewPromptExistingContract: + """Belt-and-suspenders: the prompt's existing shape (target library, + preference order, etc.) must NOT regress with our edit.""" + + def test_prompt_still_lists_preference_order(self, skill_review_prompt): + assert "UPDATE A CURRENTLY-LOADED SKILL" in skill_review_prompt + assert "ADD A SUPPORT FILE" in skill_review_prompt or "support file" in skill_review_prompt.lower() + assert "CREATE A NEW CLASS-LEVEL" in skill_review_prompt + + def test_prompt_still_mentions_protected_skills(self, skill_review_prompt): + assert "DO NOT edit" in skill_review_prompt or "Protected skills" in skill_review_prompt From 9c70704f4e2ac1755f9624d0c1d4ed3a07254025 Mon Sep 17 00:00:00 2001 From: vubah Date: Sat, 11 Jul 2026 22:46:56 +0700 Subject: [PATCH 2/2] fix(agent): teach combined review prompt the read-before-write guard Share the handshake contract across skill-only and combined memory+skills prompts. Also require a fresh skill_view on the currently-loaded skill path so both spawn_background_review_thread prompt routes survive the guard. --- agent/background_review.py | 30 ++-- ...kground_review_prompt_read_before_write.py | 130 +++++++++--------- 2 files changed, 83 insertions(+), 77 deletions(-) diff --git a/agent/background_review.py b/agent/background_review.py index dbb3df73500f..99cabf3f1aa5 100644 --- a/agent/background_review.py +++ b/agent/background_review.py @@ -178,11 +178,7 @@ def _digest_history(messages_snapshot: List[Dict], tail: int = 24) -> List[Dict] "If nothing is worth saving, just say 'Nothing to save.' and stop." ) -_SKILL_REVIEW_PROMPT = ( - "Review the conversation above and update the skill library. Be " - "ACTIVE — most sessions produce at least one skill update, even if " - "small. A pass that does nothing is a missed learning opportunity, " - "not a neutral outcome.\n\n" +_READ_BEFORE_WRITE_HANDSHAKE = ( "READ-BEFORE-WRITE HANDSHAKE (load the target before patching — #62397):\n" " Every background-review write goes through a guard that refuses the\n" " mutation when the exact target file wasn't loaded via skill_view in\n" @@ -206,7 +202,15 @@ def _digest_history(messages_snapshot: List[Dict], tail: int = 24) -> List[Dict] " without a read.\n" " Skipping this handshake is the #1 cause of \"the background review\n" " ran but nothing got patched\" — don't skip it.\n\n" - "Target shape of the library: CLASS-LEVEL skills, each with a rich " +) + +_SKILL_REVIEW_PROMPT = ( + "Review the conversation above and update the skill library. Be " + "ACTIVE — most sessions produce at least one skill update, even if " + "small. A pass that does nothing is a missed learning opportunity, " + "not a neutral outcome.\n\n" + + _READ_BEFORE_WRITE_HANDSHAKE + + "Target shape of the library: CLASS-LEVEL skills, each with a rich " "SKILL.md and a `references/` directory for session-specific detail. " "Not a long flat list of narrow one-session-one-skill entries. This " "shapes HOW you update, not WHETHER you update.\n\n" @@ -231,8 +235,10 @@ def _digest_history(messages_snapshot: List[Dict], tail: int = 24) -> List[Dict] " 1. UPDATE A CURRENTLY-LOADED SKILL. Look back through the " "conversation for skills the user loaded via /skill-name or you " "read via skill_view. If any of them covers the territory of the " - "new learning, PATCH that one first. It is the skill that was in " - "play, so it's the right one to extend.\n" + "new learning, PATCH that one first (after re-loading it with " + "skill_view in this turn — see the read-before-write handshake " + "above). It is the skill that was in play, so it's the right one " + "to extend.\n" " 2. UPDATE AN EXISTING UMBRELLA (via skills_list + skill_view). " "If no loaded skill fits but an existing class-level skill does, " "patch it. Add a subsection, a pitfall, or broaden a trigger.\n" @@ -315,7 +321,8 @@ def _digest_history(messages_snapshot: List[Dict], tail: int = 24) -> List[Dict] "**Skills**: how to do this class of task. Be ACTIVE — most " "sessions produce at least one skill update. A pass that does " "nothing is a missed learning opportunity, not a neutral outcome.\n\n" - "Target shape of the skill library: CLASS-LEVEL skills with a rich " + + _READ_BEFORE_WRITE_HANDSHAKE + + "Target shape of the skill library: CLASS-LEVEL skills with a rich " "SKILL.md and a `references/` directory for session-specific detail. " "Not a long flat list of narrow one-session-one-skill entries.\n\n" "Signals that warrant a skill update (any one is enough):\n" @@ -331,8 +338,9 @@ def _digest_history(messages_snapshot: List[Dict], tail: int = 24) -> List[Dict] "Preference order for skills — pick the earliest that fits:\n" " 1. UPDATE A CURRENTLY-LOADED SKILL. Check what skills were " "loaded via /skill-name or skill_view in the conversation. If one " - "of them covers the learning, PATCH it first. It was in play; " - "it's the right place.\n" + "of them covers the learning, PATCH it first (after re-loading " + "it with skill_view in this turn — see the read-before-write " + "handshake above). It was in play; it's the right place.\n" " 2. UPDATE AN EXISTING UMBRELLA (skills_list + skill_view to " "find the right one). Patch it.\n" " 3. ADD A SUPPORT FILE under an existing umbrella via " diff --git a/tests/run_agent/test_background_review_prompt_read_before_write.py b/tests/run_agent/test_background_review_prompt_read_before_write.py index b24b0e293368..f511c6b916f1 100644 --- a/tests/run_agent/test_background_review_prompt_read_before_write.py +++ b/tests/run_agent/test_background_review_prompt_read_before_write.py @@ -6,20 +6,14 @@ 1. The guard (tools/skill_manager_tool.py) refuses background-curator writes when the exact target wasn't loaded via skill_view in this review turn. -2. The prompt (_SKILL_REVIEW_PROMPT in agent/background_review.py) - told the fork to PATCH loaded skills but never told it about the - skill_view handshake required for support files (templates/, - scripts/, references/) or for skills that weren't loaded via - /skill-name but were located via skills_list. - -Fix: explicit instruction in the prompt so the fork -(a) knows to call skill_view before any write_file / patch / edit, -(b) knows the retry contract when _read_before_write_required comes - back, and -(c) understands support-file writes need skill_view(name, file_path=...). - -Tests below assert the prompt carries each contract clause so a future -prompt edit can't silently drop them again. +2. The prompts (_SKILL_REVIEW_PROMPT and _COMBINED_REVIEW_PROMPT in + agent/background_review.py) told the fork to PATCH loaded skills but + never told it about the skill_view handshake required for support + files or for skills that weren't loaded via /skill-name. + +Fix: explicit instruction in BOTH prompts (skill-only and combined +memory+skills routes) so the fork knows the handshake and the retry +contract when `_read_before_write_required` comes back. """ from __future__ import annotations @@ -34,88 +28,92 @@ def skill_review_prompt() -> str: return _SKILL_REVIEW_PROMPT -class TestSkillReviewPromptReadBeforeWriteContract: - """The background-review fork must be told, in plain language, that: +@pytest.fixture +def combined_review_prompt() -> str: + from agent.background_review import _COMBINED_REVIEW_PROMPT + + return _COMBINED_REVIEW_PROMPT + + +@pytest.fixture(params=["skill", "combined"]) +def review_prompt(request, skill_review_prompt, combined_review_prompt) -> str: + return skill_review_prompt if request.param == "skill" else combined_review_prompt - 1. ANY write to a skill file (SKILL.md or a support file under - references/ / templates/ / scripts/) requires a prior skill_view - call in THIS review turn. - 2. If the guard returns _read_before_write_required, the fork must - call skill_view(name) (or skill_view(name, file_path=...) for a - support file) and retry the write with the returned content. - 3. This applies even when the skill is being CREATED — the guard - fires for patch/edit/write_file/delete/remove_file. + +class TestReviewPromptReadBeforeWriteContract: + """Both prompt-selection paths must carry the handshake. + + ``spawn_background_review_thread`` picks ``_COMBINED_REVIEW_PROMPT`` + when memory and skills reviews fire together, and ``_SKILL_REVIEW_PROMPT`` + when only skills fire. Covering only one leaves the other route broken. """ - def test_prompt_mentions_skill_view_before_write(self, skill_review_prompt): - # The fork must be told to load the target via skill_view BEFORE - # any patch / write_file / edit action. - assert "skill_view" in skill_review_prompt, ( - "Background review prompt must reference skill_view so the " - "fork loads targets before patching" - ) + def test_prompt_mentions_skill_view_before_write(self, review_prompt): + assert "skill_view" in review_prompt - def test_prompt_explicitly_requires_skill_view_before_patch(self, skill_review_prompt): - # The contract must be stated explicitly (not buried in the - # middle of an unrelated paragraph where the LLM can miss it). - # We check for any of the common phrasings. - lower = skill_review_prompt.lower() + def test_prompt_explicitly_requires_skill_view_before_patch(self, review_prompt): + lower = review_prompt.lower() explicit_phrases = [ "before any patch", "before patching", "call skill_view", "skill_view(name)", "before any write", + "re-loading", ] assert any(p in lower for p in explicit_phrases), ( "Prompt must explicitly require skill_view before patch/write. " f"Searched: {explicit_phrases}" ) - def test_prompt_explains_read_before_write_retry_contract(self, skill_review_prompt): - """If the guard returns _read_before_write_required, the fork must - know to call skill_view and retry. Without this clause the fork - silently drops the learning.""" - assert "_read_before_write_required" in skill_review_prompt, ( - "Prompt must surface the exact guard signal name so the fork " - "knows what to do when it sees it" - ) + def test_prompt_explains_read_before_write_retry_contract(self, review_prompt): + assert "_read_before_write_required" in review_prompt - def test_prompt_documents_support_file_read_pattern(self, skill_review_prompt): - """support_file writes need skill_view(name, file_path=...) — the - fork can't just call skill_view(name) for templates/scripts/ - references/* writes. Without this clause every support-file write - fails the guard.""" - assert "file_path" in skill_review_prompt, ( - "Prompt must show the skill_view(name, file_path=...) form " - "for support-file writes (references/, templates/, scripts/)" - ) + def test_prompt_documents_support_file_read_pattern(self, review_prompt): + assert "file_path" in review_prompt - def test_prompt_says_use_returned_content_on_retry(self, skill_review_prompt): - """On retry, the fork must reuse the content skill_view just - returned (not re-derive it from memory) so the guard sees a - consistent in-memory version.""" - lower = skill_review_prompt.lower() + def test_prompt_says_use_returned_content_on_retry(self, review_prompt): + lower = review_prompt.lower() reuse_phrases = [ "content just returned", "returned content", "content returned by skill_view", "returned by skill_view", ] - assert any(p in lower for p in reuse_phrases), ( - "Prompt must tell the fork to reuse content returned by " - "skill_view on retry. Searched: " + str(reuse_phrases) + assert any(p in lower for p in reuse_phrases) + + def test_currently_loaded_skill_path_requires_fresh_skill_view(self, review_prompt): + lower = review_prompt.lower() + assert "update a currently-loaded skill" in lower + assert ( + "re-loading" in lower + or "reload" in lower + or "skill_view in this turn" in lower ) class TestSkillReviewPromptExistingContract: - """Belt-and-suspenders: the prompt's existing shape (target library, - preference order, etc.) must NOT regress with our edit.""" - def test_prompt_still_lists_preference_order(self, skill_review_prompt): assert "UPDATE A CURRENTLY-LOADED SKILL" in skill_review_prompt - assert "ADD A SUPPORT FILE" in skill_review_prompt or "support file" in skill_review_prompt.lower() + assert ( + "ADD A SUPPORT FILE" in skill_review_prompt + or "support file" in skill_review_prompt.lower() + ) assert "CREATE A NEW CLASS-LEVEL" in skill_review_prompt def test_prompt_still_mentions_protected_skills(self, skill_review_prompt): - assert "DO NOT edit" in skill_review_prompt or "Protected skills" in skill_review_prompt + assert ( + "DO NOT edit" in skill_review_prompt + or "Protected skills" in skill_review_prompt + ) + + +class TestCombinedReviewPromptExistingContract: + def test_combined_still_covers_memory_and_skills(self, combined_review_prompt): + assert "**Memory**" in combined_review_prompt + assert "**Skills**" in combined_review_prompt + assert "UPDATE A CURRENTLY-LOADED SKILL" in combined_review_prompt + assert ( + "Protected skills" in combined_review_prompt + or "DO NOT edit" in combined_review_prompt + )