fix(skills): require review forks to read before writing skills

[kyssta-exe]

Fixes #55647.\n\nBackground self-improvement review forks now record skill files returned by skill_view and refuse autonomous edit/patch/remove/overwrite operations unless the exact target file was loaded during the current review turn. This prevents review-only skill_manage patches from rewriting or truncating skill content inferred from the transcript rather than current disk state.\n\nTests: scripts/run_tests.sh tests/tools/test_skill_manager_tool.py tests/run_agent/test_background_review.py -q

[rodriguez46p-ui]

Hermes Agent Review — PR #55657

Verdict: Changes requested

I found one blocker in the read-before-write enforcement for background review skill mutations.

Blocking issue

• tools/skill_manager_tool.py (_delete_skill) does not call _background_review_read_before_write_guard() before mutating an existing skill. The new guard is applied to edit, patch, existing supporting-file write_file, and remove_file, but delete still proceeds through the verified-consolidation archive path after only checking absorbed_into exists. As a result, a background review fork can archive/delete an existing skill without first loading that skill's current SKILL.md, which is the behavior this PR is intended to prevent.

Reproduction from a detached PR worktree:

uv run python - <<'PY'
import json, os, tempfile
from pathlib import Path
from unittest.mock import patch
from tools.skill_manager_tool import skill_manage, _create_skill, _reset_background_review_read_marks

def content(name):
    return f"---\nname: {name}\ndescription: A test skill.\n---\n\n# {name}\n\nStep 1: Do the thing.\n"
with tempfile.TemporaryDirectory(prefix='hermes-pr55657-delete-probe-') as td:
    root = Path(td) / '.hermes'
    skills = root / 'skills'
    skills.mkdir(parents=True)
    os.environ['HERMES_HOME'] = str(root)
    _reset_background_review_read_marks()
    with patch('tools.skill_manager_tool.SKILLS_DIR', skills), \
         patch('tools.skills_tool.SKILLS_DIR', skills), \
         patch('agent.skill_utils.get_all_skills_dirs', return_value=[skills]), \
         patch('tools.skill_provenance.is_background_review', return_value=True):
        assert _create_skill('umbrella', content('umbrella'))['success']
        assert _create_skill('narrow', content('narrow'))['success']
        result = json.loads(skill_manage(action='delete', name='narrow', absorbed_into='umbrella'))
        print(json.dumps({
            'delete_without_prior_skill_view_success': result.get('success'),
            'read_before_write_required': result.get('_read_before_write_required'),
            'archived': result.get('_archived'),
        }, indent=2))
PY

Actual output:

{
  "delete_without_prior_skill_view_success": true,
  "read_before_write_required": null,
  "archived": true
}

Expected: the delete/archive should be refused with _read_before_write_required: true until the review fork has loaded skill_view("narrow") in the same review turn.

Suggested fix: in _delete_skill, after the existing background write/consolidation/pinned/target validation and before archive_skill()/shutil.rmtree(), call _background_review_read_before_write_guard(name, skill_dir / "SKILL.md", "delete", "SKILL.md"). Add a regression test mirroring the new patch/write-file tests: verified consolidation is blocked before skill_view("narrow") and succeeds after that read.

Checks run

• uv run python -m pytest tests/tools/test_skill_manager_tool.py::TestCuratorConsolidationDeleteGuard::test_background_review_patch_requires_skill_view_first tests/tools/test_skill_manager_tool.py::TestCuratorConsolidationDeleteGuard::test_background_review_support_file_overwrite_requires_that_file_read -q → 2 passed in 1.02s
• Ad-hoc delete probe above → demonstrated the missing read-before-delete guard

---

Reviewed by Hermes Agent during the hourly commander run.

[tonydwb]

Code Review Summary

Verdict: LGTM

Security fix adding a read-before-write guard for background review forks. Prevents the autonomous review fork from patching or rewriting skill content it has only inferred from the transcript.

✅ Looks Good

• Clean implementation using contextvars for thread-safe read tracking
• Well-scoped: only affects background_review origin
• Good test coverage with 3 dedicated tests (patch requires view, file overwrite requires file read, reset helper)
• Proper fallback: non-background-review origins bypass the guard entirely
• The assert additions after _resolve_skill_target are safe (the function returns (path, None) on success)

---

Reviewed by Hermes Agent

[alt-glitch]

This was generated by AI during triage.

Competing with #55665 for the same bug (#55647). This PR enforces a read-before-write invariant (refuse autonomous edit/patch unless the file was loaded via skill_view this turn) — the root-cause fix; #55665 adds a defensive >50%-shrink guard in _patch_skill. Both open against the same issue — flagging for a maintainer to pick the canonical fix.

[teknium1]

Merged via #55906 — your commit was cherry-picked onto current main with your authorship preserved in git log (20871c1d9).

The read-before-write invariant landed exactly as you designed it: the background-review fork now must skill_view a skill (or support file) before skill_manage can patch/edit/write_file/remove_file it, gated on is_background_review() so normal sessions are untouched. 107 targeted tests + an E2E run (real imports, temp HERMES_HOME) confirm unread patches are refused and post-skill_view patches still land. Thanks for the clean fix.

#55906