feat(ux): surface memory/skill saves in Activity summary (#3544)

Absorbs contributor PR #3544 (@rodboev, closes #3340) with two fixes: 1. DETECTION VOCAB (would never fire): the original gated on action names {save,create,update,upsert}, which don't match the real agent tool enums — memory.action is add|replace|remove, skill_manage.action is create|patch|edit|delete|write_file|remove_file. Split into per-tool predicates with the correct vocabularies: _isMemorySave gates memory on {add,replace}; _isSkillUpdate gates skill_manage on {create,patch,edit, write_file}. Deletions excluded so the saved/updated verbs stay accurate; running/errored excluded. 2. SNAPSHOT/RESTORE PERSISTENCE (Codex catch): classification lived only on the row._tcData JS property, which does NOT survive the outerHTML/innerHTML snapshot+restore the live tool-call group uses on session switch/restore — a restored memory/skill row would be re-counted as a generic tool and the suffix would silently vanish. buildToolCard now also stamps durable data-memory-save / data-skill-update attributes, and _syncToolCallGroupSummary counts them as a fallback when _tcData is absent. Verified live across a real outerHTML round-trip: label identical before/after. Replaces the PR's static source assertions with a node-driven behavioral test (11 cases) covering the real action vocabularies, exclusions, case-insensitivity, null-arg safety, and the durable-attribute persistence guard. Co-authored-by: rodboev <rodboev@users.noreply.github.com>
2026-06-05 22:41:43 +00:00
parent e663bc98d6
commit b26bb559d5
4 changed files with 234 additions and 4 deletions
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -3,6 +3,11 @@

 ## [Unreleased]

+## [v0.51.282] — 2026-06-05 — Release IX (stage-3544 — surface memory/skill saves in Activity summary)
+
+### Added
+- **The collapsed Activity summary now shows when the agent saved a memory or updated a skill** — e.g. "Activity: 2 tools, 1 memory saved, 1 skill updated" — so persistent-state changes are visible at a glance without expanding the group. Detection matches the real tool action vocabularies (`memory`: add/replace count as saves, `remove` excluded; `skill_manage`: create/patch/edit/write_file count as updates, delete/remove_file excluded), and only completed, non-errored calls are counted. The memory/skill counts are subtracted from the tool count so it reflects only non-memory/skill tools. Classification is stamped as durable `data-*` attributes so the suffix survives the live tool-call group's HTML snapshot/restore on session switch. Sessions with no memory/skill writes render the unchanged "Activity: N tools" label. (#3544, @rodboev; closes #3340)
+
 ## [v0.51.281] — 2026-06-05 — Release IW (stage-verdigris — Verdigris emerald/bronze skin)

 ### Added
--- a/static/ui.js
+++ b/static/ui.js
@@ -7672,6 +7672,28 @@ function _toolDisplayName(tc){
  if(name==='delegate_task') return 'Delegate task';
  return name;
 }
+
+// Activity-summary detection for persisted memory/skill writes (#3340, #3544).
+// Action vocabularies match the real agent tool enums:
+//   memory.action      = add | replace | remove   (add/replace persist content → "saved")
+//   skill_manage.action= create | patch | edit | delete | write_file | remove_file
+//                        (create/patch/edit/write_file mutate a skill → "updated")
+// Deletions (memory 'remove', skill 'delete'/'remove_file') are intentionally
+// excluded so the "saved"/"updated" label verbs stay accurate; running/errored
+// calls are excluded so only completed writes are counted.
+const _MEMORY_SAVE_ACTIONS=new Set(['add','replace']);
+const _SKILL_UPDATE_ACTIONS=new Set(['create','patch','edit','write_file']);
+function _tcAction(tc){
+  return String((tc&&tc.args&&tc.args.action)||'').toLowerCase();
+}
+function _isMemorySave(tc){
+  if(!tc||tc.name!=='memory'||tc.done===false||tc.is_error) return false;
+  return _MEMORY_SAVE_ACTIONS.has(_tcAction(tc));
+}
+function _isSkillUpdate(tc){
+  if(!tc||tc.name!=='skill_manage'||tc.done===false||tc.is_error) return false;
+  return _SKILL_UPDATE_ACTIONS.has(_tcAction(tc));
+}
 function toolIcon(name){
  const icons={
    terminal:        li('terminal'),
@@ -7798,6 +7820,15 @@ function buildToolCard(tc){
        </div>`:''}
      </div>`:''}
    </div>`;
+  row._tcData = tc;
+  // Durable classification flags: _tcData (a JS property) does NOT survive the
+  // outerHTML/innerHTML snapshot+restore the live tool-call group uses on session
+  // switch/restore, which would make _syncToolCallGroupSummary re-count restored
+  // memory/skill rows as generic tools and silently drop the suffix. Mirror the
+  // classification onto data-* attributes so it survives serialization. (#3544)
+  if(_isMemorySave(tc)){row.setAttribute('data-memory-save','1');row.removeAttribute('data-skill-update');}
+  else if(_isSkillUpdate(tc)){row.setAttribute('data-skill-update','1');row.removeAttribute('data-memory-save');}
+  else {row.removeAttribute('data-memory-save');row.removeAttribute('data-skill-update');}
  return row;
 }

@@ -7847,10 +7878,25 @@ function _syncToolCallGroupSummary(group){
  const label=group.querySelector('.tool-call-group-label');
  const durationEl=group.querySelector('.tool-call-group-duration');
  if(label){
+    const rows=Array.from(group.querySelectorAll('.tool-card-row'));
+    // Prefer the live _tcData classification; fall back to the durable data-*
+    // flags for rows restored from an HTML snapshot (which drops JS properties).
+    const isMem=r=>_isMemorySave(r._tcData)||r.getAttribute('data-memory-save')==='1';
+    const isSkill=r=>_isSkillUpdate(r._tcData)||r.getAttribute('data-skill-update')==='1';
+    const memCount=rows.filter(isMem).length;
+    const skillCount=rows.filter(r=>!isMem(r)&&isSkill(r)).length;
+    const otherCount=Math.max(0, toolCount-memCount-skillCount);
+    let suffix='';
+    if(memCount) suffix+=`, ${memCount} ${memCount===1?'memory':'memories'} saved`;
+    if(skillCount) suffix+=`, ${skillCount} ${skillCount===1?'skill':'skills'} updated`;
+    const toolsPart=otherCount?`${otherCount} tool${otherCount===1?'':'s'}`:'';
    if(group.getAttribute('data-live-tool-call-group')==='1'){
-      label.textContent=toolCount?`Activity: ${toolCount} tool${toolCount===1?'':'s'}`:'Activity · Running';
-    }else if(toolCount) label.textContent=`Activity: ${toolCount} tool${toolCount===1?'':'s'}`;
-    else label.textContent='Activity';
+      if(toolsPart) label.textContent=`Activity: ${toolsPart}${suffix}`;
+      else if(suffix) label.textContent=`Activity: ${suffix.slice(2)}`;
+      else label.textContent='Activity · Running';
+    }else if(toolsPart||suffix){
+      label.textContent=toolsPart?`Activity: ${toolsPart}${suffix}`:`Activity: ${suffix.slice(2)}`;
+    }else label.textContent='Activity';
    label.setAttribute('data-sweep-label', label.textContent);
  }
  if(durationEl){
--- a/tests/test_issue1824_cli_patch_diff_rendering.py
+++ b/tests/test_issue1824_cli_patch_diff_rendering.py
@@ -116,6 +116,10 @@ def test_rendered_apply_patch_tool_card_html_contains_diff_lines():
        # #3336: buildToolCard now wraps diff snippets via these helpers.
        "_snippetLooksLikeDiff",
        "_colorDiffLines",
+        # #3544: buildToolCard stamps durable memory/skill-save flags via these.
+        "_tcAction",
+        "_isMemorySave",
+        "_isSkillUpdate",
        "buildToolCard",
    ]
    functions = "\n".join(_function_source(UI_JS, name) for name in function_names)
@@ -125,8 +129,12 @@ def test_rendered_apply_patch_tool_card_html_contains_diff_lines():
        function li(){{return '';}}
        function toolIcon(){{return '';}}
        function _toolDisplayName(tc){{return tc.name||'tool';}}
+        // #3544: const Sets the _isMemorySave/_isSkillUpdate predicates close over
+        // (extracted helpers reference these module-level constants).
+        const _MEMORY_SAVE_ACTIONS=new Set(['add','replace']);
+        const _SKILL_UPDATE_ACTIONS=new Set(['create','patch','edit','write_file']);
        const document={{
-          createElement(){{return {{className:'', innerHTML:''}};}}
+          createElement(){{return {{className:'', innerHTML:'', setAttribute(){{}}, removeAttribute(){{}}}};}}
        }};
        {functions}

--- a/tests/test_memory_skill_badge.py
+++ b/tests/test_memory_skill_badge.py
@@ -0,0 +1,171 @@
+"""Behavioral test for the Activity-summary memory/skill-save counter (#3340, #3544).
+
+The original PR (#3544) shipped only *static source* assertions and gated detection
+on action names {save, create, update, upsert} — which do NOT match the real agent
+tool enums (memory.action = add|replace|remove; skill_manage.action = create|patch|
+edit|delete|write_file|remove_file), so the counter never fired on real saves.
+
+This test EXTRACTS the real detection helpers from static/ui.js and DRIVES them with
+the authentic tool-call shapes, asserting the counts are correct. It is the
+RED/GREEN guard for the corrected action vocabularies.
+"""
+import json
+import pathlib
+import shutil
+import subprocess
+
+import pytest
+
+_ROOT = pathlib.Path(__file__).resolve().parent.parent
+_UI_JS = (_ROOT / "static" / "ui.js").read_text(encoding="utf-8")
+NODE = shutil.which("node")
+
+pytestmark = pytest.mark.skipif(NODE is None, reason="node not on PATH")
+
+
+def _extract_helpers() -> str:
+    """Pull the memory/skill detection helper block out of ui.js verbatim."""
+    start = _UI_JS.index("const _MEMORY_SAVE_ACTIONS")
+    end = _UI_JS.index("function _isSkillUpdate")
+    end = _UI_JS.index("}", end) + 1
+    block = _UI_JS[start:end]
+    # sanity: the block must define both predicates
+    assert "_isMemorySave" in block and "_isSkillUpdate" in block
+    return block
+
+
+def _run(tool_calls):
+    assert NODE is not None  # guarded by pytestmark skipif
+    helpers = _extract_helpers()
+    js = (
+        helpers
+        + "\nconst tcs = " + json.dumps(tool_calls) + ";\n"
+        + "const mem = tcs.filter(_isMemorySave).length;\n"
+        + "const skill = tcs.filter(_isSkillUpdate).length;\n"
+        + "console.log(JSON.stringify({mem, skill}));\n"
+    )
+    r = subprocess.run([NODE, "-e", js], capture_output=True, text=True, timeout=30)
+    if r.returncode != 0:
+        raise RuntimeError(f"node failed: {r.stderr}")
+    return json.loads(r.stdout.strip())
+
+
+def test_real_memory_actions_are_counted_as_saved():
+    # The authoritative memory tool enum is add | replace | remove.
+    out = _run([
+        {"name": "memory", "args": {"action": "add"}, "done": True},
+        {"name": "memory", "args": {"action": "replace"}, "done": True},
+    ])
+    assert out["mem"] == 2, "add/replace must count as memory saves"
+
+
+def test_memory_remove_is_not_counted_as_saved():
+    out = _run([{"name": "memory", "args": {"action": "remove"}, "done": True}])
+    assert out["mem"] == 0, "'remove' is a deletion, not a save"
+
+
+def test_legacy_action_names_do_not_falsely_match():
+    # The original PR's vocabulary {save, update, upsert} is NOT the real enum.
+    out = _run([
+        {"name": "memory", "args": {"action": "save"}, "done": True},
+        {"name": "memory", "args": {"action": "upsert"}, "done": True},
+    ])
+    assert out["mem"] == 0, "non-enum action names must not match"
+
+
+def test_real_skill_actions_are_counted_as_updated():
+    # skill_manage enum: create | patch | edit | delete | write_file | remove_file
+    out = _run([
+        {"name": "skill_manage", "args": {"action": "create"}, "done": True},
+        {"name": "skill_manage", "args": {"action": "patch"}, "done": True},
+        {"name": "skill_manage", "args": {"action": "edit"}, "done": True},
+        {"name": "skill_manage", "args": {"action": "write_file"}, "done": True},
+    ])
+    assert out["skill"] == 4, "create/patch/edit/write_file must count as skill updates"
+
+
+def test_skill_deletions_are_not_counted_as_updated():
+    out = _run([
+        {"name": "skill_manage", "args": {"action": "delete"}, "done": True},
+        {"name": "skill_manage", "args": {"action": "remove_file"}, "done": True},
+    ])
+    assert out["skill"] == 0, "delete/remove_file are not 'updates'"
+
+
+def test_running_and_errored_writes_are_excluded():
+    out = _run([
+        {"name": "memory", "args": {"action": "add"}, "done": False},
+        {"name": "memory", "args": {"action": "add"}, "is_error": True, "done": True},
+        {"name": "skill_manage", "args": {"action": "create"}, "done": False},
+    ])
+    assert out == {"mem": 0, "skill": 0}, "in-progress/errored writes must not count"
+
+
+def test_non_memory_skill_tools_ignored():
+    out = _run([
+        {"name": "terminal", "args": {"action": "add"}, "done": True},
+        {"name": "read_file", "args": {}, "done": True},
+        {"name": "skills_list", "args": {"action": "create"}, "done": True},
+    ])
+    assert out == {"mem": 0, "skill": 0}, "only memory/skill_manage are inspected"
+
+
+def test_action_matching_is_case_insensitive():
+    out = _run([
+        {"name": "memory", "args": {"action": "ADD"}, "done": True},
+        {"name": "skill_manage", "args": {"action": "Patch"}, "done": True},
+    ])
+    assert out == {"mem": 1, "skill": 1}
+
+
+def test_missing_args_does_not_throw():
+    out = _run([
+        {"name": "memory", "done": True},
+        {"name": "skill_manage", "args": None, "done": True},
+    ])
+    assert out == {"mem": 0, "skill": 0}
+
+
+def test_classification_persisted_as_durable_dom_attributes():
+    """Regression guard (Codex catch on #3544): _tcData is a JS property that does
+    NOT survive the outerHTML/innerHTML snapshot+restore the live tool-call group
+    uses on session switch/restore. If classification lived ONLY on _tcData, a
+    restored memory/skill row would be re-counted as a generic tool and the suffix
+    would silently vanish. buildToolCard must therefore ALSO stamp durable data-*
+    attributes, and the summary must count them as a fallback.
+    """
+    # (a) buildToolCard stamps durable attributes for classified rows
+    assert "data-memory-save" in _UI_JS, "buildToolCard must stamp a durable memory flag"
+    assert "data-skill-update" in _UI_JS, "buildToolCard must stamp a durable skill flag"
+    build_start = _UI_JS.index("function buildToolCard(tc)")
+    build_end = _UI_JS.index("\nfunction ", _UI_JS.index("return row;", build_start))
+    build_block = _UI_JS[build_start:build_end]
+    assert "setAttribute('data-memory-save'" in build_block
+    assert "setAttribute('data-skill-update'" in build_block
+
+    # (b) the summary counts the durable attributes as a fallback when _tcData is gone
+    sync_start = _UI_JS.index("function _syncToolCallGroupSummary(group)")
+    sync_block = _UI_JS[sync_start:_UI_JS.index("if(durationEl)", sync_start)]
+    assert "data-memory-save" in sync_block, "summary must fall back to the durable memory flag"
+    assert "data-skill-update" in sync_block, "summary must fall back to the durable skill flag"
+
+
+def test_durable_flags_match_live_classification():
+    """The data-* fallback must classify identically to the live _tcData predicates,
+    so a restored row counts the same as a fresh one. Drive both predicates and
+    assert the attribute logic mirrors them for the real action vocabularies.
+    """
+    # add/replace/patch/edit/create/write_file → flagged; remove/delete/remove_file → not
+    saved = _run([
+        {"name": "memory", "args": {"action": "add"}, "done": True},
+        {"name": "memory", "args": {"action": "replace"}, "done": True},
+        {"name": "skill_manage", "args": {"action": "create"}, "done": True},
+        {"name": "skill_manage", "args": {"action": "write_file"}, "done": True},
+    ])
+    assert saved == {"mem": 2, "skill": 2}
+    excluded = _run([
+        {"name": "memory", "args": {"action": "remove"}, "done": True},
+        {"name": "skill_manage", "args": {"action": "delete"}, "done": True},
+        {"name": "skill_manage", "args": {"action": "remove_file"}, "done": True},
+    ])
+    assert excluded == {"mem": 0, "skill": 0}