feat(memory,skills): approve/deny gate for memory + skill writes (#38199)
Adds memory.write_mode and skills.write_mode (on|off|approve), applied to both foreground turns and the background self-improvement review fork — the source of the unprompted 'wrong assumption' saves users reported. - on (default): write freely, unchanged behaviour - off: never write; the tool returns a clean disabled result - approve: don't commit. Memory foreground writes prompt inline (small, reviewable in a chat bubble); background memory writes and ALL skill writes stage to a pending store instead (a SKILL.md is too large to review inline, and a daemon thread can't block on a prompt) Review staged writes from CLI or any messaging platform: /memory pending|approve|reject|mode /skills pending|approve|reject|diff|mode Skill review respects the size asymmetry: inline you see a one-line gist; the full unified diff stays out-of-band (/skills diff, dashboard, or the staged JSON file). New: tools/write_approval.py (gate + pending store), hermes_cli/ write_approval_commands.py (shared CLI+gateway handlers). Gates wired at the single entry points memory_tool() and skill_manage(), using the existing write-origin ContextVar to distinguish foreground from background_review.
This commit is contained in:
@@ -1301,9 +1301,46 @@ class CLICommandsMixin:
|
||||
def _handle_skills_command(self, cmd: str):
|
||||
"""Handle /skills slash command — delegates to hermes_cli.skills_hub."""
|
||||
from cli import ChatConsole
|
||||
# Intercept write-approval review subcommands first (pending/approve/
|
||||
# reject/diff/mode); everything else goes to the skills hub.
|
||||
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"}:
|
||||
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),
|
||||
)
|
||||
if out is not None:
|
||||
print(out)
|
||||
return
|
||||
from hermes_cli.skills_hub import handle_skills_slash
|
||||
handle_skills_slash(cmd, ChatConsole())
|
||||
|
||||
def _handle_memory_command(self, cmd: str):
|
||||
"""Handle /memory slash command — pending review + write-mode control."""
|
||||
from hermes_cli.write_approval_commands import handle_pending_subcommand
|
||||
from tools import write_approval as wa
|
||||
parts = cmd.strip().split()
|
||||
args = parts[1:] if len(parts) > 1 else []
|
||||
store = getattr(self.agent, "_memory_store", None) if getattr(self, "agent", None) else None
|
||||
out = handle_pending_subcommand(
|
||||
wa.MEMORY, args,
|
||||
memory_store=store,
|
||||
set_mode_fn=lambda m: self._save_write_mode("memory", m),
|
||||
)
|
||||
if out is None:
|
||||
out = ("Unknown /memory subcommand. "
|
||||
"Use: pending, approve <id>, reject <id>, mode <on|off|approve>.")
|
||||
print(out)
|
||||
|
||||
def _save_write_mode(self, subsystem: str, mode: str):
|
||||
"""Persist <subsystem>.write_mode to config (for /memory|/skills mode)."""
|
||||
from cli import save_config_value
|
||||
save_config_value(f"{subsystem}.write_mode", mode)
|
||||
|
||||
def _handle_background_command(self, cmd: str):
|
||||
"""Handle /background <prompt> — run a prompt in a separate background session.
|
||||
|
||||
|
||||
@@ -167,7 +167,12 @@ COMMAND_REGISTRY: list[CommandDef] = [
|
||||
cli_only=True),
|
||||
CommandDef("skills", "Search, install, inspect, or manage skills",
|
||||
"Tools & Skills", cli_only=True,
|
||||
subcommands=("search", "browse", "inspect", "install", "audit")),
|
||||
subcommands=("search", "browse", "inspect", "install", "audit",
|
||||
"pending", "approve", "reject", "diff", "mode")),
|
||||
CommandDef("memory", "Review pending memory writes / set write mode",
|
||||
"Tools & Skills",
|
||||
args_hint="[pending|approve|reject|mode] [id|on|off|approve]",
|
||||
subcommands=("pending", "approve", "reject", "mode")),
|
||||
CommandDef("bundles", "List skill bundles (aliases /<name> for multiple skills)",
|
||||
"Tools & Skills"),
|
||||
CommandDef("cron", "Manage scheduled tasks", "Tools & Skills",
|
||||
|
||||
@@ -1653,6 +1653,18 @@ DEFAULT_CONFIG = {
|
||||
"memory": {
|
||||
"memory_enabled": True,
|
||||
"user_profile_enabled": True,
|
||||
# Write gate for the memory tool (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 <id>,
|
||||
# /memory reject <id>.
|
||||
"write_mode": "on",
|
||||
"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).
|
||||
@@ -1757,6 +1769,17 @@ 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/
|
||||
# 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 <id> (full diff — CLI/dashboard/file,
|
||||
# never crammed into a chat bubble), and applied with
|
||||
# /skills approve <id> or dropped with /skills reject <id>.
|
||||
"write_mode": "on",
|
||||
},
|
||||
|
||||
# Curator — background skill maintenance.
|
||||
|
||||
@@ -0,0 +1,197 @@
|
||||
#!/usr/bin/env python3
|
||||
"""Shared handlers for the /memory and /skills write-approval subcommands.
|
||||
|
||||
Both the interactive CLI (``cli.py``) and the gateway (``gateway/run.py``) call
|
||||
into this module so the pending-review UX (list / approve / reject / diff /
|
||||
mode) lives in one place. Each caller owns only its surface concerns:
|
||||
formatting the returned text and, for the gateway, persisting config + evicting
|
||||
the cached agent on a mode change.
|
||||
|
||||
Every public handler returns a plain text string suitable for both a terminal
|
||||
and a chat message. Skill diffs are intentionally NOT inlined here — the
|
||||
``diff`` handler returns the full diff for the CLI pager, but on a messaging
|
||||
platform the gateway truncates it and points the user at the dashboard / file.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import json
|
||||
from typing import List, Optional
|
||||
|
||||
from tools import write_approval as wa
|
||||
|
||||
_VALID_MODES = (wa.MODE_ON, wa.MODE_OFF, wa.MODE_APPROVE)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Formatting helpers
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
def _fmt_pending_list(subsystem: str) -> str:
|
||||
records = wa.list_pending(subsystem)
|
||||
if not records:
|
||||
return f"No pending {subsystem} writes."
|
||||
lines = [f"Pending {subsystem} writes ({len(records)}):"]
|
||||
for r in records:
|
||||
origin = r.get("origin", "foreground")
|
||||
tag = " [auto]" if origin == "background_review" else ""
|
||||
lines.append(f" {r['id']}{tag} {r.get('summary', '')}")
|
||||
where = "/{s} approve <id>".format(s=subsystem)
|
||||
lines.append("")
|
||||
lines.append(f"Apply: {where} Reject: /{subsystem} reject <id>")
|
||||
if subsystem == wa.SKILLS:
|
||||
lines.append("Review full diff: /skills diff <id>")
|
||||
return "\n".join(lines)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Subcommand dispatch
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
def handle_pending_subcommand(
|
||||
subsystem: str,
|
||||
args: List[str],
|
||||
*,
|
||||
memory_store=None,
|
||||
set_mode_fn=None,
|
||||
) -> Optional[str]:
|
||||
"""Dispatch a /memory or /skills subcommand.
|
||||
|
||||
Args:
|
||||
subsystem: ``memory`` or ``skills``.
|
||||
args: tokens after the slash command (e.g. ``["approve", "a1b2"]``).
|
||||
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).
|
||||
|
||||
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)
|
||||
|
||||
sub = args[0].lower()
|
||||
rest = args[1:]
|
||||
|
||||
if sub == "pending":
|
||||
return _fmt_pending_list(subsystem)
|
||||
|
||||
if sub in {"approve", "apply"}:
|
||||
return _approve(subsystem, rest, memory_store)
|
||||
|
||||
if sub in {"reject", "deny", "drop"}:
|
||||
return _reject(subsystem, rest)
|
||||
|
||||
if sub == "diff" and subsystem == wa.SKILLS:
|
||||
return _diff(rest)
|
||||
|
||||
if sub == "mode":
|
||||
return _set_mode(subsystem, rest, set_mode_fn)
|
||||
|
||||
return None # not ours — caller handles
|
||||
|
||||
|
||||
def _resolve_one(subsystem: str, rest: List[str]):
|
||||
if not rest:
|
||||
return None, f"Usage: /{subsystem} approve|reject <id> (or 'all')"
|
||||
return rest[0], None
|
||||
|
||||
|
||||
def _approve(subsystem: str, rest: List[str], memory_store) -> str:
|
||||
target, err = _resolve_one(subsystem, rest)
|
||||
if err or target is None:
|
||||
return err or f"Usage: /{subsystem} approve <id>"
|
||||
|
||||
records = wa.list_pending(subsystem)
|
||||
if not records:
|
||||
return f"No pending {subsystem} writes."
|
||||
|
||||
if target.lower() == "all":
|
||||
targets = list(records)
|
||||
else:
|
||||
rec = wa.get_pending(subsystem, target)
|
||||
if not rec:
|
||||
return f"No pending {subsystem} write with id '{target}'."
|
||||
targets = [rec]
|
||||
|
||||
applied, failed = 0, []
|
||||
for rec in targets:
|
||||
ok, msg = _apply_one(subsystem, rec, memory_store)
|
||||
if ok:
|
||||
wa.discard_pending(subsystem, rec["id"])
|
||||
applied += 1
|
||||
else:
|
||||
failed.append(f"{rec['id']}: {msg}")
|
||||
|
||||
out = [f"Approved {applied} {subsystem} write(s)."]
|
||||
if failed:
|
||||
out.append("Failed:")
|
||||
out.extend(f" {f}" for f in failed)
|
||||
return "\n".join(out)
|
||||
|
||||
|
||||
def _apply_one(subsystem: str, rec, memory_store):
|
||||
payload = rec.get("payload", {})
|
||||
try:
|
||||
if subsystem == wa.MEMORY:
|
||||
if memory_store is None:
|
||||
return False, "memory store unavailable"
|
||||
from tools.memory_tool import apply_memory_pending
|
||||
result = apply_memory_pending(payload, memory_store)
|
||||
return bool(result.get("success")), result.get("error", "")
|
||||
else:
|
||||
from tools.skill_manager_tool import apply_skill_pending
|
||||
result = json.loads(apply_skill_pending(payload))
|
||||
return bool(result.get("success")), result.get("error", "")
|
||||
except Exception as e:
|
||||
return False, str(e)
|
||||
|
||||
|
||||
def _reject(subsystem: str, rest: List[str]) -> str:
|
||||
target, err = _resolve_one(subsystem, rest)
|
||||
if err or target is None:
|
||||
return err or f"Usage: /{subsystem} reject <id>"
|
||||
if target.lower() == "all":
|
||||
n = 0
|
||||
for rec in wa.list_pending(subsystem):
|
||||
if wa.discard_pending(subsystem, rec["id"]):
|
||||
n += 1
|
||||
return f"Rejected {n} pending {subsystem} write(s)."
|
||||
if wa.discard_pending(subsystem, target):
|
||||
return f"Rejected pending {subsystem} write '{target}'."
|
||||
return f"No pending {subsystem} write with id '{target}'."
|
||||
|
||||
|
||||
def _diff(rest: List[str]) -> str:
|
||||
if not rest:
|
||||
return "Usage: /skills diff <id>"
|
||||
rec = wa.get_pending(wa.SKILLS, rest[0])
|
||||
if not rec:
|
||||
return f"No pending skill write with id '{rest[0]}'."
|
||||
diff = wa.skill_pending_diff(rec)
|
||||
header = f"# Pending skill write {rec['id']}: {rec.get('summary', '')}\n"
|
||||
return header + "\n" + diff
|
||||
|
||||
|
||||
def _set_mode(subsystem: str, rest: List[str], set_mode_fn) -> str:
|
||||
if not rest:
|
||||
cur = wa.get_write_mode(subsystem)
|
||||
return (f"{subsystem}.write_mode = {cur}\n"
|
||||
f"Set with: /{subsystem} mode <on|off|approve>")
|
||||
mode = rest[0].lower()
|
||||
if mode not in _VALID_MODES:
|
||||
return f"Invalid mode '{mode}'. Use: on, off, approve."
|
||||
if set_mode_fn is None:
|
||||
return (f"To change {subsystem} write mode, run:\n"
|
||||
f" hermes config set {subsystem}.write_mode {mode}")
|
||||
try:
|
||||
set_mode_fn(mode)
|
||||
except Exception as e:
|
||||
return f"Failed to set {subsystem}.write_mode: {e}"
|
||||
return f"{subsystem}.write_mode set to '{mode}'."
|
||||
Reference in New Issue
Block a user