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 <konstantin.khlopkov93@gmail.com>
This commit is contained in:
@@ -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 []
|
||||
|
||||
@@ -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) == []
|
||||
@@ -307,6 +307,11 @@ display:
|
||||
> writes to your memory/skill stores, are unaffected by this setting. Set it
|
||||
> per-platform via `display.platforms.<platform>.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
|
||||
|
||||
Reference in New Issue
Block a user