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

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/<topic>.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/<topic>.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/<topic>.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)
 

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 <id> or drop with /skills reject <id>.
         "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.

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"

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:

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.

tools/write_approval.py

@@ -77,9 +77,23 @@ def write_approval_enabled(subsystem: str) -> bool:
     Reads ``<subsystem>.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).