From ab98a92a45f2b5de1034b18b769e2ef6f70bafc3 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Mon, 7 Sep 2026 02:06:50 -0700 Subject: [PATCH] fix(notifications): report applied skill batch operations Use successful applied result records rather than requested operations, and keep staged writes silent. Include legacy delete/write messages. Fixes #104506 Co-authored-by: Konstantin Khlopkov --- agent/background_review.py | 22 +++++++++- .../test_skill_applied_notifications.py | 41 +++++++++++++++++++ website/docs/user-guide/features/memory.md | 5 +++ 3 files changed, 67 insertions(+), 1 deletion(-) create mode 100644 tests/run_agent/test_skill_applied_notifications.py diff --git a/agent/background_review.py b/agent/background_review.py index b812b3cf11..9ed654f098 100644 --- a/agent/background_review.py +++ b/agent/background_review.py @@ -612,11 +612,31 @@ def _prior_tool_keys(prior_snapshot: List[Dict]) -> Tuple[set, set]: def _action_lines(data: Dict, detail: Dict, verbose: bool) -> List[str]: """Summary line(s) for one successful notify-tool result (``[]`` when nothing to report).""" + if data.get("staged"): + return [] message = data.get("message", "") target = data.get("target", "") or detail.get("target", "") is_skill = detail.get("tool") == "skill_manage" + if is_skill and "results" in data: + # The requested operations are not evidence of applied writes (approval + # and atomic rollback can leave all of them unapplied). + verbs = {"create": "created", "patch": "patched", "edit": "rewritten", + "write_file": "written", "remove_file": "removed", "delete": "deleted"} + results = data.get("results") + if not data.get("operations_applied") or not isinstance(results, list): + return [] + lines = [] + for result in results: + if not isinstance(result, dict) or result.get("success") is not True: + continue + verb = verbs.get(result.get("action")) + if verb and result.get("name"): + path = f" ({result['file_path']})" if result.get("file_path") else "" + lines.append(f"Skill '{result['name']}' {verb}{path}") + return lines lower = message.lower() - if not verbose and ("created" in lower or "updated" in lower or (is_skill and "patched" in lower)): + if not verbose and ("created" in lower or "updated" in lower or + (is_skill and any(word in lower for word in ("patched", "deleted", "written")))): return [message] if not is_skill and not target: return [] diff --git a/tests/run_agent/test_skill_applied_notifications.py b/tests/run_agent/test_skill_applied_notifications.py new file mode 100644 index 0000000000..60fbb2c906 --- /dev/null +++ b/tests/run_agent/test_skill_applied_notifications.py @@ -0,0 +1,41 @@ +import json +from agent.background_review import summarize_background_review_actions +from tools.skill_manager_tool import skill_manage + + +def _messages(args, data): + return [{"role": "assistant", "tool_calls": [{"id": "skill", "function": { + "name": "skill_manage", "arguments": json.dumps(args)}}]}, + {"role": "tool", "tool_call_id": "skill", "content": json.dumps(data)}] + + +def test_applied_skill_operations_notify_with_names(tmp_path, monkeypatch): + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + name = "notify-contract" + content = f"---\nname: {name}\ndescription: Use when checking notices. Verify applied writes.\n---\nRead the sample before editing.\n" + operations = [{"name": name, "action": "create", "content": content}, + {"name": name, "action": "patch", "old_string": "sample", "new_string": "example"}, + {"name": name, "action": "write_file", "file_path": "references/a.md", "file_content": "Check the example."}, + {"name": name, "action": "remove_file", "file_path": "references/a.md"}, + {"name": name, "action": "delete"}] + for op in operations: + data = json.loads(skill_manage(action="", name="", operations=[op])) + assert data["success"], data + messages = _messages({"operations": [op]}, data) + for mode in ("on", "verbose"): + actions = summarize_background_review_actions(messages, [], mode) + assert actions and all(name in line and "?" not in line for line in actions) + assert summarize_background_review_actions(messages, [], "off") == [] + assert not (tmp_path / "skills" / name).exists() + + +def test_unapplied_skill_operations_never_notify(): + args = {"operations": [{"name": "pending", "action": "create"}]} + for data in ( + {"success": True, "staged": True, "message": "Write staged for approval."}, + {"success": False, "results": [{"success": True, "name": "pending", "action": "create"}]}, + {"success": True, "operations_applied": 0, "results": []}, + {"success": True, "operations_applied": 1, "results": [{"success": False, "name": "pending", "action": "create"}]}, + ): + for mode in ("on", "verbose"): + assert summarize_background_review_actions(_messages(args, data), [], mode) == [] diff --git a/website/docs/user-guide/features/memory.md b/website/docs/user-guide/features/memory.md index 7ff094bb78..0a5a4445c0 100644 --- a/website/docs/user-guide/features/memory.md +++ b/website/docs/user-guide/features/memory.md @@ -307,6 +307,11 @@ display: > writes to your memory/skill stores, are unaffected by this setting. Set it > per-platform via `display.platforms..memory_notifications`. +Successful skill batches name each applied operation in both `on` and `verbose` +mode, including supporting-file writes/removals and skill deletion. Staged writes +awaiting approval and rolled-back batches are not reported as completed changes. +Batch summaries use the applied results rather than assuming requested writes ran. + ## Running the review on a cheaper model (`auxiliary.background_review`) The review runs on your **main chat model** by default, replaying the