Skip to content

Harden the skill self-improvement pipeline: approval gating + coding-lesson awareness - #63019

Open
Yao-Teng wants to merge 2 commits into
NousResearch:mainfrom
Yao-Teng:claude/system-issue-review-0bb370
Open

Harden the skill self-improvement pipeline: approval gating + coding-lesson awareness#63019
Yao-Teng wants to merge 2 commits into
NousResearch:mainfrom
Yao-Teng:claude/system-issue-review-0bb370

Conversation

@Yao-Teng

Copy link
Copy Markdown

Summary

Two related hardening changes to the agent's self-improvement (skill-learning) pipeline, prompted by a review of how agent/background_review.py autonomously creates/edits skills under ~/.hermes/skills/.

  • Approval-gate background-review skill writes by default (tools/write_approval.py, hermes_cli/config.py): the background-review fork judges its own lesson and persists it with no independent verification — the actual mechanism by which bad/stale skills silently enter the library. A new skills.write_approval_background_review flag (default true) stages those writes for /skills pending review instead of committing them immediately. The general skills.write_approval flag (default off) is untouched, so foreground, user-directed skill writes still flow freely — only the autonomous path gets the extra check. Users can opt back into the old behavior with skills.write_approval_background_review: false.

  • Teach the review pass to recognize coding sessions (agent/background_review.py): the review prompt previously treated every session identically, with no awareness that bundled skills like systematic-debugging and test-driven-development already exist as the natural home for coding lessons. Added a cheap _detect_coding_signal() that scans the conversation snapshot for Edit/Write/NotebookEdit calls and test-runner Bash commands (pytest, npm test, go test, etc.), pulling a few concrete (command, outcome) pairs, and appends it to the skill-review prompt when present. Also updated both review prompts to point coding lessons at the existing debugging/TDD umbrella skills first, and carved out an exception in the protected-skills rule: agent-created references/*.md files can now be added under bundled/hub skills (their SKILL.md itself stays protected) so technique detail accumulates in one authoritative place instead of spawning narrow one-off skills.

Why

Investigated the skill self-improvement pipeline end-to-end (background review fork → optional curator consolidation) and found no correctness/quality gate before a skill gets written — the two gates that do exist (security scanner, write-approval) both default off, and there's no signal distinguishing coding lessons from everything else despite dedicated coding skills already existing.

Test plan

  • uv run pytest tests/tools/test_write_approval.py — 27/27 passed (2 new tests added)
  • uv run pytest tests/tools/test_skill_manager_tool.py — passes (one test updated to account for the new default gate; one pre-existing Windows-only symlink-permission failure unrelated to this change)
  • uv run pytest tests/run_agent/ -k background_review — 33/33 passed (6 new tests added in tests/run_agent/test_background_review_coding_signal.py)
  • uvx ruff check on all changed files — clean

Yao-Teng added 2 commits July 12, 2026 14:26
The self-improvement review fork autonomously judges its own lesson and
persists it to ~/.hermes/skills with no independent verification, which
is the actual source of bad/stale skills silently entering the library.
Add a new skills.write_approval_background_review flag (default true)
that stages those writes for /skills pending review, independent of the
general write_approval flag which still defaults off for foreground
(user-directed) skill writes.
…essions

The background-review fork treats every session identically, with no
special handling for coding lessons even though bundled skills like
systematic-debugging and test-driven-development already exist as the
natural home for them. Add a cheap _detect_coding_signal() that scans
the conversation snapshot for Edit/Write/NotebookEdit calls and
test-runner Bash commands (pytest, npm test, go test, etc.), extracting
a few concrete (command, outcome) pairs, and append it to the skill
review prompt when present.

Also update both review prompts to point coding lessons at the existing
debugging/TDD umbrella skills first, and carve out an exception in the
protected-skills rule: agent-created references/*.md files can now be
added under bundled/hub skills (SKILL.md itself stays protected) so
technique detail accumulates in one authoritative place instead of
spawning narrow one-off skills.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing autonomous skill-write safety; current main does allow background-review skill writes when skills.write_approval is unset (hermes_cli/config.py:2355, tools/skill_manager_tool.py:1274-1295).

Problems

  • agent/background_review.py:38,73 recognizes Edit/Write and Bash, but Hermes currently emits patch, write_file, and terminal (tools/file_tools.py:2171-2172, tools/terminal_tool.py:3021-3024). The coding signal therefore misses normal Hermes coding sessions; its tests use those non-schema names too.
  • agent/background_review.py:204-211 says the background fork may add references to protected skills, but _background_review_write_guard() rejects every autonomous action for protected, hub-installed, and bundled skills before _write_file() (tools/skill_manager_tool.py:351-376,1156-1161). That exception cannot execute.
  • The new default-on config setting is absent from the existing write-approval documentation (website/docs/user-guide/configuration.md:595-604).

Suggested changes

  • Detect current schema tool names and add realistic transcript fixtures.
  • Either remove the protected-skill exception or implement an explicitly scoped, tested guard exception after resolving that policy choice.
  • Add an integration test for default background-review staging through skill_manage, and document the new setting.

Automated hermes-sweeper review.

# reach into the separate trajectory-capture pipeline (agent/trajectory.py),
# which serializes to a different (ShareGPT/RL) shape for a different
# consumer (fine-tuning data export) and isn't available mid-turn anyway.
_CODE_EDIT_TOOLS = {"Edit", "Write", "NotebookEdit", "MultiEdit"}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are not Hermes file-tool names: current schemas register patch and write_file (tools/file_tools.py:2171-2172), while command execution is terminal (tools/terminal_tool.py:3021-3024). As a result, ordinary Hermes edits never set saw_code_edit; please detect the emitted schema names and update the fixtures accordingly.

"Protected skills (DO NOT edit or replace SKILL.md itself):\n"
" • Bundled skills (shipped with Hermes, e.g. 'hermes-agent').\n"
" • Hub-installed skills (installed via 'hermes skills install').\n"
"Exception: you MAY add a `references/<topic>.md` file under a "

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This prompt exception is currently unreachable. The background-review preflight calls _background_review_write_guard, which rejects every action—including write_file—for protected built-in, hub-installed, and bundled skills (tools/skill_manager_tool.py:351-376,1156-1161). Please either remove this instruction or implement and test an explicitly scoped guard exception.

@alt-glitch alt-glitch added type/feature New feature or request comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint tool/skills Skills system (list, view, manage) P3 Low — cosmetic, nice to have labels Jul 12, 2026
@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 12, 2026
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 P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform 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