fix: require skill_view before background skill writes

[elliottway]

PR title

fix: require skill_view before background skill writes

Summary

• enumerate the background review fork's exact allowed tools and explicitly require skill_view before skill_manage writes
• require the full umbrella SKILL.md to be read before creating a new support file
• continue requiring the exact existing support file to be read before overwriting or removing it
• return the exact corrective skill_view(...) call when the read-before-write guard blocks a mutation
• add regression coverage for prompt/tool-contract clarity, patches, support-file creation, and support-file overwrites

Fixes #58475.

This is a current-origin/main rebase of the implementation from #59175. The commit preserves doncazper as the original author.

Verification

• changed focused tests: 57 passed
• broader background-review and skill-management subsystem suite: 134 passed
• independent clean-run review: 57 changed tests passed; 36 broader background-review tests passed
• ruff check on all five changed files: passed
• git show --check: passed
• independent code review: no actionable findings

Scope

Five files only:

• agent/background_review.py
• tools/skill_manager_tool.py
• three focused test files

No gateway lifecycle, provider routing, model selection, or production service code changes.

[teknium1]

Thanks for the focused fix. The premise is verified on current main: tools/skill_manager_tool.py:1305-1314 runs the read guard only when a support-file target already exists, so creation writes a new file without a prior umbrella SKILL.md read. agent/background_review.py:219-238 and :328-336 also direct support-file creation without that prerequisite.

The PR’s guard expansion, prompt changes, and exact tool-contract wording directly address the failure reproduced in #58475. The existing skills toolset already exposes skills_list, skill_view, and skill_manage at toolsets.py:170-173, so enumerating them is accurate. No substantive correctness or design-fit issue was found in the reviewed diff.

Automated hermes-sweeper review.

[GottZ]

This was generated by AI during triage.

Summary

Eight PRs address or reference this issue complex. #59175/#75020 directly fix the #58475 read-before-write failure, while #16026, #23288, #55965, and #64655 cover adjacent skill-quality controls, #23004 fixes transient-failure persistence in #6051, and #24846 concerns memory routing.

Related pull requests

• feat(review): class-first skill review prompt #16026 [merged] partial — (+110/-10) — partial: Adds class-first surveying and asks the reviewer to inspect candidate skills with skill_view, reducing narrow or duplicate skill creation, but it neither guards new support-file writes nor clarifies the runtime tool contract. Although merged, it remains relevant as the prompt-survey foundation for later review-hygiene work.
• fix(review): tell background reviewer not to capture transient env failures as skills #23004 [merged] best fix — (+84/-0) — fixes Skill auto-creation learns from transient failures, causing persistent tool avoidance (learned helplessness) #6051: Prevents both review prompts from persisting transient environment failures and negative tool claims, redirecting setup failures toward recording the fix instead. Although merged via e5af1dd6, it remains the reference implementation for the learned-helplessness cause; sampling and revalidation of existing content remain outside its scope.
• fix: add write-hygiene guardrails to skill review prompts (prevent SKILL.md bloat) #23288 partial — (+326/-30) — partial: Removes completion bias and adds prompt-level write hygiene plus background-only checks for date stamps, duplicate headings, and suspicious growth, but it does not close [Bug]: Background review agent repeatedly gets stuck — prompt ambiguity, vague tool whitelist, and incomprehensible error message #58475's new-support-file guard gap. Consistent with the contributor keep_open review on fix: add write-hygiene guardrails to skill review prompts (prevent SKILL.md bloat) #23288, its separate salvage path is the rebased, autonomous-only hygiene implementation shown in the diff while preserving the existing regression suite.
• fix: route background memory review by destination #24846 related — (+67/-22) — separate concern: Routes stable user facts, deployment facts, reusable procedures, and session-only state to different stores rather than addressing skill support-file writes. Consistent with the contributor keep_open review on fix: route background memory review by destination #24846, salvage requires transplanting the prompt intent to agent/background_review.py and aligning it with tools/memory_tool.py:1076-1086.
• feat: trigger-wording check — detect skills that should have loaded but didn't #55965 best fix — (+87/-0) — partial for Feature: Skill Lifecycle Quality — Better Descriptions, Proactive Improvement Loop, and Writing Principles (inspired by Anthropic skill-creator) #429: Detects descriptions that failed to trigger and proposes broadening them with observed task wording, directly addressing undertriggering but not the [Bug]: Background review agent repeatedly gets stuck — prompt ambiguity, vague tool whitelist, and incomprehensible error message #58475 support-file failure. The contributor keep_open review on feat: trigger-wording check — detect skills that should have loaded but didn't #55965 identifies required salvage work: call skill_view(name) before each patch and surface trigger-fix results through the actual action-summary path.
• [codex] Fix background review skill read-before-write #59175 [closed] duplicate — (+175/-14) — superseded direct fix: Requires the full umbrella SKILL.md read before support-file creation, enumerates the exact review tools, expands the guard, and returns corrective skill_view(...) calls. It remains relevant despite being closed because fix: require skill_view before background skill writes #75020 explicitly carries the same implementation forward on current main with original-author attribution.
• fix(review): make skill curation precision-first #64655 partial — (+110/-77) — partial: Replaces activity bias with a novelty, durability, evidence, reuse, and scope quality gate, addressing low-quality persistence but not [Bug]: Background review agent repeatedly gets stuck — prompt ambiguity, vague tool whitelist, and incomprehensible error message #58475's missing creation guard. Consistent with the contributor keep_open review on fix(review): make skill curation precision-first #64655, salvage requires a fresh skill_view for every existing write target plus prompt-contract coverage.
• fix: require skill_view before background skill writes #75020 best fix — (+195/-14) — direct fix: Reapplies [codex] Fix background review skill read-before-write #59175 on current main, requiring an umbrella read before new support files, naming the exact allowed tools, returning actionable corrective calls, and testing patch, creation, and overwrite paths. The visible MAINTAINER-BOT keep_open verdict is supported by the diff: it closes the reproduced guard and prompt gaps without importing the broader behavior changes from adjacent PRs.

Duplicates

#59175 and #75020 implement materially the same #58475 fix; the explicit duplicate chain is #59175 → #75020, with #59175 already closed as superseded. #23288 and #64655 overlap on write-quality policy but are complementary, not duplicates of that guard fix.

Suggested consolidation

Keep #75020 open with a salvage path: retain its focused current-main continuation of #59175, including the umbrella-read prerequisite, exact tool enumeration, actionable skill_view(...) errors, and regression coverage; this also follows its MAINTAINER-BOT keep_open verdict. Treat the already-closed #59175 as duplicate of #75020, and keep #23288, #24846, #55965, and #64655 separate only along their documented contributor-review salvage paths rather than folding those broader policies into #75020.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    I58475(["issue #58475 (open)"])
    subgraph Dup59175 ["PRs duplicating each other"]
        P59175["PR #59175 (closed)"]
        P75020["PR #75020 (open)"]
    end
    P75020 -->|best fix| I58475
    class I58475 open
    class P59175 closed
    class P75020 open
    class P59175 best
    class P75020 best
    class P75020 target
    click I58475 "https://github.com/NousResearch/hermes-agent/issues/58475"
    click P59175 "https://github.com/NousResearch/hermes-agent/pull/59175"
    click P75020 "https://github.com/NousResearch/hermes-agent/pull/75020"

Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).

Cross-PR triage: Reviewed 8 pull requests and 3 issues in this complex. Each diff was read against this issue; Assessment working set: 100 kB of PR diffs, 37 kB of issue/PR text, 14 kB of discussion (14 comments), 15 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

[teknium1]

Status after this week's read-before-write cluster landed: the _SKILL_REVIEW_PROMPT/_COMBINED_REVIEW_PROMPT read-before-write guidance half of this PR is now redundant — a70d2ff (PR #94530) added it to both prompts, and 335c60e (PR #94511) fixed the underlying mark-loss bug that made the guard unsatisfiable regardless of prompt. Still uncovered from your PR: (a) the support-file fit-check guidance (read full SKILL.md before adding references/, don't add on name+description alone) and (b) enumerating the exact runtime tool whitelist in the deny message instead of 'memory/skill tools'. Both look worth keeping — if you rebase to drop the now-redundant prompt half, we can review the remainder on its own merits. Happy to salvage it ourselves if you prefer, @elliottway.

[elliottway]

Thanks for the update. Please feel free to salvage the two remaining pieces on your side: the support-file fit check and the exact runtime tool whitelist in the deny message. The original implementation came from @doncazper via #59175, so please preserve their authorship when transplanting it. Happy to help with any follow-up if useful.