diff --git a/agent/background_review.py b/agent/background_review.py index cc7641a99c..1c041ab345 100644 --- a/agent/background_review.py +++ b/agent/background_review.py @@ -480,8 +480,10 @@ _SKILL_REVIEW_PROMPT = ( " 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 — but only if it is " + "new learning, PATCH that one first (re-load it with skill_view " + "during this review — see Read-before-write below). It is the " + "skill that was in play, so it's the right one to extend — but " + "only if it is " "curator-managed. Bundled, hub, pinned, and user-owned skills are " "off-limits to you no matter how relevant (see Protected skills " "below); for those, fall through to the next option.\n" @@ -514,6 +516,18 @@ _SKILL_REVIEW_PROMPT = ( "codename, library-alone name, or 'fix-X / debug-Y / audit-Z-today' " "session artifact. If the proposed name only makes sense for " "today's task, it's wrong — fall back to (1), (2), or (3).\n\n" + "Read-before-write (ENFORCED — skill_manage refuses otherwise): " + "before you patch or edit an existing skill's SKILL.md, call " + "skill_view(name) for that skill during this review. Before you " + "overwrite or remove an EXISTING supporting file, call " + "skill_view(name, file_path=...) for that exact file. Content " + "quoted earlier in the conversation transcript does NOT count — " + "the guard requires a fresh load within this review, and your " + "write must be based on what skill_view just returned. Creating " + "a brand-new skill or adding a NEW supporting file needs no " + "prior read. If a write is refused with a read-before-write " + "error, call skill_view for the named target once and retry the " + "write once; do not loop.\n\n" "User-preference embedding (important): when the user expressed a " "style/format/workflow preference, the update belongs in the " "SKILL.md body, not just in memory. Memory captures 'who the user " @@ -600,7 +614,9 @@ _COMBINED_REVIEW_PROMPT = ( "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; " + "of them covers the learning, PATCH it first (re-load it with " + "skill_view during this review — see Read-before-write below). " + "It was in play; " "it's the right place — provided it is curator-managed. Protected " "and user-owned skills are off-limits however relevant; fall " "through when one of those is the best fit.\n" @@ -620,6 +636,15 @@ _COMBINED_REVIEW_PROMPT = ( "codename, library-alone name, or 'fix-X / debug-Y' session " "artifact. If the name only fits today's task, fall back to (1), " "(2), or (3).\n\n" + "Read-before-write (ENFORCED — skill_manage refuses otherwise): " + "before patching or editing an existing skill's SKILL.md, call " + "skill_view(name) during this review; before overwriting or " + "removing an EXISTING supporting file, call skill_view(name, " + "file_path=...) for that exact file. Content quoted earlier in " + "the transcript does NOT count — base the write on what " + "skill_view just returned. New skills and NEW supporting files " + "need no prior read. On a read-before-write refusal: view the " + "named target once, retry the write once, do not loop.\n\n" "User-preference embedding: when the user complains about how " "you handled a task, update the skill that governs that task — " "memory alone isn't enough. Memory says 'who the user is and " diff --git a/tests/run_agent/test_review_prompt_class_first.py b/tests/run_agent/test_review_prompt_class_first.py index bf0555b8e7..e47b6e1977 100644 --- a/tests/run_agent/test_review_prompt_class_first.py +++ b/tests/run_agent/test_review_prompt_class_first.py @@ -144,6 +144,44 @@ def test_combined_review_prompt_rejects_unresolved_failures(): _assert_unresolved_failure_guidance(AIAgent._COMBINED_REVIEW_PROMPT, "_COMBINED_REVIEW_PROMPT") +def _assert_read_before_write_guidance(prompt: str, label: str) -> None: + """Both review prompts must teach the enforced read-before-write handshake. + + The skill_manage guard refuses patch/edit of an existing SKILL.md (and + overwrite/remove of an existing support file) unless the exact target was + loaded via skill_view during the review. Without prompt guidance the model + walks into the refusal and burns iterations retrying (#62397). + """ + lower = prompt.lower() + assert "read-before-write" in lower, f"{label}: must name the read-before-write rule" + assert "skill_view(name)" in prompt, ( + f"{label}: must give the exact SKILL.md pre-read call" + ) + assert "file_path=..." in prompt, ( + f"{label}: must give the support-file pre-read form" + ) + # Scope: only EXISTING targets need a pre-read; new creations are exempt. + assert "new" in lower and "no prior read" in lower, ( + f"{label}: must exempt new skills / new support files from the pre-read" + ) + # Transcript quotes must not be treated as satisfying the guard. + assert "does not count" in lower or "does NOT count" in prompt or "not satisfy" in lower, ( + f"{label}: must say transcript-quoted content doesn't satisfy the guard" + ) + # Bounded recovery: one view + one retry, never a loop. + assert "do not loop" in lower, ( + f"{label}: must bound refusal recovery to a single retry" + ) + + +def test_skill_review_prompt_teaches_read_before_write(): + _assert_read_before_write_guidance(AIAgent._SKILL_REVIEW_PROMPT, "_SKILL_REVIEW_PROMPT") + + +def test_combined_review_prompt_teaches_read_before_write(): + _assert_read_before_write_guidance(AIAgent._COMBINED_REVIEW_PROMPT, "_COMBINED_REVIEW_PROMPT") + +