[Bug]: Background review agent and curator can overwrite bundled/hub skills via skill_manage

[DanielMaly]

Describe the bug

skill_manage has no code-level write guard for bundled or hub-installed skills. The only protections are _pinned_guard() (blocks pinned skills) and _security_scan_skill() (blocks dangerous content, off by default). Any agent session with access to skill_manage can freely edit, patch, or delete bundled skills.

This affects two autonomous subsystems that run without user supervision:

1. Background review agent (most urgent)

_spawn_background_review() (run_agent.py:3465) runs after every conversation turn with ≥10 tool iterations. It forks an AIAgent with enabled_toolsets=["memory", "skills"] and a prompt (_SKILL_REVIEW_PROMPT at run_agent.py:3270) that says:

"Be ACTIVE — most sessions produce at least one skill update, even if small. A pass that does nothing is a missed learning opportunity."

This prompt has no instruction to avoid bundled or hub-installed skills. Combined with the lack of a code guard, the background review agent can and does patch bundled skills during normal conversations. The main agent is unaware — the review runs in a background thread after the response is delivered.

2. Curator

The curator (agent/curator.py) runs on a 7-day schedule. Its prompt (CURATOR_REVIEW_PROMPT at line 261) does include the instruction "DO NOT touch bundled or hub-installed skills", but this is a prompt-level instruction, not an enforced boundary. A poisoned community skill could inject instructions that override it (prompt injection → persistent code modification via skill_manage).

To reproduce

1. Have bundled skills installed in ~/.hermes/skills/
2. Have a conversation that triggers the background review (≥10 tool iterations, configurable via skills.creation_nudge_interval)
3. The background review agent can call skill_manage(action='patch', name='<bundled-skill>', ...) — no error is returned
4. The bundled skill is now modified in ~/.hermes/skills/
5. On next hermes update, the skills sync detects the hash divergence and prints ~ N user-modified (kept) — the agent's modification is silently preserved, not overwritten. There is no path back to the upstream version short of manually deleting the skill directory.

Why this is a problem

The issue is not that edits get overwritten — they don't. The skills sync (tools/skills_sync.py) uses hash-based detection: if the user copy differs from the origin hash, it's treated as "user-modified" and skipped. This means:

1. Silent corruption — The agent patches a bundled skill, the sync preserves the patch, and neither the user nor the sync reports anything wrong. The ~ N user-modified (kept) message sounds benign, like intentional customization.
2. No path back to upstream — Once the hash diverges, hermes update will never restore the original bundled content. The user's copy is permanently forked with whatever the agent wrote.
3. The sync can't distinguish agent accidents from user intent — Both look like hash divergence. The agent's unintended edits are indistinguishable from deliberate user customization.

Expected behavior

skill_manage should refuse write operations (edit, patch, delete, write_file, remove_file) on bundled and hub-installed skills at the tool level, returning a clear error. The background review prompt should also include an explicit "DO NOT touch bundled or hub-installed skills" instruction as defense-in-depth.

Related issues and PRs

• fix(security): add code-level guard against modifying bundled/hub skills via skill_manage #19379 — PR that adds _bundled_hub_guard() mirroring _pinned_guard(), which would fix both paths. Currently has failing tests and merge conflicts.
• [Feature]: Preserve agent-adapted skills during external provider updates via diff/merge reconciliation #1780 — Proposes a diff/merge layer to preserve agent edits to bundled skills across updates. This is related but takes the opposite view: it assumes the agent editing bundled skills is intentional and worth preserving. If this bug is fixed (writes blocked), [Feature]: Preserve agent-adapted skills during external provider updates via diff/merge reconciliation #1780 becomes moot — there would be no agent edits to preserve. If a future "fork and customize" workflow is desired, it should be explicit opt-in, not an accidental side-effect of an unguarded tool.

Additional context

• SKILLS_GUIDANCE in agent/prompt_builder.py:176 (injected into every session where skill_manage is available) actively encourages the behavior: "When using a skill and finding it outdated, incomplete, or wrong, patch it immediately — don't wait to be asked." No caveat about bundled vs. agent-created skills.
• The curator.auxiliary.model / curator.auxiliary.provider config options defined in hermes_cli/config.py:952 are not wired up — the curator always uses the main model config regardless of these settings.

[alt-glitch]

Related: PR #19379 (open) adds the code-level _bundled_hub_guard() that would fix this. This issue provides additional context about the background review agent and curator attack surfaces beyond what #19379 covers.

Also related: #19621 (merged) tightened provenance marking for background review but didn't add the write guard.

[steezkelly]

I scoped this against a local checkout after hitting a curator-adjacent CLI bug and can confirm the boundary issue exists in the current local code path: skill_manage checked pinned status for delete, but did not enforce bundled/hub provenance before edit, patch, delete, write_file, or remove_file.

I also tested a local hardening patch. Slight implementation note from that validation: instead of checking only ~/.hermes/skills/<name>/.hub-source, reusing the existing tools.skill_usage.is_agent_created(name) provenance helper covers both:

• bundled skills via ~/.hermes/skills/.bundled_manifest
• hub-installed skills via ~/.hermes/skills/.hub/lock.json, including nested/category install_path entries

Local validated behavior:

• manifest-marked bundled skill: all 5 mutation actions are refused (edit, patch, delete, write_file, remove_file)
• hub lock entry with nested install path (community/hub-skill): mutation is refused
• user/agent-created skills remain writable

Local verification:

python -m pytest tests/tools/test_skill_manager_tool.py -q
88 passed

python -m pytest tests/tools/test_skill_manager_tool.py tests/tools/test_skill_usage.py tests/agent/test_curator.py tests/agent/test_curator_activity.py -q
177 passed

Local commit for reference:

eb715dd64 fix(security): guard bundled skills from skill_manage writes

I deliberately kept this as a tool-boundary guard; prompt-level instructions for the background review/curator are useful defense-in-depth, but the reliable fix is refusing the write in skill_manage before any filesystem mutation happens.

[steezkelly]

Opened a tested PR for this using the existing provenance machinery:

#20560

Summary:

• Adds a tool-level guard in skill_manage for bundled/hub-installed skills.
• Applies the guard to edit, patch, delete, write_file, and remove_file.
• Uses tools.skill_usage.is_agent_created() so .bundled_manifest and .hub/lock.json remain the source of truth.
• Adds regression tests for bundled manifest protection and nested hub lock install paths.

Verification:

scripts/run_tests.sh tests/tools/test_skill_manager_tool.py
88 passed

python -m py_compile tools/skill_manager_tool.py tests/tools/test_skill_manager_tool.py
passed

git diff --check origin/main...HEAD
clean

I also left a note on #19379 explaining why #20560 uses the existing provenance path rather than a .hub-source marker.

[teknium1]

Thanks for the thorough writeup. After thinking through this, we're going to keep the current behavior — agents patching bundled skills is a core design choice, not an oversight. The self-improvement loop the README leads with explicitly runs the background review agent with skill_manage against the full skill library, including bundled skills like hermes-agent-dev which we patch regularly from inside the agent itself. Blocking that at the tool level would remove the primary path by which our bundled skills improve.

The two concrete pain points you raised are still real, but they want different fixes:

1. Silent fork / no path back to upstream on hermes update. This is the load-bearing issue — the user can't see what diverged or restore the original. That's a hermes update UX gap, not a skill_manage guard gap. Tracked under [Feature]: hermes update should show what changed (skill names, config migration details, offer to reset user-modified skills) #18402 (show skill-name changes on update + offer to reset user-modified skills). Handling merge/keep/reset there covers both agent-introduced divergence and deliberate user customization with the same workflow.

2. Prompt injection from a poisoned hub skill. Bounded threat: if a user installs a rogue community skill, their library is already compromised — blocking writes doesn't un-compromise it, and the same poisoned skill can still steer the agent via terminal, web, or any other tool in the session. Hub installs should be curated/audited; a write-side block is the wrong layer for the problem.

Side-finding worth spinning out separately: the dead curator.auxiliary.model / curator.auxiliary.provider config wiring you noted in the last bullet. That's a legit bug, independent of this one — happy to take a separate issue for it if you want to file one.

Closing this one. Really appreciate the depth of the report — the analysis of the .bundled_manifest / .hub/lock.json provenance paths and how the sync treats hash divergence was genuinely useful and will inform the #18402 fix.

Closing via #21178's rationale. Credit also to @steezkelly for #20560's clean implementation of _bundled_hub_guard.