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

[Snowdchike]

Summary

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 — 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/<topic>.md') for support files (the guard tracks reads per-path, not per-skill — easy to miss)
• On _read_before_write_required: true, 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.

Files changed

• agent/background_review.py — prepend the read-before-write handshake section to _SKILL_REVIEW_PROMPT. The clause names the exact guard signal (_read_before_write_required) so the fork knows what to do when it sees it.
• tests/run_agent/test_background_review_prompt_read_before_write.py (new, 7 tests) — pin every clause of the new contract so a future prompt edit can't silently drop the skill_view handshake again. Two belt-and-suspenders tests assert the existing prompt sections (preference order, protected skills) didn't regress.

Test plan

pytest tests/run_agent/test_background_review_prompt_read_before_write.py \
       tests/run_agent/test_background_review.py \
       tests/run_agent/test_background_review_cache_parity.py -v

Verified locally: 21/21 passed (7 new + 14 existing).

Closes #62397

[alt-glitch]

This was generated by AI during triage.

Duplicate of #60331 (earliest open PR) — same fix to agent/background_review.py: teaching the background-review fork the read-before-write skill_view-before-skill_manage handshake required by the v2026.7.1 guard (#55906), targeting the same issue #62397. Sibling PRs #62428, #62414, #62400 in this cluster are already deduped to #60331.

[teknium1]

Thanks for tracing the guard/prompt mismatch. Current main confirms the guard is real: tools/skill_manager_tool.py:382-409 refuses unread background-review writes and returns _read_before_write_required.

Problems

• This updates only _SKILL_REVIEW_PROMPT. When both review triggers fire, spawn_background_review_thread() selects _COMBINED_REVIEW_PROMPT (agent/background_review.py:940-945), whose currently-loaded-skill path still says “PATCH it first” without a fresh skill_view (agent/background_review.py:299-313). That route will still hit the guard.
• The new tests inspect only _SKILL_REVIEW_PROMPT, so they do not protect the combined route.

Suggested changes

• Mirror the handshake and currently-loaded-skill reload instruction into _COMBINED_REVIEW_PROMPT.
• Add coverage for both prompt-selection paths. The existing guard behavior is already exercised in tests/tools/test_skill_manager_tool.py:1289-1350.

The member duplicate note correctly points to #60331, whose diff already covers both prompts. This is an automated hermes-sweeper review.

[teknium1]

Please mirror this contract in _COMBINED_REVIEW_PROMPT too. spawn_background_review_thread() selects that prompt when both memory and skills reviews fire (agent/background_review.py:940-945), and its current preference order still instructs the fork to “PATCH it first” without a fresh skill_view.

[Snowdchike]

Addressed hermes-sweeper review:

1. Shared _READ_BEFORE_WRITE_HANDSHAKE is now injected into both _SKILL_REVIEW_PROMPT and _COMBINED_REVIEW_PROMPT.
2. Currently-loaded skill preference order on both routes now says to re-load via skill_view in this turn before PATCH.
3. Tests cover both prompt-selection paths (skill-only + combined). 15/15 pass.

This is more complete than prompt-only skill path, and keeps the exact guard signal name (_read_before_write_required) for the retry contract.