fix(agent): document read-before-write handshake in background-review prompt (#62397)

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/<topic>.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 "

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
+        )