Skip to content

feat(skill_manager): honor metadata.hermes.locked frontmatter flag - #23715

Open
unimatrix27 wants to merge 1 commit into
NousResearch:mainfrom
unimatrix27:feat/skill-locked-flag
Open

unimatrix27 wants to merge 1 commit into
NousResearch:mainfrom
unimatrix27:feat/skill-locked-flag

Conversation

@unimatrix27

Copy link
Copy Markdown

Summary

Adds _locked_guard() as the edit-side counterpart to _pinned_guard(). A skill opts in by setting metadata.hermes.locked: true in its SKILL.md frontmatter; subsequent skill_manage write actions (edit, patch, delete, write_file, remove_file) are refused with a message instructing the user — not the agent — to lift the lock by hand. Read actions are unaffected.

The default behaviour (no flag, or locked: false) is preserved exactly.

_pinned_guard  blocks: delete
               opt-in: `hermes curator pin <name>`
_locked_guard  blocks: edit, patch, delete, write_file, remove_file
               opt-in: `metadata.hermes.locked: true` in SKILL.md

Motivation

The background self-improvement loop (_spawn_background_review) plus the in-conversation skill_manage schema (which actively encourages patching during the run) can silently rewrite any user skill. For skills that gate real-world side effects — financial transfers, device control, message dispatch — silent drift is unsafe. This was filed previously as #20273 (background-review + curator overwriting bundled/hub skills) and the user-facing concern is the same as in #17583 (no distinction between "user authored, do not touch" and "agent generated, fair game").

A concrete failure mode I hit while building a Mode-B style stub skill: between two runs, the SKILL.md was rewritten by the agent to match the latest cron prompt's overrides — the city changed, the data source changed, the deterministic side-effect script was deleted from the workflow, and a new hard rule "no device control, ever, in this skill version" appeared. Nothing was logged beyond a single 💾 Skill patched line. The next run executed the rewritten version as if it were the user's design.

That is exactly the drift pattern this flag prevents.

Why this PR is compatible with the wontfix reasoning on #21315

#21315 proposed skills.evolution_mode = auto|confirm|readonly. It was closed (NOT_PLANNED) on three grounds:

  1. No new UX surface needed — every mutation already goes through skills_guard and the curator takes snapshots that can be rolled back from.
  2. readonly disables a headline feature — making self-improvement gateable globally is "fighting the product, not extending it."
  3. Precision over gating — bad patches should be fixed by improving the reviewer's heuristics, not by asking the user to triage every patch.

This PR is deliberately a much smaller change shaped to fit those constraints:

  • No new UX surface. No pending queue, no /skills review slash command, no diff prompts, no platform confirm/reject flow. The lock is a single frontmatter boolean checked at the existing tool boundary.
  • Does not disable the self-improvement loop. The default behaviour is unchanged; 99% of skills remain mutable. Lock is opt-in per skill, intended for the rare ones that gate side effects the user cannot afford to have rewritten.
  • Does not ask the user to triage every patch. The user authors the lock once when they create the sensitive skill. There is no per-run interaction, no notification queue, no review backlog.
  • Symmetric with an existing mechanism. _pinned_guard already guards deletes via the same shape (advisory check, opt-in per skill, fails open on broken metadata). This adds the missing edit-side counterpart that Pin should protect against deletion, not prevent edits #18354 essentially argued for ("two different concerns that should not be conflated"). Where Pin should protect against deletion, not prevent edits #18354 wanted pin split into delete-protection + edit-protection, this PR keeps pin as-is and introduces lock as the separate edit-protection axis.

It also addresses #20273's narrower bug surface for user-authored skills: a locked skill cannot be silently rewritten by the background-review agent even when it passes the security guard. (Bundled-skill protection per PR #19379 is orthogonal; this PR does not touch that path.)

What this PR deliberately does NOT do

Implementation

  1. New _locked_guard(name) in tools/skill_manager_tool.py, placed next to _pinned_guard for visible symmetry. Reads the target skill's SKILL.md frontmatter via the existing agent.skill_utils.parse_frontmatter helper; checks metadata.hermes.locked is True.
  2. One dispatch hook in skill_manage() before the action handlers run, gating the five write actions.
  3. Schema docstring updated alongside the existing "Pinned skills are protected from deletion" paragraph so the agent knows what to expect when a write is refused.
  4. Fails open on missing or unparseable frontmatter — mirrors _pinned_guard's broken-sidecar handling. A corrupted file should never trap the agent.

Net diff: +229 / -1 across two files. The non-test portion is +72 / -1 in skill_manager_tool.py: the function (~40 lines), the dispatch hook (5 lines), the schema-doc update (8 lines), plus blank lines.

Tests

tests/tools/test_skill_manager_tool.py::TestLockedGuard — 9 cases mirroring TestPinnedGuard's structure:

  • One refusal test per blocked action (edit, patch, delete, write_file, remove_file).
  • test_unlocked_skills_still_editable — sanity check that only the explicitly-locked skill is refused; siblings work.
  • test_locked_false_does_not_block — explicit locked: false behaves identically to omitting the flag.
  • test_create_is_exempt_but_subsequent_edits_blocked — create-time bypass + post-create lock enforcement.
  • test_broken_frontmatter_fails_open — unparseable YAML does not falsely trigger the lock.

All 95 tests in test_skill_manager_tool.py pass locally (8 existing pin tests + 9 new lock tests + 78 unrelated). No existing test was modified.

Related

Test plan

  • pytest tests/tools/test_skill_manager_tool.py::TestLockedGuard passes (9/9).
  • pytest tests/tools/test_skill_manager_tool.py::TestPinnedGuard still passes (8/8) — no regression in the existing guard.
  • pytest tests/tools/test_skill_manager_tool.py full file passes (95/95).
  • Manual smoke test: add metadata.hermes.locked: true to a real skill and attempt patch from a chat session — expect refusal.

Add `_locked_guard()` as the edit-side counterpart to `_pinned_guard()`.
A skill opts in by setting `metadata.hermes.locked: true` in its SKILL.md
frontmatter; subsequent skill_manage write actions (edit, patch, delete,
write_file, remove_file) are refused with a message instructing the user
(not the agent) to lift the lock by hand.

Read actions are unaffected — locked skills remain discoverable and
loadable, just not writable by tools. The default behaviour (no flag,
or `locked: false`) is preserved.

Use case: skills that gate real-world side effects (financial transfers,
device control, message dispatch) where silent drift via the background
self-improvement loop or in-conversation patches is unsafe. The flag
makes the skill the source of truth for what it does, not the agent's
recent context.

Pin / lock symmetry:

    _pinned_guard  blocks: delete
                   opt-in: `hermes curator pin <name>`
    _locked_guard  blocks: edit, patch, delete, write_file, remove_file
                   opt-in: `metadata.hermes.locked: true` in SKILL.md

The guard fails open on missing or unparseable frontmatter (mirroring
the pin guard's broken-sidecar behaviour) so a corrupted file can never
trap the agent.

Tests: 9 new cases mirroring TestPinnedGuard's structure — one per
blocked action, plus sanity tests for unlocked siblings, explicit
locked: false, the create exemption, and the broken-frontmatter
fallback. All 95 tests in test_skill_manager_tool.py pass.
@alt-glitch alt-glitch added type/feature New feature or request P3 Low — cosmetic, nice to have tool/skills Skills system (list, view, manage) labels May 11, 2026
fabiosiqueira added a commit to fabiosiqueira/hermes-engine that referenced this pull request Jul 7, 2026
…4 carries

Conflitos (7 arquivos) resolvidos integrando cada carry sobre o control-flow
novo do upstream (superset limpo, sem codigo orfao):

- memory-bridge (6457f8f): CONVERGIDO. Upstream absorveu a logica em
  MemoryManager.notify_memory_tool_write (superset: gate em sucesso,
  add/replace/remove, forward old_text, + expande operations batched). Tomada a
  versao upstream em agent_runtime_helpers.py, tool_executor.py e
  test_memory_provider.py. Nosso PR NousResearch#39508 ficou redundante (candidato a fechar
  como superseded).
- skill-locked (d840a28): MANTIDO (rota NousResearch#23715 ainda OPEN). Uniao com os
  guards novos do upstream (_background_review_*, _curator_consolidation_delete)
  e com TestDeleteSkillRmtreeGuard; lock-guard roda antes do approval-gate.
- send_message action=edit (7f9cc10): MANTIDO (rota NousResearch#25919 ainda OPEN). Uniao
  com react/unreact do upstream; honra remocao de _send_slack (migrado p/ plugin
  slack, NousResearch#41112).
- cron-memory skip_memory_provider (b1e8eb1): MANTIDO (rota NousResearch#18565 ainda OPEN).
  Uniao com os testes novos de memory-routing/callback do upstream.
- oneshot stderr (612b515<->b02e29bd8): par add+revert neutralizado; toma a
  versao upstream (NousResearch#30623).

Verificado: py_compile dos 7 arquivos + 387 testes-alvo passam
(send_message, skill_manager, memory_provider, memory_provider_init).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jul 13, 2026
fabiosiqueira added a commit to fabiosiqueira/hermes-engine that referenced this pull request Aug 2, 2026
Carries re-validated against production — all three live routes still OPEN
upstream, so nothing converged this round:

  NousResearch#74875 streaming session tag  → ours, rebased (see below)
  NousResearch#23715 skill_manager locked   → OPEN, not in production
  NousResearch#25919 send_message edit      → OPEN, not in production
  NousResearch#18565 cron memory provider   → OPEN, not in production

Conflict resolutions:

- agent/chat_completion_helpers.py: took the rebased NousResearch#74875 branch wholesale
  (upstream/main + carry only). The old carry's `exc_info=True` half is gone —
  upstream's 8e191af converted that call to logger.exception, a superset.
- tests/run_agent/test_memory_provider_init.py: kept only the carry's real
  additions (_build_agent + the three skip_memory_provider cases). The
  neighbouring test_aiagent_forwards_warning_callback_to_cli_memory_provider
  came through the 3-way as context but upstream deleted it deliberately in
  6b81590 (test-prune wave 1); resurrecting it would undo that.
- tests/tools/test_send_message_tool.py: additive import conflict, resolved by
  union.

Also repaired in the carry: agent_init now reads config via
load_config_readonly, so the carry's tests patching only load_config were
silently a no-op and the provider never loaded. Patched both, matching the
idiom the surrounding upstream tests already use.

Verified: test_memory_provider_init, test_send_message_tool,
test_skill_manager_tool, test_stream_worker_session_context — 65 passed,
1 skipped. skip_memory_provider coverage re-checked under mutation (knob
ignored → 2 failures).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low — cosmetic, nice to have sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades tool/skills Skills system (list, view, manage) type/feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants