From 0295f19a24af7c65c6133a342a38a79f8d599ac1 Mon Sep 17 00:00:00 2001 From: user <202721646+Yao-Teng@users.noreply.github.com> Date: Sun, 12 Jul 2026 14:26:22 +0800 Subject: [PATCH 1/2] fix(skills): gate background-review skill writes for approval by default 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. --- hermes_cli/config.py | 10 +++++++ tests/tools/test_skill_manager_tool.py | 12 +++++++- tests/tools/test_write_approval.py | 38 ++++++++++++++++++++++++++ tools/write_approval.py | 30 ++++++++++++++++++++ 4 files changed, 89 insertions(+), 1 deletion(-) diff --git a/hermes_cli/config.py b/hermes_cli/config.py index 4e2e67869020..ff5a7eda1ea7 100644 --- a/hermes_cli/config.py +++ b/hermes_cli/config.py @@ -1982,6 +1982,16 @@ def _ensure_hermes_home_managed(home: Path): # never crammed into a chat bubble), apply with # /skills approve or drop with /skills reject . "write_approval": False, + # Separate, narrower approval gate that ONLY covers skill writes made + # by the background self-improvement review fork (not foreground + # writes -- those are covered by write_approval above). That fork + # autonomously judges its own lesson and persists it with no + # independent verification, which is the actual source of bad/stale + # skills silently entering ~/.hermes/skills. On by default: those + # writes stage for review (/skills pending) rather than committing + # immediately. Set to false to let the background fork write skills + # freely, matching the pre-existing (write_approval-only) behaviour. + "write_approval_background_review": True, }, # Curator — background skill maintenance. diff --git a/tests/tools/test_skill_manager_tool.py b/tests/tools/test_skill_manager_tool.py index 245ca9347576..cbef5ba32d62 100644 --- a/tests/tools/test_skill_manager_tool.py +++ b/tests/tools/test_skill_manager_tool.py @@ -564,7 +564,17 @@ def test_full_create_via_dispatcher(self, tmp_path): assert rec.get("created_by") in {None, "", False} def test_create_from_background_review_marks_agent_created(self, tmp_path): - """Background-review fork creates ARE marked as agent-created.""" + """Background-review fork creates ARE marked as agent-created. + + Disables the background-review write-approval gate (on by default) + so this exercises the actual create + marking path rather than + staging — that gate is covered separately in test_write_approval.py. + """ + import hermes_cli.config as cfg + c = cfg.load_config() + c.setdefault("skills", {})["write_approval_background_review"] = False + cfg.save_config(c) + from tools.skill_provenance import set_current_write_origin, BACKGROUND_REVIEW token = set_current_write_origin(BACKGROUND_REVIEW) try: diff --git a/tests/tools/test_write_approval.py b/tests/tools/test_write_approval.py index fbfa804fbb9b..9e2e4b8105b8 100644 --- a/tests/tools/test_write_approval.py +++ b/tests/tools/test_write_approval.py @@ -48,6 +48,44 @@ def test_invalid_subsystem_is_off(hermes_home): assert wa.write_approval_enabled("bogus") is False +def test_skill_gate_defaults_on_for_background_review(hermes_home): + """Unset config: background-review skill writes default to gated (staged + for approval), since that fork persists its own self-assessed lessons + with no independent check. Foreground writes are unaffected.""" + from tools import write_approval as wa + from tools import skill_provenance as sp + + assert wa.write_approval_enabled("skills") is False # foreground default unchanged + + token = sp.set_current_write_origin(sp.BACKGROUND_REVIEW) + try: + assert wa.write_approval_enabled("skills") is True + assert wa.write_approval_enabled("memory") is False # only skills, not memory + finally: + sp.reset_current_write_origin(token) + + assert wa.write_approval_enabled("skills") is False # restored after reset + + +def test_skill_gate_explicit_off_wins_even_in_background(hermes_home): + """A user who explicitly sets skills.write_approval_background_review: + false opts back into the old always-on-unset behaviour for the + background-review fork too.""" + import hermes_cli.config as cfg + from tools import write_approval as wa + from tools import skill_provenance as sp + + c = cfg.load_config() + c.setdefault("skills", {})["write_approval_background_review"] = False + cfg.save_config(c) + + token = sp.set_current_write_origin(sp.BACKGROUND_REVIEW) + try: + assert wa.write_approval_enabled("skills") is False + finally: + sp.reset_current_write_origin(token) + + def test_normalize_enabled_coerces_values(): from tools import write_approval as wa # Real bools pass through. diff --git a/tools/write_approval.py b/tools/write_approval.py index b017299d806b..d115813a5ce8 100644 --- a/tools/write_approval.py +++ b/tools/write_approval.py @@ -77,9 +77,23 @@ def write_approval_enabled(subsystem: str) -> bool: Reads ``.write_approval`` from config.yaml. Defaults to ``False`` (gate off — writes flow freely) for any unset / invalid value so existing installs keep their current behaviour until the user opts in. + + For ``skills`` writes specifically, the gate is ALSO on whenever the + write originates from the background-review fork and + ``skills.write_approval_background_review`` is true (default ``True``, + independent of the general ``write_approval`` flag above). That fork + autonomously decides what "worked" and persists it with no independent + verification — the exact path that produced the "wrong assumptions" + users complained about (see module docstring). A user who explicitly + asks the agent to save a skill in a foreground turn is present and + endorsing the write, so the general flag (default off) still governs + that path. Set ``skills.write_approval_background_review: false`` to + let the background fork write skills freely again. """ if subsystem not in _SUBSYSTEMS: return False + if subsystem == SKILLS and is_background() and _background_review_gate_enabled(): + return True try: from hermes_cli.config import load_config, cfg_get cfg = load_config() @@ -89,6 +103,22 @@ def write_approval_enabled(subsystem: str) -> bool: return _normalize_enabled(raw) +def _background_review_gate_enabled() -> bool: + """Read ``skills.write_approval_background_review`` from config.yaml. + + Defaults to ``True``: unlike the general per-subsystem gate, background- + review skill writes are approval-gated out of the box (see + ``write_approval_enabled`` docstring for why). + """ + try: + from hermes_cli.config import load_config, cfg_get + cfg = load_config() + raw = cfg_get(cfg, SKILLS, "write_approval_background_review", default=True) + except Exception: + return True + return _normalize_enabled(raw) + + def _normalize_enabled(value: Any) -> bool: """Coerce a config value to a bool. Default (unknown) is False (gate off). From 07e99a0ce59648e97603970ee30cf8b21c20a9b2 Mon Sep 17 00:00:00 2001 From: user <202721646+Yao-Teng@users.noreply.github.com> Date: Sun, 12 Jul 2026 14:26:23 +0800 Subject: [PATCH 2/2] feat(skills): teach the self-improvement review to recognize coding sessions 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. --- agent/background_review.py | 115 +++++++++++++++++- .../test_background_review_coding_signal.py | 104 ++++++++++++++++ 2 files changed, 215 insertions(+), 4 deletions(-) create mode 100644 tests/run_agent/test_background_review_coding_signal.py diff --git a/agent/background_review.py b/agent/background_review.py index ee4791d98d32..1a1ff98381f8 100644 --- a/agent/background_review.py +++ b/agent/background_review.py @@ -22,11 +22,97 @@ import json import logging import os +import re from typing import Any, Dict, List, Optional logger = logging.getLogger(__name__) +# Tool names / Bash command substrings that indicate a coding session (as +# opposed to research, scheduling, browser automation, etc.). Cheap, +# regex-free signal computed from the conversation snapshot the review fork +# already has in hand -- no need to re-derive it from raw transcript text or +# 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"} +_TEST_RUNNER_PATTERN = re.compile( + r"\b(pytest|py\.test|python -m pytest|npm test|npm run test|yarn test|" + r"go test|cargo test|jest|mocha|rspec|dotnet test|gradle test|mvn test)\b", + re.IGNORECASE, +) +_MAX_CODING_SIGNAL_ENTRIES = 6 + + +def _detect_coding_signal(messages_snapshot: List[Dict]) -> str: + """Scan the conversation snapshot for edit/test tool-call activity. + + Returns a short hint block to append to the review prompt when the + session looks like a coding task, or "" when it doesn't. This gives the + review model a pre-computed signal instead of making it infer "was this + a coding session" cold from raw transcript text, and lets the prompt + carry a couple of concrete (command, outcome) pairs so a debugging + lesson can be captured with the actual error/fix rather than a vague + paraphrase. + """ + entries: List[str] = [] + saw_code_edit = False + saw_test_run = False + + for i, msg in enumerate(messages_snapshot or []): + if not isinstance(msg, dict) or msg.get("role") != "assistant": + continue + for tc in msg.get("tool_calls", []) or []: + if not isinstance(tc, dict): + continue + fn = tc.get("function", {}) or {} + fn_name = fn.get("name", "") + if fn_name in _CODE_EDIT_TOOLS: + saw_code_edit = True + continue + if fn_name != "Bash": + continue + try: + args = json.loads(fn.get("arguments", "{}")) + except (json.JSONDecodeError, TypeError): + continue + command = args.get("command", "") or "" + if not _TEST_RUNNER_PATTERN.search(command): + continue + saw_test_run = True + if len(entries) >= _MAX_CODING_SIGNAL_ENTRIES: + continue + # Pull the matching tool result (next tool message with this + # call's id) for a one-line pass/fail outcome. + outcome = "" + tcid = tc.get("id") + for later in messages_snapshot[i + 1:i + 3]: + if isinstance(later, dict) and later.get("role") == "tool" and later.get("tool_call_id") == tcid: + result_text = str(later.get("content", "")) + outcome = result_text[:200].replace("\n", " ") + break + entries.append(f" • `{command[:150]}`" + (f" → {outcome}" if outcome else "")) + + if not (saw_code_edit or saw_test_run): + return "" + + lines = [ + "\n\nCoding-session signal: this conversation included " + + ("file edits and " if saw_code_edit else "") + + ("test-runner commands" if saw_test_run else "") + + ". Before deciding where a lesson belongs, check the bundled " + "`systematic-debugging` and `test-driven-development` skills " + "(skill_view) -- if the fix/technique fits their territory, add it " + "as a `references/.md` file under them (SKILL.md itself is " + "protected, but reference files are not -- see the protected-skills " + "note below) instead of spinning up a new narrow skill.", + ] + if entries: + lines.append("Test/debug commands observed this session:") + lines.extend(entries[:_MAX_CODING_SIGNAL_ENTRIES]) + return "\n".join(lines) + + # Review-prompt strings — used by ``spawn_background_review_thread`` to build # the user-message that the forked review agent receives. AIAgent exposes # them as class attributes (``_MEMORY_REVIEW_PROMPT`` etc.) for back-compat; @@ -112,14 +198,23 @@ "skill that governs that task needs to carry the lesson.\n\n" "If you notice two existing skills that overlap, note it in your " "reply — the background curator handles consolidation at scale.\n\n" - "Protected skills (DO NOT edit these):\n" + "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/.md` file under a " + "protected skill via skill_manage action=write_file. This is how " + "session-specific technique detail (a debugging path, a " + "language-specific gotcha, a test-writing pattern) accumulates under " + "an authoritative umbrella like 'systematic-debugging' or " + "'test-driven-development' without ever touching their protected " + "SKILL.md body. Add the one-line pointer in your reply so it's " + "visible for review, not by editing the protected SKILL.md.\n" "Pinned skills (marked via 'hermes curator pin') CAN be improved — " "pin only blocks deletion/archive/consolidation by the curator, not " "content updates. Patch them when a pitfall or missing step turns up, " "same as any other agent-created skill.\n" - "If the only skills that need updating are protected, say\n" + "If the only skills that need updating are protected and the lesson " + "doesn't fit a references/ addition, say\n" "'Nothing to save.' and stop.\n\n" "Do NOT capture (these become persistent self-imposed constraints " "that bite you later when the environment changes):\n" @@ -198,14 +293,21 @@ "should carry user-preference lessons when relevant.\n\n" "If you notice overlapping existing skills, mention it — the " "background curator handles consolidation.\n\n" - "Protected skills (DO NOT edit these):\n" + "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/.md` file under a " + "protected skill via skill_manage action=write_file -- this is how " + "session-specific technique detail (a debugging path, a " + "language-specific gotcha, a test-writing pattern) accumulates under " + "an authoritative umbrella like 'systematic-debugging' or " + "'test-driven-development' without touching their protected SKILL.md.\n" "Pinned skills (marked via 'hermes curator pin') CAN be improved — " "pin only blocks deletion/archive/consolidation by the curator, not " "content updates. Patch them when a pitfall or missing step turns up, " "same as any other agent-created skill.\n" - "If the only skills that need updating are protected, say\n" + "If the only skills that need updating are protected and the lesson " + "doesn't fit a references/ addition, say\n" "'Nothing to save.' and stop.\n\n" "Do NOT capture as skills (these become persistent self-imposed " "constraints that bite you later when the environment changes):\n" @@ -712,6 +814,11 @@ def spawn_background_review_thread( else: prompt = getattr(agent, "_SKILL_REVIEW_PROMPT", _SKILL_REVIEW_PROMPT) + # Skill review (standalone or combined) benefits from a pre-computed + # coding-session hint; a memory-only pass doesn't touch skills at all. + if review_skills: + prompt = prompt + _detect_coding_signal(messages_snapshot) + def _target() -> None: _run_review_in_thread(agent, messages_snapshot, prompt) diff --git a/tests/run_agent/test_background_review_coding_signal.py b/tests/run_agent/test_background_review_coding_signal.py new file mode 100644 index 000000000000..7e1b02cc8d6e --- /dev/null +++ b/tests/run_agent/test_background_review_coding_signal.py @@ -0,0 +1,104 @@ +"""Tests for the coding-session signal fed into the skill-review prompt +(agent/background_review.py: _detect_coding_signal, spawn_background_review_thread).""" + +from __future__ import annotations + +import json + +from agent.background_review import _detect_coding_signal, spawn_background_review_thread + + +def _assistant_tool_call(tool_name, arguments, call_id="call_1"): + return { + "role": "assistant", + "content": None, + "tool_calls": [ + { + "id": call_id, + "type": "function", + "function": {"name": tool_name, "arguments": json.dumps(arguments)}, + } + ], + } + + +def _tool_result(call_id, content): + return {"role": "tool", "tool_call_id": call_id, "content": content} + + +def test_no_signal_for_non_coding_conversation(): + snapshot = [ + {"role": "user", "content": "What's the weather like?"}, + {"role": "assistant", "content": "Sunny."}, + ] + assert _detect_coding_signal(snapshot) == "" + + +def test_signal_on_file_edit_tool_call(): + snapshot = [ + {"role": "user", "content": "fix the bug"}, + _assistant_tool_call("Edit", {"file_path": "foo.py", "old_string": "a", "new_string": "b"}), + ] + signal = _detect_coding_signal(snapshot) + assert "Coding-session signal" in signal + assert "systematic-debugging" in signal + assert "test-driven-development" in signal + + +def test_signal_on_test_runner_bash_call_includes_outcome(): + snapshot = [ + {"role": "user", "content": "run the tests"}, + _assistant_tool_call("Bash", {"command": "pytest tests/test_foo.py -q"}, call_id="call_9"), + _tool_result("call_9", "1 failed, 3 passed\nAssertionError: expected 2 got 3"), + ] + signal = _detect_coding_signal(snapshot) + assert "pytest tests/test_foo.py -q" in signal + assert "AssertionError" in signal + + +def test_no_signal_for_non_test_bash_call(): + snapshot = [ + _assistant_tool_call("Bash", {"command": "ls -la"}, call_id="call_2"), + ] + assert _detect_coding_signal(snapshot) == "" + + +def test_signal_appended_to_skill_prompt_only(monkeypatch): + from run_agent import AIAgent + + agent = object.__new__(AIAgent) + agent._SKILL_REVIEW_PROMPT = "SKILL BASE" + agent._COMBINED_REVIEW_PROMPT = "COMBINED BASE" + agent._MEMORY_REVIEW_PROMPT = "MEMORY BASE" + + coding_snapshot = [_assistant_tool_call("Edit", {"file_path": "x.py"})] + + _, skill_prompt = spawn_background_review_thread( + agent, coding_snapshot, review_memory=False, review_skills=True, + ) + assert skill_prompt.startswith("SKILL BASE") + assert "Coding-session signal" in skill_prompt + + _, combined_prompt = spawn_background_review_thread( + agent, coding_snapshot, review_memory=True, review_skills=True, + ) + assert combined_prompt.startswith("COMBINED BASE") + assert "Coding-session signal" in combined_prompt + + _, memory_prompt = spawn_background_review_thread( + agent, coding_snapshot, review_memory=True, review_skills=False, + ) + assert memory_prompt == "MEMORY BASE" + + +def test_no_signal_appended_for_non_coding_session(): + from run_agent import AIAgent + + agent = object.__new__(AIAgent) + agent._SKILL_REVIEW_PROMPT = "SKILL BASE" + + plain_snapshot = [{"role": "user", "content": "summarize this article"}] + _, prompt = spawn_background_review_thread( + agent, plain_snapshot, review_memory=False, review_skills=True, + ) + assert prompt == "SKILL BASE"