From 095f526b112ebc51c507e069d08abd025362bd3d Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Tue, 9 Jun 2026 23:21:14 -0700 Subject: [PATCH] refactor(memory,skills): replace tri-state write_mode with boolean write_approval (default off) (#43354) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The shipped tri-state write_mode (on|off|approve) conflated two concepts — whether writes are enabled and whether they're gated — so 'on' (writes flow freely, gate inactive) read like 'gating is on'. Replace it with a single clear boolean gate that defaults off. memory.write_approval / skills.write_approval: false (default) — write freely; the approval gate is off (pre-gate behaviour) true — require approval: memory foreground prompts inline, memory background-review + all skill writes stage for review The old 'off = block all writes' mode is dropped; memory_enabled: false already disables memory entirely, so a third 'block' state was redundant. - tools/write_approval.py: get_write_mode/MODE_* → write_approval_enabled() bool; evaluate_gate() loses the config-driven 'blocked' path (blocked now only comes from an interactive user denial). - tools/memory_tool.py, tools/skill_manager_tool.py: comment + behaviour follow. - hermes_cli/config.py: memory/skills write_mode → write_approval (False); _config_version 28→29 with a 28→29 migration that renames any persisted write_mode (approve→true, on/off/unset→false) and drops the old key. - slash commands: '/memory|/skills mode ' → 'approval ' ('mode' kept as a back-compat alias); set_mode_fn callback now takes a bool. - write_approval_commands.py, cli_commands_mixin.py, gateway/slash_commands.py, commands.py: handlers + registry args/subcommands updated. - docs + tests rewritten for the boolean model; added migration tests. --- gateway/slash_commands.py | 18 +-- hermes_cli/cli_commands_mixin.py | 16 +-- hermes_cli/commands.py | 8 +- hermes_cli/config.py | 74 +++++++---- hermes_cli/write_approval_commands.py | 54 +++++---- tests/hermes_cli/test_config.py | 47 +++++++ tests/tools/test_write_approval.py | 135 +++++++++++++-------- tools/memory_tool.py | 4 +- tools/skill_manager_tool.py | 8 +- tools/write_approval.py | 80 ++++++------ website/docs/user-guide/features/memory.md | 39 +++--- 11 files changed, 296 insertions(+), 187 deletions(-) diff --git a/gateway/slash_commands.py b/gateway/slash_commands.py index 7dddac9b7c7..3c7436498e1 100644 --- a/gateway/slash_commands.py +++ b/gateway/slash_commands.py @@ -1955,12 +1955,12 @@ class GatewaySlashCommandsMixin: return t("gateway.reasoning.set_session", effort=effort) async def _handle_memory_command(self, event: MessageEvent) -> str: - """Handle /memory — review pending memory writes + set write mode. + """Handle /memory — review pending memory writes + toggle the approval gate. Memory entries are small enough to review inline in a chat bubble, so - the full pending/approve/reject/mode flow works on every platform. - Mode changes persist to config.yaml and evict the cached agent so the - new write_mode takes effect on the next message. + the full pending/approve/reject/approval flow works on every platform. + Gate changes persist to config.yaml and evict the cached agent so the + new setting takes effect on the next message. """ from gateway.run import _hermes_home from hermes_cli.write_approval_commands import handle_pending_subcommand @@ -1972,15 +1972,15 @@ class GatewaySlashCommandsMixin: session_key = self._session_key_for_source(event.source) config_path = _hermes_home / "config.yaml" - def _set_mode(mode: str): + def _set_approval(enabled: bool): import yaml user_config = {} if config_path.exists(): with open(config_path, encoding="utf-8") as f: user_config = yaml.safe_load(f) or {} - user_config.setdefault("memory", {})["write_mode"] = mode + user_config.setdefault("memory", {})["write_approval"] = bool(enabled) atomic_yaml_write(config_path, user_config) - # New write_mode must take effect next message → drop cached agent. + # New setting must take effect next message → drop cached agent. self._evict_cached_agent(session_key) # Apply approved writes against a fresh on-disk store (the gateway has @@ -1989,11 +1989,11 @@ class GatewaySlashCommandsMixin: store.load_from_disk() out = handle_pending_subcommand( - wa.MEMORY, args, memory_store=store, set_mode_fn=_set_mode, + wa.MEMORY, args, memory_store=store, set_mode_fn=_set_approval, ) if out is None: out = ("Unknown /memory subcommand. Use: pending, approve , " - "reject , mode .") + "reject , approval .") return out async def _handle_fast_command(self, event: MessageEvent) -> str: diff --git a/hermes_cli/cli_commands_mixin.py b/hermes_cli/cli_commands_mixin.py index 3bbf4eae002..ffb39d9e956 100644 --- a/hermes_cli/cli_commands_mixin.py +++ b/hermes_cli/cli_commands_mixin.py @@ -1306,12 +1306,12 @@ class CLICommandsMixin: parts = cmd.strip().split() args = parts[1:] if len(parts) > 1 else [] if args and args[0].lower() in {"pending", "approve", "apply", "reject", - "deny", "drop", "diff", "mode"}: + "deny", "drop", "diff", "approval", "mode"}: from hermes_cli.write_approval_commands import handle_pending_subcommand from tools import write_approval as wa out = handle_pending_subcommand( wa.SKILLS, args, - set_mode_fn=lambda m: self._save_write_mode("skills", m), + set_mode_fn=lambda enabled: self._save_write_approval("skills", enabled), ) if out is not None: print(out) @@ -1320,7 +1320,7 @@ class CLICommandsMixin: handle_skills_slash(cmd, ChatConsole()) def _handle_memory_command(self, cmd: str): - """Handle /memory slash command — pending review + write-mode control.""" + """Handle /memory slash command — pending review + approval-gate toggle.""" from hermes_cli.write_approval_commands import handle_pending_subcommand from tools import write_approval as wa parts = cmd.strip().split() @@ -1329,17 +1329,17 @@ class CLICommandsMixin: out = handle_pending_subcommand( wa.MEMORY, args, memory_store=store, - set_mode_fn=lambda m: self._save_write_mode("memory", m), + set_mode_fn=lambda enabled: self._save_write_approval("memory", enabled), ) if out is None: out = ("Unknown /memory subcommand. " - "Use: pending, approve , reject , mode .") + "Use: pending, approve , reject , approval .") print(out) - def _save_write_mode(self, subsystem: str, mode: str): - """Persist .write_mode to config (for /memory|/skills mode).""" + def _save_write_approval(self, subsystem: str, enabled: bool): + """Persist .write_approval to config (for /memory|/skills approval).""" from cli import save_config_value - save_config_value(f"{subsystem}.write_mode", mode) + save_config_value(f"{subsystem}.write_approval", bool(enabled)) def _handle_background_command(self, cmd: str): """Handle /background — run a prompt in a separate background session. diff --git a/hermes_cli/commands.py b/hermes_cli/commands.py index ba7c99fe1b5..520f4fac071 100644 --- a/hermes_cli/commands.py +++ b/hermes_cli/commands.py @@ -168,11 +168,11 @@ COMMAND_REGISTRY: list[CommandDef] = [ CommandDef("skills", "Search, install, inspect, or manage skills", "Tools & Skills", cli_only=True, subcommands=("search", "browse", "inspect", "install", "audit", - "pending", "approve", "reject", "diff", "mode")), - CommandDef("memory", "Review pending memory writes / set write mode", + "pending", "approve", "reject", "diff", "approval")), + CommandDef("memory", "Review pending memory writes / toggle the approval gate", "Tools & Skills", - args_hint="[pending|approve|reject|mode] [id|on|off|approve]", - subcommands=("pending", "approve", "reject", "mode")), + args_hint="[pending|approve|reject|approval] [id|on|off]", + subcommands=("pending", "approve", "reject", "approval")), CommandDef("bundles", "List skill bundles (aliases / for multiple skills)", "Tools & Skills"), CommandDef("cron", "Manage scheduled tasks", "Tools & Skills", diff --git a/hermes_cli/config.py b/hermes_cli/config.py index a8cb187aa92..2cbb00468b2 100644 --- a/hermes_cli/config.py +++ b/hermes_cli/config.py @@ -1653,18 +1653,19 @@ DEFAULT_CONFIG = { "memory": { "memory_enabled": True, "user_profile_enabled": True, - # Write gate for the memory tool (add/replace/remove), applied to BOTH + # Approval gate for memory writes (add/replace/remove), applied to BOTH # foreground agent turns and the background self-improvement review fork - # (the source of unprompted "wrong assumption" saves users reported): - # on — write freely (default, current behaviour) - # off — never write; the memory tool returns a clean disabled result - # approve — foreground writes block on an inline approve/deny prompt - # (entries are small enough to review in a chat bubble); - # background-review writes are staged for review instead of - # committed (a daemon thread cannot block on a prompt). - # Pending entries: /memory pending, /memory approve , - # /memory reject . - "write_mode": "on", + # (the source of unprompted "wrong assumption" saves users reported). + # false (default) — write freely; the gate is off (pre-gate behaviour) + # true — require approval: foreground writes prompt inline + # (entries are small enough to review in a chat + # bubble); background-review writes are staged + # instead of committed (a daemon thread cannot block + # on a prompt). Review staged entries with + # /memory pending, /memory approve , + # /memory reject . + # To disable memory entirely, use memory_enabled: false instead. + "write_approval": False, "memory_char_limit": 2200, # ~800 tokens at 2.75 chars/token "user_char_limit": 1375, # ~500 tokens at 2.75 chars/token # External memory provider plugin (empty = built-in only). @@ -1769,17 +1770,18 @@ DEFAULT_CONFIG = { # External hub installs (trusted/community sources) are always # scanned regardless of this setting. "guard_agent_created": False, - # Write gate for skill_manage (create/edit/patch/write_file/delete/ + # Approval gate for skill_manage (create/edit/patch/write_file/delete/ # remove_file), applied to BOTH foreground agent turns and the - # background self-improvement review fork: - # on — write freely (default, current behaviour) - # off — never write; skill_manage returns a clean disabled result - # approve — stage the write for review instead of committing. - # Pending skills are listed with /skills pending, reviewed - # with /skills diff (full diff — CLI/dashboard/file, - # never crammed into a chat bubble), and applied with - # /skills approve or dropped with /skills reject . - "write_mode": "on", + # background self-improvement review fork. + # false (default) — write freely; the gate is off (pre-gate behaviour) + # true — require approval: stage the write for review + # instead of committing (a SKILL.md is too large to + # review inline, so skills always stage rather than + # prompt). List with /skills pending, inspect with + # /skills diff (full diff — CLI/dashboard/file, + # never crammed into a chat bubble), apply with + # /skills approve or drop with /skills reject . + "write_approval": False, }, # Curator — background skill maintenance. @@ -2463,7 +2465,7 @@ DEFAULT_CONFIG = { # Config schema version - bump this when adding new required fields - "_config_version": 28, + "_config_version": 29, } # ============================================================================= @@ -4734,6 +4736,34 @@ def migrate_config(interactive: bool = True, quiet: bool = False) -> Dict[str, A if not quiet: print(" ✓ Lowered model_catalog.ttl_hours to 1 (hourly picker refresh)") + # ── Version 28 → 29: rename memory/skills write_mode → write_approval ── + # The tri-state write_mode (on|off|approve) was replaced by a clear boolean + # write_approval (default false = gate off, writes flow freely; true = + # require approval). Only an explicit "approve" carried gating intent, so + # it maps to true; everything else (on/off/unset) → false. The old + # "off = block all writes" mode is dropped — memory_enabled: false disables + # memory entirely. Only rewrite a key the user actually persisted; never + # invent one. + if current_ver < 29: + config = read_raw_config() + touched = False + for subsystem in ("memory", "skills"): + sub = config.get(subsystem) + if not isinstance(sub, dict) or "write_mode" not in sub: + continue + old = sub.pop("write_mode") + old_norm = old.strip().lower() if isinstance(old, str) else old + sub["write_approval"] = (old_norm == "approve") + config[subsystem] = sub + touched = True + results["config_added"].append( + f"{subsystem}.write_mode → write_approval={sub['write_approval']}" + ) + if touched: + save_config(config) + if not quiet: + print(" ✓ Renamed write_mode → write_approval (boolean gate)") + if current_ver < latest_ver and not quiet: print(f"Config version: {current_ver} → {latest_ver}") diff --git a/hermes_cli/write_approval_commands.py b/hermes_cli/write_approval_commands.py index e709a690687..5f399e0cc53 100644 --- a/hermes_cli/write_approval_commands.py +++ b/hermes_cli/write_approval_commands.py @@ -20,7 +20,10 @@ from typing import List, Optional from tools import write_approval as wa -_VALID_MODES = (wa.MODE_ON, wa.MODE_OFF, wa.MODE_APPROVE) + +def _fmt_state(subsystem: str) -> str: + on = wa.write_approval_enabled(subsystem) + return f"{subsystem}.write_approval = {'on' if on else 'off'}" # --------------------------------------------------------------------------- @@ -63,18 +66,17 @@ def handle_pending_subcommand( memory_store: live MemoryStore for applying approved memory writes (CLI passes ``self.agent._memory_store``; gateway applies against a freshly loaded store). - set_mode_fn: optional callable ``(mode: str) -> None`` that persists the - new write_mode to config (gateway provides this; CLI uses its own - ``save_config_value`` and passes a closure). + set_mode_fn: optional callable ``(enabled: bool) -> None`` that + persists the new write_approval boolean to config (gateway provides + this; CLI uses its own ``save_config_value`` and passes a closure). Returns a text string to show the user. Returns None when the args are not a write-approval subcommand (caller falls through to its other handling, e.g. /skills search). """ if not args: - # Bare /memory or /skills with no sub → show pending + current mode. - mode = wa.get_write_mode(subsystem) - return f"{subsystem}.write_mode = {mode}\n\n" + _fmt_pending_list(subsystem) + # Bare /memory or /skills with no sub → show pending + gate state. + return f"{_fmt_state(subsystem)}\n\n" + _fmt_pending_list(subsystem) sub = args[0].lower() rest = args[1:] @@ -91,8 +93,8 @@ def handle_pending_subcommand( if sub == "diff" and subsystem == wa.SKILLS: return _diff(rest) - if sub == "mode": - return _set_mode(subsystem, rest, set_mode_fn) + if sub in {"approval", "mode"}: # 'mode' kept as a back-compat alias + return _set_approval(subsystem, rest, set_mode_fn) return None # not ours — caller handles @@ -179,19 +181,29 @@ def _diff(rest: List[str]) -> str: return header + "\n" + diff -def _set_mode(subsystem: str, rest: List[str], set_mode_fn) -> str: +def _set_approval(subsystem: str, rest: List[str], set_mode_fn) -> str: + """Turn the approval gate on/off for a subsystem. + + ``set_mode_fn`` (when provided) persists the new boolean to config. + """ if not rest: - cur = wa.get_write_mode(subsystem) - return (f"{subsystem}.write_mode = {cur}\n" - f"Set with: /{subsystem} mode ") - mode = rest[0].lower() - if mode not in _VALID_MODES: - return f"Invalid mode '{mode}'. Use: on, off, approve." + return (f"{_fmt_state(subsystem)}\n" + f"Set with: /{subsystem} approval ") + arg = rest[0].strip().lower() + truthy = {"on", "true", "yes", "1", "enable", "enabled"} + falsey = {"off", "false", "no", "0", "disable", "disabled"} + if arg in truthy: + enabled = True + elif arg in falsey: + enabled = False + else: + return f"Invalid value '{arg}'. Use: on or off." if set_mode_fn is None: - return (f"To change {subsystem} write mode, run:\n" - f" hermes config set {subsystem}.write_mode {mode}") + val = "true" if enabled else "false" + return (f"To change the {subsystem} approval gate, run:\n" + f" hermes config set {subsystem}.write_approval {val}") try: - set_mode_fn(mode) + set_mode_fn(enabled) except Exception as e: - return f"Failed to set {subsystem}.write_mode: {e}" - return f"{subsystem}.write_mode set to '{mode}'." + return f"Failed to set {subsystem}.write_approval: {e}" + return f"{subsystem}.write_approval set to '{'on' if enabled else 'off'}'." diff --git a/tests/hermes_cli/test_config.py b/tests/hermes_cli/test_config.py index 1a25c1a5d2c..5d8cb4436ca 100644 --- a/tests/hermes_cli/test_config.py +++ b/tests/hermes_cli/test_config.py @@ -1075,3 +1075,50 @@ class TestEnvWriteDenylist: # But the write path still refuses to update it with pytest.raises(ValueError, match="denylist"): save_env_value("LD_PRELOAD", "/tmp/evil.so") + + +class TestWriteApprovalMigration: + """Version 28→29 renames memory/skills write_mode → write_approval (bool). + + Only an explicit ``approve`` carried gating intent and maps to ``True``; + ``on``/``off``/unset map to ``False`` (gate off). The old ``write_mode`` key + is removed. Only a persisted key is rewritten — never invented. + """ + + def _write(self, tmp_path, body: str): + (tmp_path / "config.yaml").write_text(body) + + def test_approve_maps_to_true(self, tmp_path): + with patch.dict(os.environ, {"HERMES_HOME": str(tmp_path)}): + self._write(tmp_path, + "_config_version: 28\nmemory:\n write_mode: approve\n" + "skills:\n write_mode: approve\n") + migrate_config(interactive=False, quiet=True) + raw = yaml.safe_load((tmp_path / "config.yaml").read_text()) + assert raw["memory"]["write_approval"] is True + assert raw["skills"]["write_approval"] is True + assert "write_mode" not in raw["memory"] + assert "write_mode" not in raw["skills"] + + def test_on_and_off_map_to_false(self, tmp_path): + with patch.dict(os.environ, {"HERMES_HOME": str(tmp_path)}): + # YAML 1.1 parses bare on/off as bools — write_mode could be either + # the string or the bool; both legacy "not gating" values → False. + self._write(tmp_path, + "_config_version: 28\nmemory:\n write_mode: 'on'\n" + "skills:\n write_mode: 'off'\n") + migrate_config(interactive=False, quiet=True) + raw = yaml.safe_load((tmp_path / "config.yaml").read_text()) + assert raw["memory"]["write_approval"] is False + assert raw["skills"]["write_approval"] is False + + def test_unset_key_defaults_to_false(self, tmp_path): + with patch.dict(os.environ, {"HERMES_HOME": str(tmp_path)}): + self._write(tmp_path, "_config_version: 28\nmemory:\n memory_enabled: true\n") + migrate_config(interactive=False, quiet=True) + raw = yaml.safe_load((tmp_path / "config.yaml").read_text()) + # No write_mode was persisted, so the rename is a no-op; the missing- + # field pass then seeds the default (False = gate off). Either way the + # gate ends up off and there's no leftover write_mode key. + assert raw["memory"].get("write_approval", False) is False + assert "write_mode" not in raw.get("memory", {}) diff --git a/tests/tools/test_write_approval.py b/tests/tools/test_write_approval.py index cbe757b3c17..d00a95e7138 100644 --- a/tests/tools/test_write_approval.py +++ b/tests/tools/test_write_approval.py @@ -1,9 +1,10 @@ """Tests for the memory/skill write-approval gate (tools/write_approval.py) and the shared slash-command handlers (hermes_cli/write_approval_commands.py). -Covers the tri-state write_mode (on/off/approve) for both subsystems, the -foreground-vs-background staging split, pending store CRUD, and the -list/approve/reject/diff/mode subcommand dispatch. +Covers the boolean write_approval gate (off by default = write freely; on = +require approval) for both subsystems, the foreground-vs-background staging +split, pending store CRUD, and the list/approve/reject/diff/approval +subcommand dispatch. """ import json @@ -24,64 +25,64 @@ def hermes_home(monkeypatch): shutil.rmtree(d, ignore_errors=True) -def _set_mode(subsystem, mode): +def _set_approval(subsystem, enabled): import hermes_cli.config as cfg c = cfg.load_config() - c.setdefault(subsystem, {})["write_mode"] = mode + c.setdefault(subsystem, {})["write_approval"] = enabled cfg.save_config(c) # --------------------------------------------------------------------------- -# Mode resolution +# Config resolution # --------------------------------------------------------------------------- -def test_default_write_mode_is_on(hermes_home): +def test_default_gate_is_off(hermes_home): from tools import write_approval as wa - assert wa.get_write_mode("memory") == "on" - assert wa.get_write_mode("skills") == "on" + # Default: gate off → writes flow freely. + assert wa.write_approval_enabled("memory") is False + assert wa.write_approval_enabled("skills") is False -def test_invalid_subsystem_returns_on(hermes_home): +def test_invalid_subsystem_is_off(hermes_home): from tools import write_approval as wa - assert wa.get_write_mode("bogus") == "on" + assert wa.write_approval_enabled("bogus") is False -def test_normalize_mode_handles_yaml_bool(): +def test_normalize_enabled_coerces_values(): from tools import write_approval as wa - assert wa._normalize_mode(False) == "off" - assert wa._normalize_mode(True) == "on" - assert wa._normalize_mode("approve") == "approve" - assert wa._normalize_mode("garbage") == "on" + # Real bools pass through. + assert wa._normalize_enabled(True) is True + assert wa._normalize_enabled(False) is False + # Truthy strings → True (incl. legacy 'approve'). + assert wa._normalize_enabled("on") is True + assert wa._normalize_enabled("approve") is True + assert wa._normalize_enabled("true") is True + # Everything else → False (gate off is the safe default). + assert wa._normalize_enabled("off") is False + assert wa._normalize_enabled("garbage") is False + assert wa._normalize_enabled(None) is False # --------------------------------------------------------------------------- # Memory gate # --------------------------------------------------------------------------- -def test_memory_off_blocks_write(hermes_home): +def test_memory_gate_off_allows_write(hermes_home): + # Default (gate off) → write straight through, no staging. from tools.memory_tool import memory_tool, MemoryStore - _set_mode("memory", "off") - store = MemoryStore(); store.load_from_disk() - r = json.loads(memory_tool("add", "user", "should not save", store=store)) - assert r["success"] is False - assert "disabled" in r["error"].lower() - assert store.user_entries == [] - - -def test_memory_on_allows_write(hermes_home): - from tools.memory_tool import memory_tool, MemoryStore - _set_mode("memory", "on") + from tools import write_approval as wa store = MemoryStore(); store.load_from_disk() r = json.loads(memory_tool("add", "user", "save me", store=store)) assert r["success"] is True assert r["entry_count"] == 1 + assert wa.pending_count("memory") == 0 -def test_memory_approve_no_interactive_stages(hermes_home): - # No approval callback registered and not a gateway context → stage. +def test_memory_gate_on_no_interactive_stages(hermes_home): + # Gate on, no approval callback / not a gateway context → stage. from tools.memory_tool import memory_tool, MemoryStore from tools import write_approval as wa - _set_mode("memory", "approve") + _set_approval("memory", True) store = MemoryStore(); store.load_from_disk() r = json.loads(memory_tool("add", "memory", "stage me", store=store)) assert r.get("staged") is True @@ -93,10 +94,10 @@ def test_memory_approve_no_interactive_stages(hermes_home): assert pend[0]["id"] == r["pending_id"] -def test_memory_approve_then_apply(hermes_home): +def test_memory_gate_on_then_apply(hermes_home): from tools.memory_tool import memory_tool, MemoryStore, apply_memory_pending from tools import write_approval as wa - _set_mode("memory", "approve") + _set_approval("memory", True) store = MemoryStore(); store.load_from_disk() r = json.loads(memory_tool("add", "user", "approved entry", store=store)) pid = r["pending_id"] @@ -116,33 +117,36 @@ _SKILL = ( ) -def test_skill_off_blocks_create(hermes_home): - from tools.skill_manager_tool import skill_manage - _set_mode("skills", "off") - r = json.loads(skill_manage("create", "blocked-skill", content=_SKILL)) - assert r["success"] is False - assert "disabled" in r["error"].lower() +def test_skill_gate_off_allows_create(hermes_home): + # Default (gate off) → skill is created normally, not staged. + import importlib + import tools.skill_manager_tool as smt + importlib.reload(smt) + from tools import write_approval as wa + r = json.loads(smt.skill_manage("create", "free-skill", content=_SKILL)) + assert r.get("success") is True + assert wa.pending_count("skills") == 0 -def test_skill_approve_always_stages(hermes_home): +def test_skill_gate_on_always_stages(hermes_home): # Skills stage even in the foreground (too big to review inline). from tools.skill_manager_tool import skill_manage from tools import write_approval as wa - _set_mode("skills", "approve") + _set_approval("skills", True) r = json.loads(skill_manage("create", "staged-skill", content=_SKILL)) assert r.get("staged") is True assert "staged-skill" in r.get("gist", "") assert wa.pending_count("skills") == 1 -def test_skill_approve_then_apply_writes_file(hermes_home): +def test_skill_gate_on_then_apply_writes_file(hermes_home): # SKILLS_DIR is resolved at import time, so reload the skill module under # this test's HERMES_HOME to exercise the real on-disk write path. import importlib import tools.skill_manager_tool as smt importlib.reload(smt) from tools import write_approval as wa - _set_mode("skills", "approve") + _set_approval("skills", True) r = json.loads(smt.skill_manage("create", "applied-skill", content=_SKILL)) rec = wa.get_pending("skills", r["pending_id"]) res = json.loads(smt.apply_skill_pending(rec["payload"])) @@ -153,7 +157,7 @@ def test_skill_approve_then_apply_writes_file(hermes_home): def test_skill_create_diff_is_full_content(hermes_home): from tools.skill_manager_tool import skill_manage from tools import write_approval as wa - _set_mode("skills", "approve") + _set_approval("skills", True) r = json.loads(skill_manage("create", "diff-skill", content=_SKILL)) rec = wa.get_pending("skills", r["pending_id"]) diff = wa.skill_pending_diff(rec) @@ -212,24 +216,49 @@ def test_handle_reject(hermes_home): assert wa.pending_count("skills") == 0 -def test_handle_mode_set(hermes_home): +def test_handle_approval_on(hermes_home): from hermes_cli.write_approval_commands import handle_pending_subcommand from tools import write_approval as wa captured = {} out = handle_pending_subcommand( - wa.MEMORY, ["mode", "approve"], - set_mode_fn=lambda m: captured.update(mode=m), + wa.MEMORY, ["approval", "on"], + set_mode_fn=lambda enabled: captured.update(enabled=enabled), ) - assert captured["mode"] == "approve" - assert "approve" in out + assert captured["enabled"] is True + assert "on" in out -def test_handle_mode_invalid(hermes_home): +def test_handle_approval_off(hermes_home): from hermes_cli.write_approval_commands import handle_pending_subcommand from tools import write_approval as wa - out = handle_pending_subcommand(wa.MEMORY, ["mode", "bogus"], - set_mode_fn=lambda m: None) - assert "Invalid mode" in out + captured = {} + out = handle_pending_subcommand( + wa.SKILLS, ["approval", "off"], + set_mode_fn=lambda enabled: captured.update(enabled=enabled), + ) + assert captured["enabled"] is False + assert "off" in out + + +def test_handle_mode_alias_still_works(hermes_home): + # 'mode' is kept as a back-compat alias for 'approval'. + from hermes_cli.write_approval_commands import handle_pending_subcommand + from tools import write_approval as wa + captured = {} + out = handle_pending_subcommand( + wa.MEMORY, ["mode", "on"], + set_mode_fn=lambda enabled: captured.update(enabled=enabled), + ) + assert captured["enabled"] is True + assert "on" in out + + +def test_handle_approval_invalid(hermes_home): + from hermes_cli.write_approval_commands import handle_pending_subcommand + from tools import write_approval as wa + out = handle_pending_subcommand(wa.MEMORY, ["approval", "bogus"], + set_mode_fn=lambda enabled: None) + assert "Invalid value" in out def test_handle_unknown_subcommand_returns_none(hermes_home): diff --git a/tools/memory_tool.py b/tools/memory_tool.py index 9f0fe2d0d5d..78ce592cea8 100644 --- a/tools/memory_tool.py +++ b/tools/memory_tool.py @@ -681,8 +681,8 @@ def memory_tool( if target not in {"memory", "user"}: return tool_error(f"Invalid target '{target}'. Use 'memory' or 'user'.", success=False) - # Write gate: off blocks the write; approve stages it (background) or - # prompts inline (foreground). on (default) passes straight through. + # Approval gate: when on, stages the write (background) or prompts inline + # (foreground); when off (default) passes straight through. gate_result = _apply_write_gate(action, target, content, old_text) if gate_result is not None: return gate_result diff --git a/tools/skill_manager_tool.py b/tools/skill_manager_tool.py index 09fc71a0da3..4f96b171e21 100644 --- a/tools/skill_manager_tool.py +++ b/tools/skill_manager_tool.py @@ -908,10 +908,10 @@ def skill_manage( Returns JSON string with results. """ - # Write gate: off blocks the write; approve stages it for review (skills are - # too large to review inline, so they always stage regardless of origin). - # on (default) passes straight through. The gate is bypassed when this call - # is itself replaying an already-approved staged write (_skill_apply_pending). + # Approval gate: when on, stages the write for review (skills are too large + # to review inline, so they always stage regardless of origin); when off + # (default) passes straight through. The gate is bypassed when this call is + # itself replaying an already-approved staged write (_skill_apply_pending). gate_result = _apply_skill_write_gate( action, name, content=content, category=category, file_path=file_path, file_content=file_content, diff --git a/tools/write_approval.py b/tools/write_approval.py index 6aa8dba4490..d5b3913d728 100644 --- a/tools/write_approval.py +++ b/tools/write_approval.py @@ -58,48 +58,48 @@ MEMORY = "memory" SKILLS = "skills" _SUBSYSTEMS = (MEMORY, SKILLS) -# Tri-state write modes -MODE_ON = "on" -MODE_OFF = "off" -MODE_APPROVE = "approve" -_VALID_MODES = (MODE_ON, MODE_OFF, MODE_APPROVE) +# Config key (per subsystem). A single boolean: the approval gate is OFF by +# default (writes flow freely, the pre-gate behaviour), and ON means stage / +# prompt every write for the user's approval. There is intentionally no third +# "block all writes" state — to disable a subsystem entirely use its own +# enable flag (e.g. ``memory.memory_enabled: false``). +CONFIG_KEY = "write_approval" # --------------------------------------------------------------------------- # Config resolution # --------------------------------------------------------------------------- -def get_write_mode(subsystem: str) -> str: - """Return the configured write_mode for ``subsystem`` (memory|skills). +def write_approval_enabled(subsystem: str) -> bool: + """Return whether the approval gate is enabled for ``subsystem``. - Reads ``.write_mode`` from config.yaml. Falls back to ``on`` - (current behaviour) for any unset / invalid value so existing installs are - unaffected until the user opts in. + 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. """ if subsystem not in _SUBSYSTEMS: - return MODE_ON + return False try: from hermes_cli.config import load_config, cfg_get cfg = load_config() - raw = cfg_get(cfg, subsystem, "write_mode", default=MODE_ON) + raw = cfg_get(cfg, subsystem, CONFIG_KEY, default=False) except Exception: - return MODE_ON - return _normalize_mode(raw) + return False + return _normalize_enabled(raw) -def _normalize_mode(value: Any) -> str: - """Coerce a config value to a valid mode string. +def _normalize_enabled(value: Any) -> bool: + """Coerce a config value to a bool. Default (unknown) is False (gate off). - YAML 1.1 parses bare ``off`` / ``on`` as booleans, so handle bools the way - the approval-mode normalizer does. + Accepts real bools and the usual truthy/falsey strings. YAML 1.1 parses + bare ``on``/``off``/``yes``/``no`` as bools already, so the string branch + is mostly for hand-edited configs. """ if isinstance(value, bool): - return MODE_OFF if value is False else MODE_ON + return value if isinstance(value, str): - v = value.strip().lower() - if v in _VALID_MODES: - return v - return MODE_ON + return value.strip().lower() in {"on", "true", "yes", "1", "approve", "enabled"} + return False # --------------------------------------------------------------------------- @@ -260,29 +260,19 @@ def evaluate_gate(subsystem: str, *, inline_summary: str = "", inline_detail: full content shown in the inline prompt (memory entries are small; skills never take the inline path). - Mode matrix: - on → allow - off → blocked - approve → memory + foreground → inline approve/deny prompt - memory + background → stage - skills (any origin) → stage (too big to review inline) - """ - mode = get_write_mode(subsystem) + Decision matrix: + gate off (default) → allow (writes flow freely) + gate on, memory + foreground → inline approve/deny prompt + gate on, memory + background → stage + gate on, skills (any origin) → stage (too big to review inline) - if mode == MODE_ON: + Note: there is no config-driven "blocked" outcome — the gate only ever + delays a write for approval, never silently refuses it. ``blocked`` is + still produced when the user *actively denies* an inline prompt. + """ + if not write_approval_enabled(subsystem): return GateDecision(allow=True) - if mode == MODE_OFF: - return GateDecision( - blocked=True, - message=( - f"{subsystem.capitalize()} writes are disabled " - f"({subsystem}.write_mode = off). The change was not saved. " - f"Set {subsystem}.write_mode to 'on' or 'approve' to allow writes." - ), - ) - - # mode == approve background = is_background() # Skills always stage — a SKILL.md is too large to review inline, and a @@ -292,7 +282,7 @@ def evaluate_gate(subsystem: str, *, inline_summary: str = "", return GateDecision( stage=True, message=( - f"Staged for approval ({subsystem}.write_mode = approve). " + f"Staged for approval ({subsystem}.write_approval is on). " f"Not yet saved — review with {where}." ), ) @@ -315,7 +305,7 @@ def evaluate_gate(subsystem: str, *, inline_summary: str = "", return GateDecision( stage=True, message=( - "Staged for approval (memory.write_mode = approve). " + "Staged for approval (memory.write_approval is on). " "Not yet saved — review with /memory pending." ), ) diff --git a/website/docs/user-guide/features/memory.md b/website/docs/user-guide/features/memory.md index f91b6538a54..ef68bf6fdae 100644 --- a/website/docs/user-guide/features/memory.md +++ b/website/docs/user-guide/features/memory.md @@ -209,21 +209,22 @@ memory: user_profile_enabled: true memory_char_limit: 2200 # ~800 tokens user_char_limit: 1375 # ~500 tokens - write_mode: on # on | off | approve + write_approval: false # false = write freely (default) | true = require approval ``` -## Controlling memory writes (`write_mode`) +## Controlling memory writes (`write_approval`) By default the agent saves memory freely — including from the background -self-improvement review that runs after a turn. If you'd rather not have -memory written behind your back, `memory.write_mode` gives you three options -(applied to **both** foreground turns and the background review): +self-improvement review that runs after a turn. If you'd rather approve saves +first, set `memory.write_approval: true`. It's a simple on/off gate applied to +**both** foreground turns and the background review: -| Mode | Behaviour | -|------|-----------| -| `on` | Write freely (default — current behaviour). | -| `off` | Never write. The memory tool returns a clean "disabled" result; nothing is saved. | -| `approve` | Don't commit writes — review them first. Foreground writes prompt you inline (entries are small enough to read in a chat bubble). Background-review writes are **staged** instead of committed (a background thread can't block on a prompt). | +| `write_approval` | Behaviour | +|------------------|-----------| +| `false` (default) | Write freely — the gate is off (the pre-gate behaviour). | +| `true` | Require approval before anything is saved. Foreground writes prompt you inline (entries are small enough to read in a chat bubble). Background-review writes are **staged** instead of committed (a background thread can't block on a prompt). | + +> To turn memory off entirely (not just gate it), set `memory_enabled: false`. Review staged writes from the CLI or any messaging platform: @@ -231,33 +232,33 @@ Review staged writes from the CLI or any messaging platform: /memory pending # list staged memory writes (auto ones tagged [auto]) /memory approve # apply one (or 'all') /memory reject # drop one (or 'all') -/memory mode approve # change write_mode and persist it +/memory approval on # turn the gate on (or 'off') and persist it ``` This is the answer to "the agent saved a wrong assumption about me": set -`write_mode: approve`, and every save — especially the unprompted background +`write_approval: true`, and every save — especially the unprompted background ones — waits for your yes/no before it ever enters your profile. -## Controlling skill writes (`skills.write_mode`) +## Controlling skill writes (`skills.write_approval`) -Skills use the same three-state gate, but the review UX differs because a +Skills use the same on/off gate, but the review UX differs because a `SKILL.md` is far too large to read in a chat bubble: ```yaml skills: - write_mode: on # on | off | approve + write_approval: false # false = write freely (default) | true = require approval ``` -In `approve` mode, skill writes (create / edit / patch / write_file / delete) -always **stage** regardless of origin. You review the one-line gist inline, but -the full diff stays out-of-band: +When `write_approval: true`, skill writes (create / edit / patch / write_file / +delete) always **stage** regardless of origin. You review the one-line gist +inline, but the full diff stays out-of-band: ``` /skills pending # list staged skill writes + a one-line gist each /skills diff # full unified diff (best viewed in CLI or dashboard) /skills approve # apply it (or 'all') /skills reject # drop it (or 'all') -/skills mode approve # change write_mode and persist it +/skills approval on # turn the gate on (or 'off') and persist it ``` On a messaging platform, approve a skill from its gist + metadata, or open