diff --git a/agent/background_review.py b/agent/background_review.py index bf78f679236db..99cabf3f1aa50 100644 --- a/agent/background_review.py +++ b/agent/background_review.py @@ -178,12 +178,39 @@ def _digest_history(messages_snapshot: List[Dict], tail: int = 24) -> List[Dict] "If nothing is worth saving, just say 'Nothing to save.' and stop." ) +_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" + " 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" +) + _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" - "Target shape of the library: CLASS-LEVEL skills, each with a rich " + + _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" @@ -208,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" @@ -292,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" @@ -308,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 new file mode 100644 index 0000000000000..f511c6b916f1f --- /dev/null +++ b/tests/run_agent/test_background_review_prompt_read_before_write.py @@ -0,0 +1,119 @@ +"""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 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 + +import pytest + + +@pytest.fixture +def skill_review_prompt() -> str: + from agent.background_review import _SKILL_REVIEW_PROMPT + + return _SKILL_REVIEW_PROMPT + + +@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 + + +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, review_prompt): + assert "skill_view" in review_prompt + + 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, review_prompt): + assert "_read_before_write_required" in review_prompt + + 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, 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) + + 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: + 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 + ) + + +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 + )