Skip to content

fix(skills): require background-review forks to read before writing skills (#55647) - #22

Merged
hashbender merged 1 commit into
mainfrom
mirror/pr-55906
Jun 30, 2026
Merged

fix(skills): require background-review forks to read before writing skills (#55647)#22
hashbender merged 1 commit into
mainfrom
mirror/pr-55906

Conversation

@hashbender

Copy link
Copy Markdown
Owner

Summary

The per-session background-review fork can no longer patch a skill it never read. skill_manage(patch/edit/write_file/remove_file) now refuses to mutate a skill during a background-review turn unless that exact file was loaded via skill_view first — closing the silent skill-corruption path in NousResearch#55647 where a single review pass shrank an 807-line module to a 186-line stub.

Salvage of NousResearch#55657 by @kyssta-exe, cherry-picked onto current main with authorship preserved.

Changes

  • tools/skill_manager_tool.py: per-fork ContextVar records which skill files the review fork has read; _background_review_read_before_write_guard() refuses patch/edit/write_file/remove_file on a skill (or support file) not loaded this turn. Gated on is_background_review() — zero effect on normal sessions.
  • tools/skills_tool.py: skill_view marks the loaded file as read so a subsequent targeted patch is allowed.
  • agent/background_review.py: reset the read-marks when the review fork tears down.
  • tests/tools/test_skill_manager_tool.py: read-before-write blocked, then allowed after skill_view; support-file overwrite requires reading that specific file.
  • scripts/release.py: AUTHOR_MAP entry for the salvaged commit's noreply email.

Note on the issue's premise

Root cause #1 (no read-before-write invariant) is confirmed and is what this PR fixes. Root cause #2 (the fork inherits the full parent toolset) does not hold on current mainagent/background_review.py already builds a runtime tool whitelist from ["memory", "skills"] via set_thread_tool_whitelist(...), so the fork cannot call terminal/patch/execute_code. The bug survived that whitelist only because skill_view is whitelisted yet was never required before skill_manage. This PR adds that requirement.

Validation

Before After
Bg-review patch on unread skill lands silently, can delete most of the file refused with _read_before_write_required
Bg-review patch after skill_view lands lands (unchanged)
Normal (non-review) session unaffected unaffected
  • tests/tools/test_skill_manager_tool.py: 107 passed.
  • E2E (real imports, temp HERMES_HOME, 2912-char SKILL.md): unread patch blocked with the flag → skill_view → patch allowed. PASS.

Closes NousResearch#55647.

Infographic

Read-before-write background-review skill guard


Mirror-of: NousResearch#55906
NousResearch#55906

@hashbender
hashbender merged commit df3c2d9 into main Jun 30, 2026
30 of 31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

background-review: skill_manage patches hallucinate existing skill content; review fork writes without read-before-write invariant

1 participant