fix(background-review): gate skills toolset on review_skills flag

[kyssta-exe]

Fixes #64926

Problem

The background review fork unconditionally included the skills toolset in its runtime tool whitelist, even when only a memory review was requested (review_memory=True, review_skills=False). This allowed the memory-only review fork to call skill_manage and auto-patch skill files without user consent.

When a user set skills.creation_nudge_interval: 0 (disabling skill reviews) but kept memory reviews active, the memory review fork still had full access to skill_manage and could independently decide to modify skills.

Fix

In agent/background_review.py:

1. Pass review_memory and review_skills through to _run_review_in_thread
2. Conditionally include the skills toolset only when review_skills=True
3. Conditionally include the memory toolset only when review_memory=True (and memory is enabled)
4. Update the runtime deny message and user prompt to reflect the actually available tools

Testing

All 19 background review tests pass.

[alt-glitch]

This was generated by AI during triage.

Fix for #64926. Complementary to #64963 (which adds a skills.read_only config guard in skill_manager_tool.py covering every write path); this PR instead gates the skills toolset in the background-review fork on review_skills. Related, not duplicate — the two close the gap at different layers. Flagging the cluster for a maintainer to pick.

[tonydwb]

Code Review Summary

Verdict: Approved

Summary

PR #64964 gates the skills toolset on a review_skills flag in background review, preventing skills from being auto-patched when only a memory review was requested.

Assessment

• Correctness: Properly separates toolsets based on what was requested.
• Scope: Small focused change.
• Risk: Low. Additive feature gate.

No concerns

---

Reviewed by Hermes Agent

[teknium1]

Thanks for the focused fix. The premise is confirmed on current main: agent/background_review.py:819-821 always adds skills, while agent/turn_finalizer.py:493-516 independently computes and forwards the two review triggers.

Problems

• tests/run_agent/test_background_review_toolset_restriction.py:117-129 changes the existing case from memory-only to both modes enabled. That validates the allowed combined case, but leaves the reported memory-only isolation guarantee untested.

Suggested changes

• Add a memory-only regression case asserting memory is allowed while skill_manage, skill_view, and skills_list are denied; add the symmetric skill-only/memory-enabled assertion for the new memory gate.

Automated hermes-sweeper review.

[teknium1]

This now tests the combined review path only. Please add a separate review_memory=True, review_skills=False case asserting that skill_manage, skill_view, and skills_list are absent; that is the reported regression.

[GottZ]

This was generated by AI during triage.

Summary

Three open PRs address #64926: #64963 adds a dispatcher-level read-only guard, #64964 isolates background-review toolsets according to the requested review modes, and #64996 implements the broader read-only boundary across the dispatcher, direct helpers, curator operations, and Skills Hub paths.

Related pull requests

• fix(skills): add skills.read_only runtime write guard (#64926) #64963 partial — (+163/-26) — n/a: Adds skills.read_only around mutating skill_manage actions, but its complete diff leaves direct dashboard, learning, curator, and hub mutations outside that boundary, removes the supporting-file read-before-write regression test, and omits the config default and user documentation. Despite the keep_open review and contributor characterization as complementary, fix(skills): add skills.read_only hard deny for runtime skill writes #64996 contains the same config/dispatcher core plus the broader mutation guards, so fix(skills): add skills.read_only runtime write guard (#64926) #64963 is superseded by fix(skills): add skills.read_only hard deny for runtime skill writes #64996.
• fix(background-review): gate skills toolset on review_skills flag #64964 partial — (+29/-12) — n/a: Conditionally exposes the skills and memory toolsets to background review, preventing a memory-only review from receiving skill_manage, but it does not create a global read-only boundary. Keep open with a salvage path, consistent with the contributor keep_open review: add the requested memory-only and symmetric skill-only regression cases, since the current test was changed to exercise both modes together.
• fix(skills): add skills.read_only hard deny for runtime skill writes #64996 best fix — (+289/-2) — n/a: Adds the config default and hard-deny checks across skill_manage, direct mutation helpers, curator archive/restore, and Skills Hub quarantine/install/uninstall; the current diff addresses the contributor review's restore and pre-quarantine-write objections with explicit guards and tests. It remains the recorded complex-level best fix, while the issue discussion identifies the general write_file/patch path into skills directories as a separate bypass not gated by this diff.

Duplicates

#64963 and #64996 substantially duplicate the skills.read_only configuration and skill_manage guard; #64996 supersets that shared core with direct-helper, curator, and Skills Hub coverage. #64964 is complementary rather than a duplicate.

Suggested consolidation

Keep #64996 open with a salvage path: preserve its broader hard-deny implementation and address or explicitly scope the documented general write_file/patch bypass before claiming a complete runtime read-only boundary. Close #64963 as duplicate of #64996 despite its keep_open review, because the complete diffs show #64996 supersets its dispatcher guard and covers the review-documented bypass paths; keep #64964 open separately for background-review isolation, with author action to add the contributor-requested mode-specific regression tests.

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
    I64926(["issue #64926 (open)"])
    P64964["PR #64964 (open)"]
    P64964 -.->|partial| I64926
    class I64926 open
    class P64964 open
    class P64964 target
    click I64926 "https://github.com/NousResearch/hermes-agent/issues/64926"
    click P64964 "https://github.com/NousResearch/hermes-agent/pull/64964"

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 3 pull requests and 1 issue in this complex. Each diff was read against this issue; Assessment working set: 34 kB of PR diffs, 15 kB of issue/PR text, 15 kB of discussion (13 comments), 5 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

[teknium1]

Closing on maintainer ruling: the premise is inverted. Autonomously patching and creating skills is the entire purpose of the background self-improvement review — it's the feature the README leads with, not an unconsented side effect. Cross-availability of skill tools in memory-triggered forks is by design: a combined or memory-triggered review that notices a skill-worthy lesson is supposed to be able to save it. Gating the toolsets per-trigger would starve exactly that.

The adjacent real problems have separate resolutions: the deny-storm mechanics were fixed in #98254 (read tools whitelisted, self-correcting deny message), and users who want to restrict what reviews can touch get explicit config (auxiliary.background_review.extra_tools, and skill pinning / curator ownership guards already protect individual skills). Appreciate the clean implementation and tests — this is a direction call, not a quality one.