Skip to content

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

Merged
teknium1 merged 2 commits into
mainfrom
hermes/hermes-64e1073f
Jun 30, 2026
Merged

fix(skills): require background-review forks to read before writing skills (#55647)#55906
teknium1 merged 2 commits into
mainfrom
hermes/hermes-64e1073f

Conversation

@teknium1

Copy link
Copy Markdown
Contributor

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 #55647 where a single review pass shrank an 807-line module to a 186-line stub.

Salvage of #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 #55647.

Infographic

Read-before-write background-review skill guard

@alt-glitch alt-glitch added type/bug Something isn't working comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint tool/skills Skills system (list, view, manage) P1 High — major feature broken, no workaround labels Jun 30, 2026
@teknium1
teknium1 merged commit 86200e7 into main Jun 30, 2026
31 checks passed
@teknium1
teknium1 deleted the hermes/hermes-64e1073f branch June 30, 2026 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P1 High — major feature broken, no workaround tool/skills Skills system (list, view, manage) type/bug Something isn't working

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

3 participants